Bug 82816

Summary:

presentationAttributeCacheMaximumSize is set too low

Product:

WebKit

Reporter:

Julien Chaffraix <jchaffraix>

Component:

CSS

Assignee:

Julien Chaffraix <jchaffraix>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

kling, koivisto, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

78070

Attachments:

Description	Flags
Proposed change.	none
Updated change. Add a timer based clearing mechanism per Antti's suggestion.	none
Patch for landing	none

Julien Chaffraix

Reported 2012-03-30 19:20:16 PDT

Before bug 77204, the presentation attribute style (called matched declaration at the time) cache used to grow unbounded. Bug 80707 re-introduced the missing cache and set the limit to 128 which gave around a 75% hit rate. This is however a regression from the original caching for a few reasons: - the hit rate on the previous cache was higher in the 85% range (beware it's not an apple-to-apple comparison due to the different modeling used for attribute styles, see 2nd point) - due to the new mapping (several attributes to one style vs 1 attribute to 1 style before), there is a combinatorial explosion compared to the previous (more simple) model. Any cache miss in this cache, unfortunately means that we *will* miss in our matched property cache as we are doing pointer comparisons. This bug proposes to bump the cache size to get back some of the performance.

Attachments
Proposed change. (2.56 KB, patch) 2012-03-30 19:36 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Updated change. Add a timer based clearing mechanism per Antti's suggestion. (4.74 KB, patch) 2012-04-02 15:34 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Patch for landing (5.39 KB, patch) 2012-04-03 14:31 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Julien Chaffraix

Comment 1 2012-03-30 19:36:40 PDT

Created attachment 134932 [details] Proposed change.

Antti Koivisto

Comment 2 2012-03-31 06:58:00 PDT

Comment on attachment 134932 [details] Proposed change. Any long running process will now gather 4096 cache entries wasting non-trivial amount of memory (in ballpark of ~1MB). I don't think thats acceptable. I think you should also add a timer based clearing mechanism. If there are no cache hits for some period of time the cache will get wiped.

Julien Chaffraix

Comment 3 2012-04-02 15:34:59 PDT

Created attachment 135207 [details] Updated change. Add a timer based clearing mechanism per Antti's suggestion.

Antti Koivisto

Comment 4 2012-04-03 12:59:23 PDT

Comment on attachment 135207 [details] Updated change. Add a timer based clearing mechanism per Antti's suggestion. View in context: https://bugs.webkit.org/attachment.cgi?id=135207&action=review r=me but please consider the comments > Source/WebCore/dom/StyledElement.cpp:77 > +class PresentationAttributeCacheClearTimer : public TimerBase { > +public: I think it would be nicer to have-a-timer than be-a-timer. > Source/WebCore/dom/StyledElement.cpp:79 > + static void didHitPresentationAttributeCache() > + { The usual style is to have a singleton instance, something like static PresentationAttributeCacheCleaner& presentationAttributeCacheCleaner() { DEFINE_STATIC_LOCAL(PresentationAttributeCacheCleaner, cleaner, ()); return cleaner; } and use non-static methods on that. > Source/WebCore/dom/StyledElement.cpp:83 > + static PresentationAttributeCacheClearTimer timer; Should use DEFINE_STATIC_LOCAL() (unless refactored as above). > Source/WebCore/dom/StyledElement.cpp:93 > + static const unsigned presentationAttributeCacheCleanTimeInSeconds = 60; > + static const int minPresentationAttributeCacheSizeForCleaning = 100; > + static const unsigned minPresentationAttributeCacheHitCountPerMinute = (100 * presentationAttributeCacheCleanTimeInSeconds) / 60; How did you come up with these values? min -> minimum

Julien Chaffraix

Comment 5 2012-04-03 13:38:30 PDT

Comment on attachment 135207 [details] Updated change. Add a timer based clearing mechanism per Antti's suggestion. View in context: https://bugs.webkit.org/attachment.cgi?id=135207&action=review The rest of the comments are good calls that I somewhat missed and will be updated. >> Source/WebCore/dom/StyledElement.cpp:93 >> + static const unsigned minPresentationAttributeCacheHitCountPerMinute = (100 * presentationAttributeCacheCleanTimeInSeconds) / 60; > > How did you come up with these values? > > min -> minimum I wanted to avoid poking the timer too often as it involves updating the timer heap. I though 1 minute for presentationAttributeCacheCleanTimeInSeconds was a good time. minPresentationAttributeCacheHitCountPerMinute and minPresentationAttributeCacheSizeForCleaning were chosen together to keep our maximum size around the original size before this patch (128) if we don't see a lot of presentation attribute. Also we don't want to clear our cache if it's hit too much as we will kill our hit rate. I thought something between once and twice a second was a good value for an average hit rate. Now most of what I say is hand waving and not backed by any peculiar data if that was the question.

Julien Chaffraix

Comment 6 2012-04-03 14:31:16 PDT

Created attachment 135430 [details] Patch for landing

WebKit Review Bot

Comment 7 2012-04-03 15:39:30 PDT

Comment on attachment 135430 [details] Patch for landing Clearing flags on attachment: 135430 Committed r113098: <http://trac.webkit.org/changeset/113098>

WebKit Review Bot

Comment 8 2012-04-03 15:39:34 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.