Bug 102686 – [V8] toFastV8 for non-Nodes

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

[Dan Carney]

[V8] toFastV8 for non-Nodes

[Dan Carney]

Created attachment 174973 [details]
Patch

[Dan Carney]

Comment on attachment 174973 [details]
Patch

patch is hitting an assertion

can you post a backtrace to the assertion?

[WebKit Review Bot]

Comment on attachment 174973 [details]
Patch

Attachment 174973 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14928005

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Kentaro Hara]

Comment on attachment 174973 [details]
Patch

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

The patch looks great. Marking r- due to the code duplication for AccessorInfo and Arguments.

> Source/WebCore/ChangeLog:3
> +        [V8] toFastV8 for non-Nodes

I'm a bit confused with the title. Is this patch intending to use toV8Fast() for a case where IsDOMNodeType($type) is true?

Nit: toV8Fast

> Source/WebCore/ChangeLog:8
> +        This patch makes most generated bindings use toFastV8

Ditto.

> Source/WebCore/ChangeLog:10
> +        Additionally, a toFastV8 for v8::Arguments was added

Ditto.

> Source/WebCore/ChangeLog:11
> +        to make this most of thia optimization.

Typo: thia. Sorry for nit-pickings.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:536
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Arguments& info, Node*)

Nit: We're likely to use 'args' instead of 'info'.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:539
> +
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Arguments& info, Node*)
> +{
> +    return toV8(impl, info.Holder(), info.GetIsolate());
> +}

We might not want to duplicate this function for AccessorInfo and Arguments. Wouldn't it be possible to change the signature to

  v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, v8::Handle<v8::Object> holder, Node*);

and thus eliminate the duplication?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:588
> +        foreach my $argType ("AccessorInfo","Arguments") {

Nit: "AccessorInfo", "Arguments" (One space after ,)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:620
> +inline v8::Handle<v8::Value> toV8Fast(PassRefPtr< ${nativeType} > impl, const v8::Arguments& info, Node* holder)

Nit: We are likely to use 'args' instead of 'info'.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4144
> +    if ($getFastArgs) {

Don't you need to check IsDOMNodeType($type) ?

[Dan Carney]

Created attachment 177259 [details]
Patch

[Dan Carney]

Comment on attachment 174973 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        [V8] toFastV8 for non-Nodes
> 
> I'm a bit confused with the title. Is this patch intending to use toV8Fast() for a case where IsDOMNodeType($type) is true?
> 
> Nit: toV8Fast

fixed
it means that the callback is from for a node subclass, but the object requested could be anything with a wrapper

>> Source/WebCore/ChangeLog:8
>> +        This patch makes most generated bindings use toFastV8
> 
> Ditto.

ok

>> Source/WebCore/ChangeLog:10
>> +        Additionally, a toFastV8 for v8::Arguments was added
> 
> Ditto.

ok

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:539
>> +}
> 
> We might not want to duplicate this function for AccessorInfo and Arguments. Wouldn't it be possible to change the signature to
> 
>   v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, v8::Handle<v8::Object> holder, Node*);
> 
> and thus eliminate the duplication?

ok

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:588
>> +        foreach my $argType ("AccessorInfo","Arguments") {
> 
> Nit: "AccessorInfo", "Arguments" (One space after ,)

ok

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:620
>> +inline v8::Handle<v8::Value> toV8Fast(PassRefPtr< ${nativeType} > impl, const v8::Arguments& info, Node* holder)
> 
> Nit: We are likely to use 'args' instead of 'info'.

ok

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4144
>> +    if ($getFastArgs) {
> 
> Don't you need to check IsDOMNodeType($type) ?

not here, the presence of the argument implies there is a node to make the call from

[Adam Barth]

Comment on attachment 177259 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:589
> -inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::AccessorInfo& info, Node* holder)
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Local<v8::Object> holder, v8::Isolate* isolate, Node* node)

