Deploy ScriptWrappable to more always-wrapped objects
Created attachment 174725 [details] Patch
This is the patch I used to find which objects should use ScriptWrappable: +++ b/Source/WebCore/bindings/js/JSDOMBinding.h @@ -190,7 +190,12 @@ enum ParameterDefaultPolicy { WrapperClass* wrapper = WrapperClass::create(getDOMStructure<WrapperClass>(exec, globalObject), globalObject, node); // FIXME: The entire function can be removed, once we fix caching. // This function is a one-off hack to make Nodes cache in the right global object. - cacheWrapper(currentWorld(exec), node, wrapper); + DOMWrapperWorld* world = currentWorld(exec); + cacheWrapper(world, node, wrapper); + if (world->isNormal()) { + if (!getInlineCachedWrapper(world, node)) + wrapper->toString(exec)->value(exec).show(); + } return wrapper; }
Created attachment 174727 [details] Example output from debugging code I recommend using: cat objects.txt| sort | uniq -c | sort -r to understand the output.
(Also, note that because I was lazy and used String::show(), the above code only works in Debug builds.)
Comment on attachment 174725 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174725&action=review > Source/WebCore/html/HTMLCollection.h:41 > -class HTMLCollectionCacheBase : public DynamicNodeListCacheBase { > +class HTMLCollectionCacheBase : public ScriptWrappable, public DynamicNodeListCacheBase { I would move the ScriptWrappable base class to HTMLCollection so that it can be next to the RefCounted base class. That will help us avoid having two copies of the base class by accident.
We should investigate CSSStyleDeclaration as well since that show up in your sampling.
Created attachment 174728 [details] Patch
Yup. I just have to make sure that CSSStyleDeclaration is only used by JS and not by the DOM implementation. If it's used more often by the DOM implementation than from JS than it may not be a memory win. We might also mark CSSComputedStyleDeclaration as ScriptWrappable and not the baseclass. I just don't know yet. :)
Comment on attachment 174728 [details] Patch I think the only question about this patch is whether to do Event. Given that events are very commonly used in script, I suspect its a win.
Yes, I do think Event is the only questionable one here. I suspect Events are often created w/o being wrapped for JS, but given that they're short-lived, the only question is if by making them 4-8bytes larger here are we bumping them up to the next allocation bucket. If they live longer than the dispatch, they'll always be wrapped, and thus the ScriptWrappable here is a memory (and perf) win.
Comment on attachment 174728 [details] Patch Talked to Antti. Will add CSSStyleDeclaration to the list.
Created attachment 174737 [details] Patch for landing
Antti and my conversation for posterity (I removed the overlapping conversations in the channel, as well as a couple unrelated parts of the conversation): 11:40 AM <eseidel> anttik: around? Are CSSStyleDeclaration sub-classes used by the DOM implementation or do they only exist to be exposed to JS? 11:44 AM <anttik> eseidel: there might still be some editing code around that uses it 11:44 AM <anttik> eseidel: but the idea is that it is a pure script proxy 11:44 AM <anttik> only instantiated as a result of JS API access 11:45 AM <eseidel> anttik: fantastic. then I'll make it ScriptWrappable (will store a direct pointer to its DOM Wrapper, saving the more expensive HashSet entry and making access faster in the main world) 11:45 AM <anttik> eseidel: sounds good to me 11:46 AM <anttik> eseidel: generally they exist in low numbers so bloating them is not bad 11:49 AM <anttik> eseidel: all the CSSRule subclasses are similar 11:50 AM <eseidel> anttik: will fix them too then :) 11:50 AM <eseidel> anttik: to be clear, those should only be intantiated when they have a wrapper from JS? 11:50 AM <anttik> yes 11:50 AM <eseidel> anttik: or can they be intantiated other times but not wrapped? 11:50 AM <eseidel> anttik: aka, they're not created eagerly when some other CSSOM object is? 11:50 AM <anttik> there are some cases related to inspector 11:50 AM <anttik> where they exist without js object 11:51 AM <eseidel> anttik: which isn't the main world, so it won't benifit from ScriptWrappable, sadly 11:51 AM <eseidel> anttik: k 11:51 AM <anttik> but not in normal engine work 11:51 AM <eseidel> anttik: but your guess is that it's correct for CSSRule to be ScriptWrappable, given what context you have? 11:52 AM <eseidel> ScriptWrappable adds a direct pointer to the wrapper, used by the main world, avoiding needing a a hash-entry in order to map from the impl to the JS object 11:52 AM <anttik> eseidel: i haven't really wrapped my head around ScriptWrappable 11:52 AM <anttik> but that sound likely 11:52 AM <eseidel> which saves memory and speeds up the wrapping/unwrapping for the main world 11:52 AM <eseidel> anttik: I'm happy to explain it to you at greater length if that's of use to you 11:52 AM <eseidel> anttik: Nodes used to be the only ScriptWrappable until about a week ago 11:53 AM <eseidel> anttik: Nodes carried a direct pointer to their wrapper in the main world 11:53 AM <eseidel> anttik: now any class can 11:53 AM <eseidel> anttik: if it inherits from ScriptWrappable 11:53 AM <eseidel> and the bindings just magically do the right thing 11:53 AM <eseidel> the main world is were all the normal stuff goes. the inspector is the non-main world 11:53 AM <eseidel> I think in JSC parlance it's the "normal" world instead of "main" 11:54 AM <eseidel> the tradeoff is the pointer on the object (4 or 8 bytes) instead of the hash entry, which is 2 pointers 11:54 AM <anttik> eseidel: yeah, CSSRules sound like valid client for this. the only catch is that virtually no one actually uses CSSRules so none of this matter either way :) 11:54 AM <eseidel> anttik: k :) 11:54 AM <eseidel> anttik: I'll leave them non-ScriptWrappable for now for simplicity then 11:54 AM <anttik> CSSStyleDeclarations are pretty popular 11:55 AM <anttik> so that should be good 11:55 AM <eseidel> anttik: yeah, they showed up in my debug logging 11:55 AM <eseidel> anttik: I hadn't wrapped them yet as I didn't know if they were used by the dom Imple or not 11:55 AM <eseidel> anttik: but thankfully you set me straight :)
Comment on attachment 174737 [details] Patch for landing Clearing flags on attachment: 174737 Committed r135001: <http://trac.webkit.org/changeset/135001>
All reviewed patches have been landed. Closing bug.
This is insanely neat! :)
7.5% improvement: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstyleprototype/report.html?rev=168399&graph=jslib_style_prototype_Prototype___getHeight__&trace=score&history=150 5% improvement: http://build.chromium.org/f/chromium/perf/chromium-rel-win7-webkit/dromaeo_domcorequery/report.html?rev=168496&graph=dom_query_getElementsByTagName__not_in_document_&trace=score&history=150 Nice work! We should run your profiling code more and find more candidates.
There isn't a lot left: 86 [object NamedNodeMap] 25 [object Plugin] 12 [object PluginArray] 11 [object Screen] 10 [object MimeTypeArray] 9 Error: SyntaxError: DOM Exception 12 4 [object DOMStringMap] 3 [object CanvasRenderingContext2D] 3 Error: InvalidCharacterError: DOM Exception 5 2 matrix(1.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000) 2 [object Geolocation] 2 [object CSSStyleSheet] 2 [object CSSRuleList] 1 Error: NotFoundError: DOM Exception 8
(In reply to comment #18) > There isn't a lot left: > > 86 [object NamedNodeMap] FWIW NamedNodeMap exists only when instantiated from DOM bindings (Node.attributes), and so would be a prime candidate for this awesome-ptimization.
Big improvement on a bunch of benchmarks: http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery____is__visible_&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery____is__visible_&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___height___x10&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___height___x10&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstyleprototype/report.html?rev=168452&graph=jslib_style_prototype_Prototype___getStyle__&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_domcorequery/report.html?rev=168452&graph=dom_query_getElementsByTagName_div_&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___css_color__x100&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___css_color__x100&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery____show__&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery____show__&trace=score&history=50 http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___width___x10&trace=score&history=50 http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=168452&graph=jslib_style_jquery_jQuery___width___x10&trace=score&history=50
(In reply to comment #17) > Nice work! We should run your profiling code more and find more candidates. Done in bug 102601.