WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
112084
Add a single character cache to WidthCache
https://bugs.webkit.org/show_bug.cgi?id=112084
Summary
Add a single character cache to WidthCache
Benjamin Poulain
Reported
2013-03-11 17:42:36 PDT
Add a single character cache to WidthCache
Attachments
Patch
(4.15 KB, patch)
2013-03-11 17:47 PDT
,
Benjamin Poulain
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Benjamin Poulain
Comment 1
2013-03-11 17:47:34 PDT
Created
attachment 192608
[details]
Patch
Andreas Kling
Comment 2
2013-03-11 18:46:33 PDT
Would it make sense to special-case even harder with a 256-entry array for the Latin1 characters?
Benjamin Poulain
Comment 3
2013-03-11 19:21:44 PDT
(In reply to
comment #2
)
> Would it make sense to special-case even harder with a 256-entry array for the Latin1 characters?
To test your hypothesis, I loaded a bunch of wikipedia pages and dumped the characters in a file. Here are all the characters used in that case: '\n', ' ', '"', "'", ')', '(', '\xab', '-', ',', '/', '.', '1', '0', '3', '2', '5', '4', '7', '6', '9', '8', 'E', '\xa0', '[', ']', 'a', 'c', 'e', 'g', 'f', 'i', 'h', 'k', 'j', 'l', 't', '|' The single space is the most common one, with 2773 out of 4269 chars. Given that the data is sparse, I think it is not needed to have a special case for Latin1 characters. What do you think?
Geoffrey Garen
Comment 4
2013-03-12 08:58:15 PDT
Comment on
attachment 192608
[details]
Patch 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.
Benjamin Poulain
Comment 5
2013-03-12 15:41:03 PDT
Comment on
attachment 192608
[details]
Patch Clearing flags on attachment: 192608 Committed
r145601
: <
http://trac.webkit.org/changeset/145601
>
Benjamin Poulain
Comment 6
2013-03-12 15:41:04 PDT
All reviewed patches have been landed. Closing bug.
Benjamin Poulain
Comment 7
2013-03-12 15:44:32 PDT
> 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.
I completely agree with you. Do you have good test cases I can use to do this analysis?
Geoffrey Garen
Comment 8
2013-03-12 16:09:31 PDT
PerformanceTests/Layout/chapter-reflow*.html and PLT3 are what I would use to test.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug