Bug 111417 – [V8] Add machinery for generating specialized bindings for the main world

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

[Marja Hölttä]

- Use the "context type" parameter added in bug 110875 to generate specialized "main world" versions of selected getters / setters / methods.
- The specialized versions will do a faster conversion instead of toV8Fast, because we don't need to "are we in the main world or in an isolated world or a worker" check.
- Add placeholders for deciding which specializations to create in CodeGeneratorV8.pm; leave it empty for now.

[Marja Hölttä]

Created attachment 191467 [details]
Patch

[Adam Barth]

Comment on attachment 191467 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:73
> +my %specializedMainWorldBindings;
> +# At the moment the hash map is empty. To fill it, add lines like this:
> +# @{$specializedMainWorldBindings{"Class"}} = ({"attributeName" => 1}, {"methodName" => 1});

Why not have this be an IDL attribute?  [V8SpecializeForMainWorld] ?

[Kentaro Hara]

Comment on attachment 191467 [details]
Patch

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

I'd recommend you split the patch into pieces. As a first step, how about just changing getters? When you're confident it works, you can enable it on setters and callbacks as well.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:73
>> +# @{$specializedMainWorldBindings{"Class"}} = ({"attributeName" => 1}, {"methodName" => 1});
> 
> Why not have this be an IDL attribute?  [V8SpecializeForMainWorld] ?

In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right?

If so, please add a comment like "FIXME: This is just a temporary hack to implement two ObjectTemplates for all interfaces. Once the work is completed, this should be removed."

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:596
> +template<class HolderContainer, class Wrappable>
> +inline v8::Handle<v8::Value> toV8InMainWorld(${nativeType}* impl, const HolderContainer& container, Wrappable* wrappable)

toV8InMainWorld => toV8FastForMainWorld

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:600
> +    v8::Handle<v8::Object> wrapper = DOMDataStore::getWrapperInMainWorld(impl);

Nit: getWrapperForMainWorld

