V8CSSStyleDeclaration caches the calculated CSSPropertyID. Similarly, we can implement the cache in JSCSSStyleDeclaration. This is a follow-up patch of bug 79014.
Created attachment 130071 [details] Patch
I wanted to measure the performance by DOM/Accessors.html, but run-perf-tests are not working just right now...
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 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.
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.
(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.
Created attachment 130074 [details] Patch
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 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.
(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 :)
(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().
Created attachment 130082 [details] Patch
Benjamin: Updated the patch - Without the patch: 225ms 1278ms - With the patch: 149ms 1199ms
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.
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 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.
Created attachment 130215 [details] patch for landing
(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 on attachment 130215 [details] patch for landing Clearing flags on attachment: 130215 Committed r109829: <http://trac.webkit.org/changeset/109829>