Bug 82816 - presentationAttributeCacheMaximumSize is set too low
Summary: presentationAttributeCacheMaximumSize is set too low
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks: 78070
  Show dependency treegraph
 
Reported: 2012-03-30 19:20 PDT by Julien Chaffraix
Modified: 2012-04-03 15:39 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 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.
Comment 1 Julien Chaffraix 2012-03-30 19:36:40 PDT
Created attachment 134932 [details]
Proposed change.
Comment 2 Antti Koivisto 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.
Comment 3 Julien Chaffraix 2012-04-02 15:34:59 PDT
Created attachment 135207 [details]
Updated change. Add a timer based clearing mechanism per Antti's suggestion.
Comment 4 Antti Koivisto 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
Comment 5 Julien Chaffraix 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.
Comment 6 Julien Chaffraix 2012-04-03 14:31:16 PDT
Created attachment 135430 [details]
Patch for landing
Comment 7 WebKit Review Bot 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>
Comment 8 WebKit Review Bot 2012-04-03 15:39:34 PDT
All reviewed patches have been landed.  Closing bug.