Bug 96039 – [V8] Weave creationContext through toV8 and related functions

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

[Adam Barth]

[V8] Weave creationContext through toV8 and related functions

[Adam Barth]

Created attachment 162618 [details]
Patch

[Adam Barth]

Created attachment 162619 [details]
run-bindings-tests diffs

[Adam Barth]

I put the run-bindings-tests diffs in a separate patch file to make it easier to read.

[Kentaro Hara]

Comment on attachment 162618 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:508
> +inline v8::Handle<v8::Value> toV8(${nativeType}* impl, v8::Handle<v8::Context> creationContext = v8::Handle<v8::Context>(), v8::Isolate* isolate = 0${forceNewObjectParameter})

Isn't there any perf concern about creating v8::Handle<v8::Context>() every time? toV8() should be super fast especially for node wrapper objects. If you cannot observe any perf regression in Binding/first-child, then it looks OK.

[Adam Barth]

> Isn't there any perf concern about creating v8::Handle<v8::Context>() every time? toV8() should be super fast especially for node wrapper objects. If you cannot observe any perf regression in Binding/first-child, then it looks OK.

I'll check before landing.  At the moment, I'm just trying to get it working.

[Adam Barth]

Looks like I might have made it marginally faster.  The "before" numbers are before this patch and the "after" numbers are after this patch and the patch in Bug 96044.

== Before ==

Running 1 tests
Running Bindings/first-child.html (1 of 1)
DESCRIPTION: This benchmark covers 'firstChild', 'lastChild', 'nextSibling' and 'previousSibling' in Dromaeo/dom-traverse.html, and other DOM attributes that return a Node object.
RESULT Bindings: first-child= 812.341722521 runs/s
median= 812.182741117 runs/s, stdev= 1.8782032458 runs/s, min= 807.061790668 runs/s, max= 815.286624204 runs/s
RESULT Bindings: first-child: JSHeap= 1330116.8 bytes
median= 1326596.0 bytes, stdev= 12326.5484934 bytes, min= 1319888.0 bytes, max= 1367520.0 bytes
RESULT Bindings: first-child: Malloc= 0.0 bytes
median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes

== After ==

DESCRIPTION: This benchmark covers 'firstChild', 'lastChild', 'nextSibling' and 'previousSibling' in Dromaeo/dom-traverse.html, and other DOM attributes that return a Node object.
RESULT Bindings: first-child= 813.735729057 runs/s
median= 814.249363868 runs/s, stdev= 1.80420562007 runs/s, min= 810.126582278 runs/s, max= 816.326530612 runs/s
RESULT Bindings: first-child: JSHeap= 1330162.4 bytes
median= 1326696.0 bytes, stdev= 12300.6580897 bytes, min= 1319976.0 bytes, max= 1367520.0 bytes
RESULT Bindings: first-child: Malloc= 0.0 bytes
median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes

[Kentaro Hara]

(In reply to comment #6)
> Looks like I might have made it marginally faster.  The "before" numbers are before this patch and the "after" numbers are after this patch and the patch in Bug 96044.

Looks good:)

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/127806 broke bindings tests on the bots.
Could you check it, please?

[Tim Horton]

(In reply to comment #8)
> https://trac.webkit.org/changeset/127806 broke bindings tests on the bots.
> Could you check it, please?

Oddly, this bug is still in NEW. And, it looks like the second patch (rebaselining the bindings test) didn't get landed.

[Adam Barth]

(In reply to comment #8)
> https://trac.webkit.org/changeset/127806 broke bindings tests on the bots.
> Could you check it, please?

Oh, sorry.  I forgot that I put the bindings results in a separate patch.

[Adam Barth]

https://bugs.webkit.org/show_bug.cgi?id=96039

[Adam Barth]

> Isn't there any perf concern about creating v8::Handle<v8::Context>() every time? toV8() should be super fast especially for node wrapper objects. If you cannot observe any perf regression in Binding/first-child, then it looks OK.

Turns out you were right, but the issue was with calling CreationContext().  Binding/first-child didn't see the problem initially because that particular code path wasn't using CreationContext() in this patch.

http://trac.webkit.org/changeset/127946 delays delays calling CreationContext() until we actually need it, which seems to solve the problem.  http://trac.webkit.org/changeset/127955 then causes Binding/first-child to go through this new code path and doesn't show a perf regression.