[JSC] Cache the CSSPropertyID in JSCSSStyleDeclaration for performance optimization

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kentaro Hara]

V8CSSStyleDeclaration caches the calculated CSSPropertyID. Similarly, we can implement the cache in JSCSSStyleDeclaration.

This is a follow-up patch of bug 79014.

[Kentaro Hara]

Created attachment 130071 [details]
Patch

[Kentaro Hara]

I wanted to measure the performance by DOM/Accessors.html, but run-perf-tests are not working just right now...

[Benjamin Poulain]

I actually intentionally avoided the cache because it offered only a small speedup on positive match but a significant impact on negative match.

Are you sure of your numbers? The 35% does not match at all what I had when doing exactly the same thing.

[Benjamin Poulain]

Comment on attachment 130071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130071&action=review

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:181
> +    typedef HashMap<StringImpl*, CSSPropertyInfo*> CSSPropertyInfoMap;
> +    DEFINE_STATIC_LOCAL(CSSPropertyInfoMap, map, ());
> +    CSSPropertyInfo* propertyInfo = map.get(propertyNameString);

Oh, I see the difference with my test now, your key is the pointer to string, not the string.

This does not seems like a good idea, nothing ensure you will keep this StringImpl in memory.

[Kentaro Hara]

Created attachment 130073 [details]
Test HTML

> I actually intentionally avoided the cache because it offered only a small speedup on positive match but a significant impact on negative match.
> 
> Are you sure of your numbers? The 35% does not match at all what I had when doing exactly the same thing.

I attached the HTML for performance tests. In my AppleWebKit/Mac environment,

- Without the patch: 225ms 1278ms
- With the patch: 149ms 1188ms

Also I confirmed that if I remove the cache from V8CSSStyleDeclaration, DOM/Accessors.html's performance degrades much.

[Kentaro Hara]

(In reply to comment #4)
> (From update of attachment 130071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130071&action=review
> 
> > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:181
> > +    typedef HashMap<StringImpl*, CSSPropertyInfo*> CSSPropertyInfoMap;
> > +    DEFINE_STATIC_LOCAL(CSSPropertyInfoMap, map, ());
> > +    CSSPropertyInfo* propertyInfo = map.get(propertyNameString);
> 
> Oh, I see the difference with my test now, your key is the pointer to string, not the string.
> 
> This does not seems like a good idea, nothing ensure you will keep this StringImpl in memory.

Ah, makes sense (though I am not sure how much we cannot ensure it in the real world). Then let me try to cache the String. I guess that it would have some performance impact, judging from the fact that removing the cache from V8CSSStyleDeclaration degrades the performance much.

[Kentaro Hara]

Created attachment 130074 [details]
Patch

[Kentaro Hara]

Benjamin: Updated the patch

- Without the patch: 225ms 1278ms
- With the patch: 141ms 1170ms

(Maybe I need to measure the performance overhead for cache-miss cases though.)

[Benjamin Poulain]

Comment on attachment 130074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130074&action=review

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:180
> +    typedef HashMap<String, CSSPropertyInfo*> CSSPropertyInfoMap;

I would try having CSSPropertyInfo by value instead of allocating them.

[Benjamin Poulain]

(In reply to comment #8)
> Benjamin: Updated the patch

Thanks, I will definitely try your patch and try to find why we have different outcome.

But tomorrow, tonight I am heading to bed :)

[Kentaro Hara]

(In reply to comment #10)
> (In reply to comment #8)
> Thanks, I will definitely try your patch and try to find why we have different outcome.
> 
> But tomorrow, tonight I am heading to bed :)

OK, thanks. I'll upload the patch that does not allocate CSSPropertyInfo().

[Kentaro Hara]

Created attachment 130082 [details]
Patch

[Kentaro Hara]

Benjamin: Updated the patch

- Without the patch: 225ms 1278ms
- With the patch: 149ms 1199ms

[Alexis Menard (darktears)]

Comment on attachment 130082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130082&action=review

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:180
> +    String stringForCache = String(propertyNameString);

Identifier already store a UString internally, maybe you can use this one as key of the map.  That would avoid to create the String. Does it bring benefits? Maybe not.

[Benjamin Poulain]

Created attachment 130205 [details]
Modified version of the test

Here is a slightly modified version of the test.

The problem I have on my hardware is the 3 first test are faster, and the last one is almost twice as slow.

[Benjamin Poulain]

Comment on attachment 130082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130082&action=review

This seems like a good idea., the trade off looks very reasonable. Let's see how that perform in the wild :)

> Source/WebCore/ChangeLog:13
> +        In my local Mac environment, this optimization improves the performance
> +        of CSS property getters by 35%, and the performance of CSS property setters
> +        by 8%.

Are those numbers still accurate after your update?

> Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:183
> +    typedef HashMap<String, CSSPropertyInfo> CSSPropertyInfoMap;
> +    DEFINE_STATIC_LOCAL(CSSPropertyInfoMap, map, ());
> +    propertyInfo = map.get(stringForCache);

I would prefer propertyInfoCache or something more descriptive instead of map.

[Kentaro Hara]

Created attachment 130215 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #16)
> > Source/WebCore/ChangeLog:13
> > +        In my local Mac environment, this optimization improves the performance
> > +        of CSS property getters by 35%, and the performance of CSS property setters
> > +        by 8%.
> 
> Are those numbers still accurate after your update?

I confirmed it is still accurate.

> > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:183
> > +    typedef HashMap<String, CSSPropertyInfo> CSSPropertyInfoMap;
> > +    DEFINE_STATIC_LOCAL(CSSPropertyInfoMap, map, ());
> > +    propertyInfo = map.get(stringForCache);
> 
> I would prefer propertyInfoCache or something more descriptive instead of map.

Done. Thanks for reviewing!

(In reply to comment #14)
> (From update of attachment 130082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130082&action=review
> 
> > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:180
> > +    String stringForCache = String(propertyNameString);
> 
> Identifier already store a UString internally, maybe you can use this one as key of the map.  That would avoid to create the String. Does it bring benefits? Maybe not.

It seems that we cannot use a UString as a hashmap key:

/Users/haraken/WebKit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/HashTable.h:355: error: 'isDeletedValue' is not a member of 'WTF::HashTraits<JSC::UString>'

[WebKit Review Bot]

Comment on attachment 130215 [details]
patch for landing

Clearing flags on attachment: 130215

Committed r109829: <http://trac.webkit.org/changeset/109829>