Bug 112430

Summary: [V8] Generate specialized callbacks for the main world
Product: WebKit Reporter: Marja Hölttä <marja>
Component: New BugsAssignee: Marja Hölttä <marja>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, cmarcelo, dcarney, dgrogan, eric.carlson, esprehn+autocc, feature-media-reviews, haraken, japhet, jer.noble, jsbell, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 113013, 113017    
Bug Blocks: 110874    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Marja Hölttä 2013-03-15 04:36:51 PDT
The patch in bug 111417 added specialized getters and setters; next I'll add specialized callbacks.
Comment 1 Marja Hölttä 2013-03-21 14:41:24 PDT
Created attachment 194343 [details]
Patch
Comment 2 Marja Hölttä 2013-03-21 14:45:13 PDT
Some metrics from running the bindings perf tests:

Better:
first-child 8%
get-element-by-tag-name 11%
id-setter 13%
set-attribute 15%
typed-array-construct-from-array 13%

No change:
others

Worse:
-
Comment 3 jochen 2013-03-21 14:48:11 PDT
Comment on attachment 194343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194343&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3032
> +          $methodForMainWorld = "${interfaceName}V8Internal::${name}MethodCallbackForMainWorld";

nit. 4 spaces
Comment 4 Marja Hölttä 2013-03-21 15:16:19 PDT
Created attachment 194358 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-21 15:54:51 PDT
Comment on attachment 194358 [details]
Patch

Clearing flags on attachment: 194358

Committed r146534: <http://trac.webkit.org/changeset/146534>
Comment 6 WebKit Review Bot 2013-03-21 15:54:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Kentaro Hara 2013-03-21 17:22:28 PDT
Comment on attachment 194358 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194358&action=review

> Source/WebCore/ChangeLog:15
> +        No new tests (updated existing binding tests).

Please add tests for DOM attributes/methods that have [V8PerWorldBindings].

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2068
> +    my ($parameterCheckString, $paramIndex, %replacements) = GenerateParametersCheck($function, $interfaceName, "");

Nit: "" is not needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2582
> +    if ($function->signature->extendedAttributes->{"V8PerWorldBindings"}) {
> +        push(@implContent, "    if (currentWorldType == MainWorld) {\n");
> +        push(@implContent, "        ${conditional}$template->Set(v8::String::NewSymbol(\"$name\"), v8::FunctionTemplate::New(${interfaceName}V8Internal::${name}MethodCallbackForMainWorld, v8Undefined(), ${signature})$property_attributes);\n");
> +        push(@implContent, "    } else {\n");
> +        push(@implContent, "        ${conditional}$template->Set(v8::String::NewSymbol(\"$name\"), v8::FunctionTemplate::New(${interfaceName}V8Internal::${name}MethodCallback, v8Undefined(), ${signature})$property_attributes);\n");
> +        push(@implContent, "    }\n");
> +    } else {
> +        push(@implContent, "    ${conditional}$template->Set(v8::String::NewSymbol(\"$name\"), v8::FunctionTemplate::New(${interfaceName}V8Internal::${name}MethodCallback, v8Undefined(), ${signature})$property_attributes);\n");
> +    }

I'd guess that this change is premature. You need to first support [V8PerWorldBindings] for "NonStandard" methods (e.g. per-context methods) before making the change. Otherwise, once you add [V8PerWorldBindings] to NonStandard methods, the compile will fail because there is no xxxMethodCallbackForMainWorld.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2927
> +        GenerateFunction($function, $interface, "");

Nit: "" is not needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2934
> +                GenerateOverloadedFunction($function, $interface, "");

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2939
> +            GenerateFunctionCallback($function, $interface, "");

Ditto.
Comment 8 Hajime Morrita 2013-03-21 19:17:52 PDT
Comment on attachment 194358 [details]
Patch

