Bug 80841 - [V8] Refactor V8 wrapping methods
: [V8] Refactor V8 wrapping methods
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kentaro Hara
:
Depends on: 80913 80916
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 08:23 PDT by Kentaro Hara
Modified: 2012-03-12 19:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.27 KB, patch)
2012-03-12 08:32 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
WIP (12.19 KB, patch)
2012-03-12 17:32 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-03-12 08:23:44 PDT
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().
Comment 1 Kentaro Hara 2012-03-12 08:32:16 PDT
Created attachment 131327 [details]
Patch
Comment 2 Kentaro Hara 2012-03-12 08:35:45 PDT
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?
Comment 3 Erik Arvidsson 2012-03-12 10:25:42 PDT
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
Comment 4 Adam Barth 2012-03-12 14:26:13 PDT
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?
Comment 5 Kentaro Hara 2012-03-12 17:32:18 PDT
Created attachment 131470 [details]
WIP
Comment 6 Kentaro Hara 2012-03-12 17:35:20 PDT
(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.
Comment 7 Kentaro Hara 2012-03-12 19:27:05 PDT
All intended refactoring has been completed.