Bug 102539 – Deploy ScriptWrappable to more always-wrapped objects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Deploy ScriptWrappable to more always-wrapped objects

[Eric Seidel (no email)]

Created attachment 174725 [details]
Patch

[Eric Seidel (no email)]

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;
     }

[Eric Seidel (no email)]

Created attachment 174727 [details]
Example output from debugging code

I recommend using:
cat objects.txt| sort | uniq -c | sort -r 
to understand the output.

[Eric Seidel (no email)]

(Also, note that because I was lazy and used String::show(), the above code only works in Debug builds.)

[Adam Barth]

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.

[Adam Barth]

We should investigate CSSStyleDeclaration as well since that show up in your sampling.

[Eric Seidel (no email)]

Created attachment 174728 [details]
Patch

[Eric Seidel (no email)]

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. :)

[Adam Barth]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 174728 [details]
Patch

Talked to Antti.  Will add CSSStyleDeclaration to the list.

[Eric Seidel (no email)]

Created attachment 174737 [details]
Patch for landing

[Eric Seidel (no email)]

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 :)

[WebKit Review Bot]

Comment on attachment 174737 [details]
Patch for landing

Clearing flags on attachment: 174737

Committed r135001: <http://trac.webkit.org/changeset/135001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

This is insanely neat! :)

[Adam Barth]

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.

[Eric Seidel (no email)]

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

[Andreas Kling]

(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.

[Ojan Vafai]

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

[Eric Seidel (no email)]

(In reply to comment #17)
> Nice work!  We should run your profiling code more and find more candidates.

Done in bug 102601.