Bug 80841 – [V8] Refactor V8 wrapping methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

V8DOMWrapper.{h,cpp} has getWrapper(), getWrapperSlow(), getExistingWrapper(), getExistingWrapperSlow() and getExistingWrapperInline(), which are confusing to track call-paths. We can refactor them and unify them into getWrapper().

[Kentaro Hara]

Created attachment 131327 [details]
Patch

[Kentaro Hara]

Comment on attachment 131327 [details]
Patch

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

> Source/WebCore/bindings/v8/V8IsolatedContext.h:-89
> -        return reinterpret_cast<V8IsolatedContext*>(getGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex));

Actually I didn't intend to make getEntered() non-inline. I had to made it non-inline because V8IsolatedContext.h cannot include "V8DOMWindow.h" to avoid recursive header including and thus cannot use V8DOMWindow::enteredIsolatedWorldIndex. Is there any solution to keep getEntered() inline?

[Erik Arvidsson]

Comment on attachment 131327 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8IsolatedContext.h:-89
>> -        return reinterpret_cast<V8IsolatedContext*>(getGlobalObject(v8::Context::GetEntered())->GetPointerFromInternalField(V8DOMWindow::enteredIsolatedWorldIndex));
> 
> Actually I didn't intend to make getEntered() non-inline. I had to made it non-inline because V8IsolatedContext.h cannot include "V8DOMWindow.h" to avoid recursive header including and thus cannot use V8DOMWindow::enteredIsolatedWorldIndex. Is there any solution to keep getEntered() inline?

You could split it into 2 parts so that at least the common case is inlined

[Adam Barth]

Comment on attachment 131327 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No tests. No change in behavior.

Have you run the performance tests?

I need to read this carefully, but I'm distracted by gardening.  Other folks should feel free to review if they like.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:438
>  ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl)
>  {
>  END
> -    my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getExistingWrapper(impl)" : "${domMapFunction}.get(impl)";
> +    my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getWrapper(impl)" : "${domMapFunction}.get(impl)";

Doesn't this change semantics?

[Kentaro Hara]

Created attachment 131470 [details]
WIP

[Kentaro Hara]

(In reply to comment #4)
> > Source/WebCore/ChangeLog:13
> > +        No tests. No change in behavior.
> 
> Have you run the performance tests?

I confirmed no performance regression on DOM core tests in DROMAEO.

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:438
> >  ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl)
> >  {
> >  END
> > -    my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getExistingWrapper(impl)" : "${domMapFunction}.get(impl)";
> > +    my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getWrapper(impl)" : "${domMapFunction}.get(impl)";
> 
> Doesn't this change semantics?

No. This patch just refactors code so that it does not change any call path.

> I need to read this carefully, but I'm distracted by gardening.  Other folks should feel free to review if they like.

Maybe this patch is doing too many things at a breath, and it is not clear if the patch does not change the semantics. Let me land this patch step by step.

[Kentaro Hara]

All intended refactoring has been completed.