Bug 35009 – [V8] Add context type variable to global object and merge object wrapping paths

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

Currently, we have no way of knowing whether the global object is a DOMWindow* or a WorkerContext* without doing the rather performance-unfriendly lookupDOMWrapper() and checking for the desired instance in the prototype chain.  If we add a context type variable (essentially just the relevant V8ClassIndex::V8WrapperType) to the global object, we can quickly check that and know what kind of global object we're dealing with.  Because we know what type it is, we can just unwrap it, rather than having to iterate through the prototype chain.

This will simplify the logic of object wrapping in WorkerContexts versus standard DOMWindows, and may even result in a slight performance gain.

[Nate Chapin]

Created attachment 48848 [details]
patch

In tests on my local machine, it's hard to tell whether this actually changes performance on the dromaeo and dom benchmark tests.  If it is a change, however, it certainly looks like an improvement.

[Adam Barth]

Comment on attachment 48848 [details]
patch

Ok.  I'm not super happy about:

+ V8DOMWindow::toNative(v8::Handle<v8::Object>::Cast(global->GetPrototype()))->frame()

That seems like too low-level a manipulation for that program point.  Don't we have a helper function somewhere that does that kind of thing?

[anton muhin]

Hidden properties are /very/ slow and may affect internal fields.  in your case, as you only access that on global object, I think you'd better off with internal fields and, if it's performance critical (and I think it is), use GetPointerFromInternalField/SetPointerToInternalField.  Use some sentinel pointer (aligned!) as a marker.  The catch might be to find an index, but that should be doable.

Note: that using type might be not the best idea for now, but if you prefer that, let me know and we could extend V8 pubic API to make type access faster (it's currently unnecessary slow, but too rarely used to care about, at least I think so :)

And mandatory question, have you estimated performance effect of your change?

[Nate Chapin]

(In reply to comment #3)
> Hidden properties are /very/ slow and may affect internal fields.  in your
> case, as you only access that on global object, I think you'd better off with
> internal fields and, if it's performance critical (and I think it is), use
> GetPointerFromInternalField/SetPointerToInternalField.  Use some sentinel
> pointer (aligned!) as a marker.  The catch might be to find an index, but that
> should be doable.
> 
> Note: that using type might be not the best idea for now, but if you prefer
> that, let me know and we could extend V8 pubic API to make type access faster
> (it's currently unnecessary slow, but too rarely used to care about, at least I
> think so :)
> 
> And mandatory question, have you estimated performance effect of your change?

The estimate from my local tests was that it would be either neutral or a slight performance improvement.  I didn't realize how slow hidden properties are though, so let me experiment a bit and see if I can speed things up some more.

[Nate Chapin]

Created attachment 48938 [details]
faster patch

I'm having trouble determining whether this patch is a meaningful change in dromaeo test performance, but it's a big (5% or so) improvement on the dom benchmark test.

It's a bit hackish though. The idea is to tell what type of context were in by the internal field count on the wrapper context object. This works as long as DOMWindow objects and WorkerContext objects have different field counts, which given the huge disparity will likely remain true indefinitely.  I included an ASSERT and a comment to this effect as a precaution though.

I recognize this may be too much of a hack, but I figured I'd throw it out and see what people think.

[Eric Seidel (no email)]

Comment on attachment 48848 [details]
patch

Cleared Adam Barth's review+ from obsolete attachment 48848 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/bindings/v8/V8DOMWrapper.cpp:271:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1


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

[Adam Barth]

Comment on attachment 48938 [details]
faster patch

Clever.  Please fix the style nit.

[anton muhin]

(In reply to comment #5)
> Created an attachment (id=48938) [details]
> faster patch
> 
> I'm having trouble determining whether this patch is a meaningful change in
> dromaeo test performance, but it's a big (5% or so) improvement on the dom
> benchmark test.
> 
> It's a bit hackish though. The idea is to tell what type of context were in by
> the internal field count on the wrapper context object. This works as long as
> DOMWindow objects and WorkerContext objects have different field counts, which
> given the huge disparity will likely remain true indefinitely.  I included an
> ASSERT and a comment to this effect as a precaution though.
> 
> I recognize this may be too much of a hack, but I figured I'd throw it out and
> see what people think.

I like it, and 5% is a very good achievement.

One catch might be interaction with debug agent---they used to inject some stuff into the global object.  Yury (cc'ed, [e-mail]) is an expert, Yury, any comments?  Plus, run your change through interactive ui tests---they have some coverage for debugger.

[Yury Semikhatsky]

Comment on attachment 48938 [details]
faster patch

> +    ASSERT(V8DOMWindow::internalFieldCount != V8WorkerContext::internalFieldCount && V8DOMWindow::internalFieldCount != V8SharedWorkerContext::internalFieldCount);
This should be checked at compile time.

[Yury Semikhatsky]

(In reply to comment #9)
> One catch might be interaction with debug agent---they used to inject some
> stuff into the global object.  Yury (cc'ed, [e-mail]) is an expert,
> Yury, any comments?  Plus, run your change through interactive ui tests---they
> have some coverage for debugger.
This change shouldn't affect the script injection. We just store a reference to some inspector object in a hidden property so it doesn't seem to conflict in any way with this change. But thanks for attract my attention to this change, Anton.

[Nate Chapin]

Looks like I forgot to close this: http://trac.webkit.org/changeset/54972