Bug 131196 - Fast-path for casting JS wrappers to JSNode.
Summary: Fast-path for casting JS wrappers to JSNode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2014-04-03 17:40 PDT by Andreas Kling
Modified: 2014-04-03 22:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.31 KB, patch)
2014-04-03 18:09 PDT, Andreas Kling
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.