Bug 131196

Summary: Fast-path for casting JS wrappers to JSNode.
Product: WebKit Reporter: Andreas Kling <kling>
Component: BindingsAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, kling, simon.fraser
Priority: P2 Keywords: Performance
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch mhahnenberg: review+

Description Andreas Kling 2014-04-03 17:40:16 PDT
We should have a fast-path for casting JS wrappers to JSNode.
Comment 1 Andreas Kling 2014-04-03 18:09:31 PDT
Created attachment 228567 [details]
Patch
Comment 2 Mark Hahnenberg 2014-04-03 18:18:55 PDT
Comment on attachment 228567 [details]
Patch

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

r=me with comments

> Source/WebCore/bindings/js/JSDOMWrapper.h:33
> +    JSNodeExtendedType = 0,

I'd say something like:

JSNodeType = ExtendedObjectType,

That way you don't have to remember to do the addition later, and you don't have to keep typing "Extended" everywhere :-)
Comment 3 Geoffrey Garen 2014-04-03 18:23:54 PDT
Comment on attachment 228567 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSObject.h:1125
> +    return type() >= VariableObjectType && type() < ExtendedObjectType;

I think this would be clearer as "== GlobalObjectType || == ActivationObjectType" (removing VariableObjectType altogether).

> Source/JavaScriptCore/runtime/JSType.h:73
> +    // We use (>=VariableObjectType && <ExtendedObjectType) checks to test for Global & Activation objects, but exclude NameScopes.

Space. And see comment above.

> Source/JavaScriptCore/runtime/JSType.h:79
> +    ExtendedObjectType,

I would call this "LastJSCType = ActivationObjectType", or something like that.

> Source/WebCore/bindings/js/JSNodeCustom.h:85
> +    return value.asCell()->type() == (JSC::ExtendedObjectType + JSNodeExtendedType) ? JSC::jsCast<JSNode*>(value) : nullptr;

It kind of stinks that the client of the constant needs to know that the constant isn't the right value to test, and you have to add something else in. Can we just make a JSNodeType constant that includes JSC::ExtendedObjectType?
Comment 4 Andreas Kling 2014-04-03 19:30:12 PDT
Committed r166760: <http://trac.webkit.org/changeset/166760>
Comment 5 Simon Fraser (smfr) 2014-04-03 22:08:45 PDT
Broke bindings tests.