Bug 80250

Summary: [JSC] Cache the CSSPropertyID in JSCSSStyleDeclaration for performance optimization
Product: WebKit Reporter: Kentaro Hara <haraken@chromium.org>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken@chromium.org>
Status: RESOLVED FIXED    
Severity: Normal CC: akling@apple.com, alexis@webkit.org, arv@chromium.org, benjamin@webkit.org, ggaren@apple.com, koivisto@iki.fi, sam@webkit.org, webkit.review.bot@gmail.com
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 From 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 From 2012-03-05 00:51:02 PST -------
Created an attachment (id=130071) [details]
Patch
------- Comment #2 From 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 From 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 From 2012-03-05 01:03:30 PST -------
(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.
------- Comment #5 From 2012-03-05 01:04:30 PST -------
Created an attachment (id=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 From 2012-03-05 01:07:52 PST -------
(In reply to comment #4)
> (From update of attachment 130071 [details] [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 From 2012-03-05 01:24:09 PST -------
Created an attachment (id=130074) [details]
Patch
------- Comment #8 From 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 From 2012-03-05 01:29:53 PST -------
(From update of attachment 130074 [details])
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 From 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 From 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 From 2012-03-05 01:52:00 PST -------
Created an attachment (id=130082) [details]
Patch
------- Comment #13 From 2012-03-05 01:52:49 PST -------
Benjamin: Updated the patch

- Without the patch: 225ms 1278ms
- With the patch: 149ms 1199ms
------- Comment #14 From 2012-03-05 09:32:11 PST -------
(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.
------- Comment #15 From 2012-03-05 14:19:56 PST -------
Created an attachment (id=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 From 2012-03-05 14:26:45 PST -------
(From update of attachment 130082 [details])
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 From 2012-03-05 15:45:08 PST -------
Created an attachment (id=130215) [details]
patch for landing
------- Comment #18 From 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] [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 From 2012-03-05 17:51:18 PST -------
(From update of attachment 130215 [details])
Clearing flags on attachment: 130215

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