Wow, the IDL change looks massive.
Should we start from some minimal set of hottest functions and see the result before going further?
Also, numbers on some existing (non-micro) benchmarks would be nice to have. 
That is especially because this change is intrusive to IDL files, which everyone sees and cares about the readability.
Comment 9 Kentaro Hara 2013-03-21 19:22:55 PDT
(In reply to comment #8)
> (From update of attachment 194358 [details])
> Wow, the IDL change looks massive.
> Should we start from some minimal set of hottest functions and see the result before going further?
> Also, numbers on some existing (non-micro) benchmarks would be nice to have. 
> That is especially because this change is intrusive to IDL files, which everyone sees and cares about the readability.

Sounds reasonable to me. Let's start from performance-sensitive attributes/methods.

(If we want to enable the feature in a per-interface basis, we can add [V8PerWorldBindings] to an interface instead of adding [V8PerWorldBindings] to all attributes/methods of the interface. (Though I don't think that per-interface is a good idea.))
Comment 10 WebKit Review Bot 2013-03-22 01:01:33 PDT
Re-opened since this is blocked by bug 113017
Comment 11 Marja Hölttä 2013-03-22 19:40:05 PDT
This was reverted because it regressed chrome sizes. So I can take your comments into account.

Kentaro: Do we have data on what are the hottest functions in the real world? I wouldn't want to optimize for Dromaeo.

I don't see the cleanliness of the IDL files as a big problem, taking into account the ugliness difference of "before" and "after" states; there are quite a many of custom IDL attributes as is.

However, if you want to keep IDL cleaner I can add the "specialize all setters + getters + callbacks of this class" interface level attribute. I think specializing everything in Node, Element and Document is a reasonable first step.
Comment 12 Marja Hölttä 2013-03-22 19:46:56 PDT
I meant: I can take your comments into account *in the new version I'll try to commit*. :)
Comment 13 Kentaro Hara 2013-03-22 19:48:28 PDT
> Kentaro: Do we have data on what are the hottest functions in the real world? I wouldn't want to optimize for Dromaeo.

I think we can choose hot attributes/methods based on our sixth sense with some data. Let me suggest the list later.

> This was reverted because it regressed chrome sizes. So I can take your comments into account.

How much? Does this imply that it is not allowed to generate two callbacks for all attributes/methods in Node, Element and Document? If that is the case, it implies that we cannot anyway take the approach of per-interface [V8PerWorldBindings].

(Actually I was surprised that [V8PerWorldBindings] has so big impact on the chrome size. Does the size increase make sense to you?)
Comment 14 Kentaro Hara 2013-03-22 19:53:22 PDT
(In reply to comment #13)
> > Kentaro: Do we have data on what are the hottest functions in the real world? I wouldn't want to optimize for Dromaeo.
> 
> I think we can choose hot attributes/methods based on our sixth sense with some data. Let me suggest the list later.

Although I completely agree that we shouldn't stick to Dromaeo, as a first step, how about choosing only attributes/methods used in Dromaeo? It would be a good idea to observe how much performance gain we can see in perf bots and how much binary increase we will hit by duplicating callbacks, to consider what we should do next.
Comment 15 Marja Hölttä 2013-03-22 19:56:01 PDT
It only increased the size for ~ 60 K which was as expected, so %-wise it's not too big of a increase, it just happened to push the Chrome size above the notify-perf-sheriffs threshold and I had forgotten to notify them first :)

So I think we can specialize everything in Node, Document and Element and it shouldn't be too much.
Comment 16 Kentaro Hara 2013-03-22 20:17:57 PDT
(In reply to comment #15)
> It only increased the size for ~ 60 K which was as expected, so %-wise it's not too big of a increase, it just happened to push the Chrome size above the notify-perf-sheriffs threshold and I had forgotten to notify them first :)
> 
> So I think we can specialize everything in Node, Document and Element and it shouldn't be too much.

Thanks for the clarification.

Then how about supporting both per-interface [V8PerWorldBindings] and per-attribute/method [V8PerWorldBindings] ([Conditional] is already doing the same thing) ? We can use per-interface [V8PerWorldBindings] for Node, Document and Element. WDYT?
Comment 17 Marja Hölttä 2013-03-22 20:23:39 PDT
So, my primary opinion is:

Let's keep it as is, the IDL is not considerably uglier than it is now.

However, if you say that won't do, my secondary opinion is:

I'll add a per-interface V8PerWorldBindings (and keep the per-attribute/callback version there too), and specialize everything in Node, Element and Document.
Comment 18 Kentaro Hara 2013-03-22 20:41:54 PDT
(In reply to comment #17)
> So, my primary opinion is:
> 
> Let's keep it as is, the IDL is not considerably uglier than it is now.
> 
> However, if you say that won't do, my secondary opinion is:
> 
> I'll add a per-interface V8PerWorldBindings (and keep the per-attribute/callback version there too), and specialize everything in Node, Element and Document.

hmm, I re-considered about it:

- As morrita mentioned, we don't want to mess up IDL files than is necessary. IDL files are shared with other ports.

- We don't want to increase binary size for duplicating attributes/methods that are rarely used in a real world. 60 KB increase might be large for mobile builds. For example, regarding Document, I don't think createEntityReference, importNode, 
createElementNS, createAttributeNS, getElementsByTagNameNS, createExpression, getOverrideStyle, evaluate etc are not used frequently.

In that sense, per-interface [V8PerWorldBindings] wouldn't be a good idea. My opinion is:

- First, let's add [V8PerWorldBindings] to attributes/methods used in Dromaeo and see performance & memory impacts on build bots including mobile builds.

- After confirming it works as expected, let's add [V8PerWorldBindings] to more attributes/methods with some reasoning (sixth sense). This would be a relatively easy work.

At least I might want to avoid adding [V8PerWorldBindings] to whatever attributes/methods without any reasoning (since it increases the binary size wastefully). WDYT?
Comment 19 Kentaro Hara 2013-03-22 20:43:29 PDT
(In reply to comment #18)
> I don't think createEntityReference, importNode, 
> createElementNS, createAttributeNS, getElementsByTagNameNS, createExpression, getOverrideStyle, evaluate etc are not used frequently.

s/are not used frequently/are used frequently/.
Comment 20 Marja Hölttä 2013-03-25 09:25:25 PDT
Created attachment 194870 [details]
Patch
Comment 21 Kentaro Hara 2013-03-25 09:27:15 PDT
Comment on attachment 194870 [details]
Patch

We discussed offline. LGTM!
Comment 22 WebKit Review Bot 2013-03-25 09:51:57 PDT
Comment on attachment 194870 [details]
Patch

Clearing flags on attachment: 194870

Committed r146785: <http://trac.webkit.org/changeset/146785>
Comment 23 WebKit Review Bot 2013-03-25 09:52:02 PDT
All reviewed patches have been landed.  Closing bug.