Bug 112430 – [V8] Generate specialized callbacks for the main world

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

[Marja Hölttä]

The patch in bug 111417 added specialized getters and setters; next I'll add specialized callbacks.

[Marja Hölttä]

Created attachment 194343 [details]
Patch

[Marja Hölttä]

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

[Marja Hölttä]

Created attachment 194358 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194358 [details]
Patch

Clearing flags on attachment: 194358

Committed r146534: <http://trac.webkit.org/changeset/146534>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

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.

[Hajime Morrita]

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.

[Kentaro Hara]

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

[WebKit Review Bot]

Re-opened since this is blocked by bug 113017

[Marja Hölttä]

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.

[Marja Hölttä]

I meant: I can take your comments into account *in the new version I'll try to commit*. :)

[Kentaro Hara]

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

[Kentaro Hara]

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

[Marja Hölttä]

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.

[Kentaro Hara]

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

[Marja Hölttä]

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.

[Kentaro Hara]

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

[Kentaro Hara]

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

[Marja Hölttä]

Created attachment 194870 [details]
Patch

[Kentaro Hara]

Comment on attachment 194870 [details]
Patch

We discussed offline. LGTM!

[WebKit Review Bot]

Comment on attachment 194870 [details]
Patch

Clearing flags on attachment: 194870

Committed r146785: <http://trac.webkit.org/changeset/146785>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.