Bug 80250

Summary: [JSC] Cache the CSSPropertyID in JSCSSStyleDeclaration for performance optimization
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: akling, arv, benjamin, ggaren, koivisto, menard, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Test HTML
none
Patch
none
Patch
none
Modified version of the test
none
patch for landing none

Description Kentaro Hara 2012-03-05 00:43:12 PST
V8CSSStyleDeclaration caches the calculated CSSPropertyID. Similarly, we can implement the cache in JSCSSStyleDeclaration.

This is a follow-up patch of bug 79014.
Comment 1 Kentaro Hara 2012-03-05 00:51:02 PST
Created attachment 130071 [details]
Patch
Comment 2 Kentaro Hara 2012-03-05 00:51:57 PST
I wanted to measure the performance by DOM/Accessors.html, but run-perf-tests are not working just right now...
Comment 3 Benjamin Poulain 2012-03-05 00:58:49 PST
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.
Comment 4 Benjamin Poulain 2012-03-05 01:03:30 PST
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.
Comment 5 Kentaro Hara 2012-03-05 01:04:30 PST
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.
Comment 6 Kentaro Hara 2012-03-05 01:07:52 PST
(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.
Comment 7 Kentaro Hara 2012-03-05 01:24:09 PST
Created attachment 130074 [details]
Patch
Comment 8 Kentaro Hara 2012-03-05 01:25:34 PST
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.)
Comment 9 Benjamin Poulain 2012-03-05 01:29:53 PST
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.
Comment 10 Benjamin Poulain 2012-03-05 01:32:36 PST
(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 :)
Comment 11 Kentaro Hara 2012-03-05 01:33:43 PST
(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().
Comment 12 Kentaro Hara 2012-03-05 01:52:00 PST
Created attachment 130082 [details]
Patch
Comment 13 Kentaro Hara 2012-03-05 01:52:49 PST
Benjamin: Updated the patch

- Without the patch: 225ms 1278ms
- With the patch: 149ms 1199ms
Comment 14 Alexis Menard (darktears) 2012-03-05 09:32:11 PST
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.
Comment 15 Benjamin Poulain 2012-03-05 14:19:56 PST
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.
Comment 16 Benjamin Poulain 2012-03-05 14:26:45 PST
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.
Comment 17 Kentaro Hara 2012-03-05 15:45:08 PST
Created attachment 130215 [details]
patch for landing
Comment 18 Kentaro Hara 2012-03-05 15:48:21 PST
(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>'
Comment 19 WebKit Review Bot 2012-03-05 17:51:18 PST
Comment on attachment 130215 [details]
patch for landing

Clearing flags on attachment: 130215

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