Bug 27372 - [v8] switch to faster methods to obtain and set implementation
: [v8] switch to faster methods to obtain and set implementation
Status: RESOLVED FIXED
: WebKit
WebKit Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-07-17 08:16 PST by
Modified: 2009-07-20 19:19 PST (History)


Attachments
Initial version (23.58 KB, patch)
2009-07-17 08:20 PST, anton muhin
abarth: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-17 08:16:47 PST
Recently new methods to access internal fields were introduced which should be faster.  Switch to them plus refactor a bit the code to pass v8::Handle<v8::Object> (instead of <v8::Value>) into the methods fetching implementation from the wrapper.

Please, note, this change should not be landed before http://codereview.chromium.org/159003 is submitted.
------- Comment #1 From 2009-07-17 08:20:03 PST -------
Created an attachment (id=32942) [details]
Initial version
------- Comment #2 From 2009-07-17 14:26:37 PST -------
This looks fine to me.

I have landed the change in chromium that this change depends on, so it is safe to land this change once it gets the r+.
------- Comment #3 From 2009-07-17 22:55:51 PST -------
(From update of attachment 32942 [details])
Thanks for the patch.  This looks great.  One question, how do we know this is safe:

+        node = V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[0]));

or

+        contextNode = V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[1]));

Why can't the page pass a value as the argument?
------- Comment #4 From 2009-07-20 04:29:08 PST -------
(In reply to comment #2)
> This looks fine to me.
> 
> I have landed the change in chromium that this change depends on, so it is safe
> to land this change once it gets the r+.

Thanks a lot, Mads.
------- Comment #5 From 2009-07-20 04:30:25 PST -------
(In reply to comment #3)
> (From update of attachment 32942 [details] [details])
> Thanks for the patch.  This looks great.  One question, how do we know this is
> safe:
> 
> +        node =
> V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[0]));
> 
> or
> 
> +        contextNode =
> V8DOMWrapper::convertDOMWrapperToNode<Node>(v8::Handle<v8::Object>::Cast(args[1]));
> 
> Why can't the page pass a value as the argument?

Adam, could you double check, but we did this cast unconditionally in DOMWrapper::convertToNativeObject anyway.  I just moved cast into callers, not callee not to penalize callers which already have an object
------- Comment #6 From 2009-07-20 12:40:45 PST -------
(From update of attachment 32942 [details])
Awesome.  Thanks for explain this patch to me.  Yes, I see now that the callers all have branches guarding the caste (and that we've retained the assert that's keeping us honest).
------- Comment #7 From 2009-07-20 18:50:13 PST -------
Will land.
------- Comment #8 From 2009-07-20 19:19:49 PST -------
Committing to http://svn.webkit.org/repository/webkit/trunk ...
    M    WebCore/ChangeLog
    M    WebCore/bindings/scripts/CodeGeneratorV8.pm
    M    WebCore/bindings/v8/V8DOMWrapper.cpp
    M    WebCore/bindings/v8/V8DOMWrapper.h
    M    WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
    M    WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
    M    WebCore/bindings/v8/custom/V8DocumentCustom.cpp
    M    WebCore/bindings/v8/custom/V8ElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
    M    WebCore/bindings/v8/custom/V8HTMLSelectElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
    M    WebCore/bindings/v8/custom/V8NodeCustom.cpp
    M    WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
Committed r46145
    M    WebCore/ChangeLog
    M    WebCore/bindings/scripts/CodeGeneratorV8.pm
    M    WebCore/bindings/v8/V8DOMWrapper.cpp
    M    WebCore/bindings/v8/V8DOMWrapper.h
    M    WebCore/bindings/v8/custom/V8HTMLCanvasElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8DocumentCustom.cpp
    M    WebCore/bindings/v8/custom/V8NodeCustom.cpp
    M    WebCore/bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp
    M    WebCore/bindings/v8/custom/V8InspectorControllerCustom.cpp
    M    WebCore/bindings/v8/custom/V8ElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8HTMLOptionsCollectionCustom.cpp
    M    WebCore/bindings/v8/custom/V8ClipboardCustom.cpp
    M    WebCore/bindings/v8/custom/V8HTMLSelectElementCustom.cpp
    M    WebCore/bindings/v8/custom/V8XSLTProcessorCustom.cpp
r46145 = b8d7bc3aef4a0028123fa2cd9ca298b39e32e9dd (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46145