Bug 82816 - presentationAttributeCacheMaximumSize is set too low
: presentationAttributeCacheMaximumSize is set too low
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 78070
  Show dependency treegraph
 
Reported: 2012-03-30 19:20 PST by
Modified: 2012-04-03 15:39 PST (History)


Attachments
Proposed change. (2.56 KB, patch)
2012-03-30 19:36 PST, Julien Chaffraix
no flags Review Patch | 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 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (5.39 KB, patch)
2012-04-03 14:31 PST, Julien Chaffraix
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-03-30 19:20:16 PST
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 From 2012-03-30 19:36:40 PST -------
Created an attachment (id=134932) [details]
Proposed change.
------- Comment #2 From 2012-03-31 06:58:00 PST -------
(From update of attachment 134932 [details])
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 From 2012-04-02 15:34:59 PST -------
Created an attachment (id=135207) [details]
Updated change. Add a timer based clearing mechanism per Antti's suggestion.
------- Comment #4 From 2012-04-03 12:59:23 PST -------
(From update of attachment 135207 [details])
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 From 2012-04-03 13:38:30 PST -------
(From update of attachment 135207 [details])
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 From 2012-04-03 14:31:16 PST -------
Created an attachment (id=135430) [details]
Patch for landing
------- Comment #7 From 2012-04-03 15:39:30 PST -------
(From update of attachment 135430 [details])
Clearing flags on attachment: 135430

Committed r113098: <http://trac.webkit.org/changeset/113098>
------- Comment #8 From 2012-04-03 15:39:34 PST -------
All reviewed patches have been landed.  Closing bug.