This seems to imply that all callers need to call GetIsolate before calling toV8Fast.  In the common path through this function, we previously avoided the GetIsolate call.  Does calling that function eagerly affect performance (I would expect it to).

It might be worth having two overloaded copies of this function: one for AccessorInfo and one for Arguments.  We could even make it a template since the code would be identical.  This function is always inlined, so there wouldn't be any cost to having two copies.

[Adam Barth]

Comment on attachment 177259 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:585
> +        my $fastWrapper = $codeGenerator->IsSubType($interface, "Node") ? "impl->wrapper()" : "DOMDataStore::mainWorldStore()->get(impl)";

Very clever!

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1076
> +        if (IsDOMNodeType($interfaceName)) {

In principle, we should be able to use toV8Fast whenever $interfaceName is ScriptWrappable.  We can have the compiler do the dispatch by exposing DOMDataStore::wrapperIsStoredInObject publicly.  We can do that in a subsequent patch if you like.

[Adam Barth]

Can you run dom-traverse to make sure we're not regressing?  I suspect calling GetIsolate one extra time will show up on that perf test.  (It should be fixable as described above.)

[Dan Carney]

Comment on attachment 177259 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:589
>> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Local<v8::Object> holder, v8::Isolate* isolate, Node* node)
> 
> This seems to imply that all callers need to call GetIsolate before calling toV8Fast.  In the common path through this function, we previously avoided the GetIsolate call.  Does calling that function eagerly affect performance (I would expect it to).
> 
> It might be worth having two overloaded copies of this function: one for AccessorInfo and one for Arguments.  We could even make it a template since the code would be identical.  This function is always inlined, so there wouldn't be any cost to having two copies.

I had the two copies before but haraken didn't want it.  I thought the Isolate was needed in all paths, but I see now that it's not in the fast path.  I'll switch to a templatized method.

[Dan Carney]

Created attachment 177476 [details]
Patch

[Dan Carney]

(In reply to comment #12)
> Created an attachment (id=177476) [details]
> Patch

This time round I moved all the complexity to DOMDataStore inline templates. Seems much cleaner.

[Dan Carney]

Note, this patch doesn't have the adjustments to the binding test classes. It's harder to read the patch, so I'll add that after an r+.

[Dan Carney]

(In reply to comment #10)
> Can you run dom-traverse to make sure we're not regressing?  I suspect calling GetIsolate one extra time will show up on that perf test.  (It should be fixable as described above.)

Your comment no longer applies, but here are the numbers, run twice each with interwoven runs. Looks like a small perf bump.

with patch:

avg 3069.1348651348653 runs/s
median 0 runs/s
stdev 21.196854488648402 runs/s
min 3009.99000999001 runs/s
max 3088.81018981019 runs/s

avg 3192.8747252747253 runs/s
median 0 runs/s
stdev 12.296375015512377 runs/s
min 3163.9750249750246 runs/s
max 3213.18981018981 runs/s

without patch:

avg 2963.217982017982 runs/s
median 0 runs/s
stdev 16.350015231822884 runs/s
min 2920.834165834166 runs/s
max 2987.306693306693 runs/s

avg 2925.672727272727 runs/s
median 0 runs/s
stdev 6.087018068830624 runs/s
min 2908.5304695304694 runs/s
max 2936.0009990009994 runs/s

[Adam Barth]

Comment on attachment 177476 [details]
Patch

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

Looks great.

> Source/WebCore/bindings/v8/DOMDataStore.h:65
> +    static v8::Handle<v8::Object> getWrapperFast(T* object, const HolderContainer& container, Wrappable* wrappable)

wrappable -> holder

[Dan Carney]

Created attachment 177692 [details]
Patch

Comment on attachment 177692 [details]
Patch

Clearing flags on attachment: 177692

Committed r136652: <http://trac.webkit.org/changeset/136652>

All reviewed patches have been landed.  Closing bug.