presentationAttributeCacheMaximumSize is set too low

• Component: CSS
• Status: RESOLVED FIXED

[Julien Chaffraix]

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.

[Julien Chaffraix]

Created attachment 134932 [details]
Proposed change.

[Antti Koivisto]

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]

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

[Antti Koivisto]

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 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]

Created attachment 135430 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 135430 [details]
Patch for landing

Clearing flags on attachment: 135430

Committed r113098: <http://trac.webkit.org/changeset/113098>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.