(Nit: Sorry, last time I recommended worldTypeInMainWorld, but it looks like xxxForMainWorld is a naming convention in V8 binding.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:909
> +    my $isMainWorldSpecialization = shift;

This could be $forMainWorldSuffix, so that you can pass "ForMainWorld" or "" (or omit the argument). Then call sites will become more readable.

The same comment for other parts of the patch.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:918
> +    my $functionSuffix = "";
> +    if ($isMainWorldSpecialization) {
> +        $functionSuffix = "InMainWorld";
> +    }

Then you can remove this.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:951
> +    my $toV8FastFunc = "toV8Fast";
> +    my $functionSuffix = "";
> +    if ($isMainWorldSpecialization) {
> +        $toV8FastFunc = "toV8InMainWorld";
> +        $functionSuffix = "InMainWorld";
> +    }

Once you rename toV8InMainWorld to toV8FastForMainWorld, you can remove $toV8FastFunc.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2832
> +        GenerateNormalAttrGetter($attribute, $interface, 0);

This can be GenerateNormalAttrGetter($attribute, $interface).

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2838
> +                GenerateNormalAttrGetter($attribute, $interface, 1);

This can be GenerateNormalAttrGetter($attribute, $interface, "ForMainWorld").

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3065
> +            # FIXME: how to deal with overloaded functions?
> +            # Don't put any nonstandard functions into this table:

This is non-trivial. Let's care about callbacks in a follow-up patch. You can enable the feature on only getters, as a starting point.

[Adam Barth]

> > Why not have this be an IDL attribute?  [V8SpecializeForMainWorld] ?
> 
> In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right?
> 
> If so, please add a comment like "FIXME: This is just a temporary hack to implement two ObjectTemplates for all interfaces. Once the work is completed, this should be removed."

Why not use an IDL attribute, even temporarily?

[Kentaro Hara]

(In reply to comment #4)
> > > Why not have this be an IDL attribute?  [V8SpecializeForMainWorld] ?
> > 
> > In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right?
> 
> Why not use an IDL attribute, even temporarily?

Sounds reasonable.

[Marja Hölttä]

I'm splitting this up, the first part is bug 111724.

[Marja Hölttä]

Created attachment 193131 [details]
Patch

[Marja Hölttä]

The new patch generates bindings for getters and setters (not for callbacks).

(In reply to comment #8)
> The new patch generates bindings for getters and setters (not for callbacks).

but since you didn't modify any idl, it's not actually doing that yet, right?

[Marja Hölttä]

It does. The code generator .pm just dummily generates custom versions for *everything* (except per-context attrs, didn't figure out how to do that).

(In reply to comment #10)
> It does. The code generator .pm just dummily generates custom versions for *everything* (except per-context attrs, didn't figure out how to do that).

In that case, we should see some speed up on the perf tests, right?

Can you run the tests and post results?

[Kentaro Hara]

Comment on attachment 193131 [details]
Patch

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

Looks great.

> Source/WebCore/ChangeLog:10
> +        The new specialized bindings will be faster, because they don't need to
> +        do the "main world, isolated world or a worker" check, but can right
> +        away assume that we're in the main world.

As jochen pointed out, you can measure the performance impact by using PerformanceTests/Bindings/first-child.html, which would be the most sensitive micro-benchmark to your change. Please paste the results here.

(Actually I won't be much surprised if you cannot observe any performance improvement. When I tried the optimization last time, I couldn't observe any perf win.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2799
> +        if (!$attrExt->{"V8EnabledPerContext"}) {

Why do you want to exclude attributes with [V8EnabledPerContext]? (Either way you can fix it in a separate patch.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2809
> +            if (!$attrExt->{"V8EnabledPerContext"}) {

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3062
> -    if (!${enable_function}())
> +   if (!${enable_function}())

Nit: Drop this change.

[Marja Hölttä]

Created attachment 193274 [details]
Patch

[Marja Hölttä]

Some changes I made to the previous version:
- Also generate toV8ForMainworld, not only toV8FastForMainWorld.
- Added ASSERTS in toV8(Fast)*ForMainWorld, to assert that we indeed are in the main world.

I ran the Bindings/* performance tests twice, and check which tests show a similar kind of improvement on both runs. Results:

Better:
/Bindings/document-implementation:Runs: ~ 12% better
/Bindings/gc-tree:Time ~ 5% better
/Bindings/insert-before:Runs ~ 3% better
/Bindings/node-list-access:Runs ~ 3% better
/Bindings/undefined-first-child:Runs ~ 3% better

No measurable change:
/Bindings/append-child:Runs
/Bindings/create-element:Runs
/Bindings/event-target-wrapper:Runs
/Bindings/first-child:Runs
/Bindings/gc-forest:Time
/Bindings/gc-mini-tree:Time
/Bindings/get-attribute:Runs
/Bindings/get-element-by-id:Runs
/Bindings/id-getter:Runs
/Bindings/id-setter:Runs
/Bindings/scroll-top:Runs
/Bindings/set-attribute:Runs
/Bindings/typed-array-construct-from-array:Runs
/Bindings/typed-array-construct-from-same-type:Runs
/Bindings/typed-array-construct-from-typed:Runs
/Bindings/undefined-get-element-by-id:Runs
/Bindings/undefined-id-getter:Runs

Worse:
/Bindings/get-elements-by-tag-name:Runs ~ 3% worse << flake?

[Kentaro Hara]

(In reply to comment #14)
> Better:
> /Bindings/document-implementation:Runs: ~ 12% better
> /Bindings/gc-tree:Time ~ 5% better
> /Bindings/insert-before:Runs ~ 3% better
> /Bindings/node-list-access:Runs ~ 3% better
> /Bindings/undefined-first-child:Runs ~ 3% better

Looks great, but I'm a bit suspicious of the accuracy. For example, undefined-first-child just returns v8::Undefined() when imp->firstChild() is 0. So I think that the performance won't be affected by your change.

[Kentaro Hara]

Either way there is no reason that the patch regresses performance, I'm very happy to land it. We're just interested in performance improvement of the patch:)

[Marja Hölttä]

Yep, the ~3 % changes are probably just noise. The ~ 12 % I can somewhat believe.

[Kentaro Hara]

(In reply to comment #17)
> Yep, the ~3 % changes are probably just noise. The ~ 12 % I can somewhat believe.

Hmm, still I wonder why you got 12% speed-up in document-implementation.html whereas you didn't get speed-up in first-child.html. I think they are essentially the same in that they return a ScriptWrapperable to V8. (You don't need to investigate the issue though.)

[WebKit Review Bot]

Comment on attachment 193274 [details]
Patch

Clearing flags on attachment: 193274

Committed r145896: <http://trac.webkit.org/changeset/145896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112443

[Marja Hölttä]

Created attachment 193690 [details]
Patch

[WebKit Review Bot]

Attachment 193690 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h', u'Source/WebCore/bindings/v8/DOMDataStore.h', u'Source/WebCore/bindings/v8/V8DOMConfiguration.cpp', u'Source/WebCore/bindings/v8/V8DOMConfiguration.h', u'Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8EventTargetCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8MicroDataItemValueCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp']" exit_code: 1
Source/WebCore/bindings/v8/V8DOMConfiguration.h:83:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 193690 [details]
Patch

rs=me

[Marja Hölttä]

I reverted this because it made the data segment of the executable (in this case, chrome, though the same thing is visible in DumpRenderTree) grow by ~ 140 K.

In the new version, I don't create a new attribute array for storing the "forMainWorld" function pointers, but modify the existing attribute array to contain it. So now the data segment size increase only by ~ 40 K.

[Marja Hölttä]

Created attachment 193694 [details]
Patch

[Kentaro Hara]

(In reply to comment #25)
> I reverted this because it made the data segment of the executable (in this case, chrome, though the same thing is visible in DumpRenderTree) grow by ~ 140 K.
> 
> In the new version, I don't create a new attribute array for storing the "forMainWorld" function pointers, but modify the existing attribute array to contain it. So now the data segment size increase only by ~ 40 K.

Good idea! Can the 40 K increase roughly be explained by the attrGetter/attrSetter duplication? (I'd just like to confirm that we're not doing something ridiculous.)

[Kentaro Hara]

Comment on attachment 193694 [details]
Patch

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

> Source/WebCore/bindings/v8/DOMDataStore.h:90
> +        if (mainWorldWrapperIsStoredInObject(object) && isMainWorldObject(object)) {
> +            if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist()))

Why is this check needed? If I'm not missing something, the point of this patch is removing this check.

(With this check, I don't understand why you observed performance improvement...)

> Source/WebCore/bindings/v8/V8DOMConfiguration.h:61
>      template<class ObjectOrTemplate>
>      static inline void configureAttribute(v8::Handle<ObjectOrTemplate> instance, v8::Handle<ObjectOrTemplate> prototype, const BatchedAttribute& attribute, v8::Isolate*)

Nit: Shall we add a FIXME that says this method should be deprecated?

[Marja Hölttä]

Comment on attachment 193694 [details]
Patch

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

>> Source/WebCore/bindings/v8/DOMDataStore.h:90
>> +            if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist()))
> 
> Why is this check needed? If I'm not missing something, the point of this patch is removing this check.
> 
> (With this check, I don't understand why you observed performance improvement...)

Ah yes. Thanks for pointing that out.

Also, below, we call mainWorldStore() instead of resolving the current world (like in the version above).

Probably this was still a perf boost because in the perf tests, isolated worlds didn't exist.

[Marja Hölttä]

Kentaro:

The data segment size increase was because the previous version added this extra array for the ForMainWorld function pointers. The new version doesn't.

But, I don't think we want this version either; this is increasing the total executable size by 1 M. :/

So we'd probably want to select which functions we specialize with an IDL attribute.

[Marja Hölttä]

Oh, and to answer your previous question:

The per-context functions are left out of this patch, because plumbing the world type to be available in the installPerContextProperties functions is non-trivial. (I would need to add it to "wrap" and lots of other binding funcs.)

[Kentaro Hara]

(In reply to comment #30)
> Kentaro:
> 
> The data segment size increase was because the previous version added this extra array for the ForMainWorld function pointers. The new version doesn't.
> 
> But, I don't think we want this version either; this is increasing the total executable size by 1 M. :/
> 
> So we'd probably want to select which functions we specialize with an IDL attribute.

Hmm, probably.

Assuming that each getter/setter requires 200 B and there are 3000 getters/setters (I didn't count it, I just estimated by the fact that there are 600 IDL files), we cannot avoid consuming 200 B * 3000 = 600 KB, approximately 1 MB, which wouldn't be acceptable.

In the selective duplication approach, are you going to add two templates for all interfaces but generate two getters/setters/methods only for getters/setters/methods that have [PerWorldAccess] ? Or do you have another idea in mind ?

[Kentaro Hara]

(In reply to comment #31)
> Oh, and to answer your previous question:
> 
> The per-context functions are left out of this patch, because plumbing the world type to be available in the installPerContextProperties functions is non-trivial. (I would need to add it to "wrap" and lots of other binding funcs.)

Makes sense to me. Thanks.

If you take an approach where some interfaces have two templates and others have one template (which is technically possible), I'm afraid that the code base will become very messy and buggy.

My question is what would be a good balancing point between clean code base and an increase in the executable file size.

[Marja Hölttä]

After the patch in bug 111724 landed, we already have 2 different templates for main and non-main worlds. So I'd just selectively customize some of them and leave the others as they are now.

[Kentaro Hara]

(In reply to comment #34)
> After the patch in bug 111724 landed, we already have 2 different templates for main and non-main worlds. So I'd just selectively customize some of them and leave the others as they are now.

OK, we (already) have two templates for all interfaces. So in this patch, we can just try to generate two getters/settters for DOM attributes that have [PerWorldDOMAccess] ?

[Marja Hölttä]

Exactly.

[WebKit Review Bot]

Comment on attachment 193694 [details]
Patch

Attachment 193694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17184376

New failing tests:
storage/websql/sql-data-types.html

[Kentaro Hara]

(In reply to comment #36)
> Exactly.

Sounds good to me. I hope it will be an easy work for you (I guess you will just need to add a check about [V8PerWorldDOMAccess]).

[Marja Hölttä]

Created attachment 193857 [details]
Patch

[Marja Hölttä]

The latest patch adds a IDL attribute (V8PerWorldBindings) and specializes some getters / setters based on that. For starters, I specialized all the attributes of Node and Element.

Comment on attachment 193857 [details]
Patch

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

ok

can you post perf results with that patch?

> Source/WebCore/ChangeLog:12
> +        This patch generates main world bindings for getters and setters of Node

can you mention the name of the IDL attribute you introduced?

[Adam Barth]

This patch is really cool.  Thanks for working on this!

[Marja Hölttä]

Some perf tests results. Same setup than last time (run twice, locally, only include tests which show consistent results). Some of them are noise nevertheless.

Better:
create-element 15%
gc-forest 10%
gc-mini-tree 10%
set-attribute 10%
typed-array-construct-from-same-type 5%

No change:
append-child
document-implementation
event-target-wrapper
first-child
gc-tree
get-attribute
get-element-by-id
get-elements-bytag-name
id-getter
id-setter
insert-before
scroll-top
typed-array-construct-from-array
typed-array-construct-from-typed
typed-array-set-from-typed
undefined-first-child
undefined-get-element-by-id
undefine-id-getter

Worse:
-

[Adam Barth]

If you run the Dromeo/dom-* tests, I would expect you'd see a win for dom-traverse.html (and perhaps others).

[Marja Hölttä]

Created attachment 193889 [details]
Patch

[WebKit Review Bot]

Comment on attachment 193889 [details]
Patch

Rejecting attachment 193889 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 193889, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
e Source/WebCore/bindings/v8/custom/V8EventTargetCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8MicroDataItemValueCustom.cpp
patching file Source/WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp
patching file Source/WebCore/dom/Element.idl
patching file Source/WebCore/dom/Node.idl

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/17247092

[Marja Hölttä]

Created attachment 193911 [details]
Patch

[Marja Hölttä]

(Forgot to add that the expected executable size increase because of this change is around 100 K.)

[WebKit Review Bot]

Comment on attachment 193911 [details]
Patch

Clearing flags on attachment: 193911

Committed r146259: <http://trac.webkit.org/changeset/146259>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Comment on attachment 193911 [details]
Patch

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

Cool!

> Source/WebCore/bindings/v8/DOMDataStore.h:89
> +        if (mainWorldWrapperIsStoredInObject(object) && isMainWorldObject(object))

This branch is resolved statically and removed by a compiler, right?

[Dan Carney]

> This branch is resolved statically and removed by a compiler, right?


yes, but it's wrong. the isMainWorldObject(object) part should be removed, since it limits this optimization to nodes.

Also, isMainWorldObject is incorrectly named. It should be isMainThreadObject, since that's what it's trying to check