[webkit-reviews] review granted: [Bug 112084] Add a single character cache to WidthCache : [Attachment 192608] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 112084: Add a single character cache to WidthCache
https://bugs.webkit.org/show_bug.cgi?id=112084

Attachment 192608: Patch
https://bugs.webkit.org/attachment.cgi?id=192608&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=192608&action=review


r=me

The data here convinces me that a 256-entry fixed array is probably not worth
the space. Hashing a UChar will be cheap.

> Source/WebCore/platform/graphics/WidthCache.h:158
> +	   float *value;

"float*", please.

> Source/WebCore/platform/graphics/WidthCache.h:161
> +	       isNewEntry = addResult.isNewEntry;

I wonder if the single character should influence the cache's ramp-up strategy.


A few reasons we might not want it to:

(1) Single character hits -- especially single space -- will always be common.
So, they're not a good indicator that we're laying out lots of duplicated
words.

(2) The single character cache has a small, fixed size limit, so its memory
tradeoff is much much smaller.

(3) The single character cache has a tiny lookup cost, so its speed tradeoff is
smaller.

That change would probably require extensive testing, though.