Bug 21294 - Devirtualize getOwnPropertySlot()
Summary: Devirtualize getOwnPropertySlot()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
: 21793 (view as bug list)
Depends on:
Blocks: 20813
  Show dependency treegraph
 
Reported: 2008-10-01 23:01 PDT by Cameron Zwarich (cpst)
Modified: 2008-10-22 17:16 PDT (History)
1 user (show)

See Also:


Attachments
patch, working but ChangeLog not done yet (52.44 KB, patch)
2008-10-22 10:08 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch (50.40 KB, patch)
2008-10-22 12:00 PDT, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-10-01 23:01:21 PDT
We always use a virtual call for getOwnPropertySlot(), when we should really be able to inline the common cases in many of its callers, or at least use non-virtual calls. This could be achieved by a bit in the StructureID TypeInfo, just like was done for 'instanceof'.
Comment 1 Cameron Zwarich (cpst) 2008-10-16 22:02:38 PDT
Sam is now working on other tasks, so I will assign this to myself.
Comment 2 Darin Adler 2008-10-22 09:39:10 PDT
*** Bug 21793 has been marked as a duplicate of this bug. ***
Comment 3 Darin Adler 2008-10-22 10:08:51 PDT
Created attachment 24559 [details]
patch, working but ChangeLog not done yet
Comment 4 Darin Adler 2008-10-22 12:00:55 PDT
Created attachment 24561 [details]
patch
Comment 5 Sam Weinig 2008-10-22 13:08:24 PDT
Comment on attachment 24561 [details]
patch

-    // WebCore uses this to make document.all and style.filter undetectable.
     static const unsigned MasqueradesAsUndefined = 1;
+        // WebCore uses MasqueradesAsUndefined to make document.all and style.filter undetectable.
     static const unsigned ImplementsHasInstance = 1 << 1;
The whitespace for this comment seems wrong.

+JSGlueGlobalObject::JSGlueGlobalObject(PassRefPtr<StructureID> structure, JSFlags flags)
+    : JSGlobalObject(structure, new Data, this)
+{
+    d()->flags = flags;
+    d()->userObjectStructure = UserObjectImp::createStructureID(jsNull());
+}
Might be cleaner to make the Data constructor take arguments.

+    if (prototype->isNull()) {
+        static StructureID* structureID = JSInspectorCallbackWrapper::createStructureID(jsNull()).releaseRef();
+        return new (unwrappedExec) JSInspectorCallbackWrapper(unwrappedExec, unwrappedObject, structureID);
+    }
Why is leaking the StructureID okay here?

For QtRuntimeMethod::createStructureID, why does it have the ImplementsHasInstance flag like InternalFunction?

r=me.
Comment 6 Darin Adler 2008-10-22 16:25:30 PDT
(In reply to comment #5)
> (From update of attachment 24561 [details] [edit])
> -    // WebCore uses this to make document.all and style.filter undetectable.
>      static const unsigned MasqueradesAsUndefined = 1;
> +        // WebCore uses MasqueradesAsUndefined to make document.all and
> style.filter undetectable.
>      static const unsigned ImplementsHasInstance = 1 << 1;
> The whitespace for this comment seems wrong.

I'll move the comment back.

> +    d()->flags = flags;
>
> Might be cleaner to make the Data constructor take arguments.

Sure, but since this is just JavaScriptGlue, I think I'll leave it as-is.

> +    if (prototype->isNull()) {
> +        static StructureID* structureID =
> JSInspectorCallbackWrapper::createStructureID(jsNull()).releaseRef();
> +        return new (unwrappedExec) JSInspectorCallbackWrapper(unwrappedExec,
> unwrappedObject, structureID);
> +    }
>
> Why is leaking the StructureID okay here?

As we discussed, I'll use StructureID::startIgnoringLeaks().

> For QtRuntimeMethod::createStructureID, why does it have the
> ImplementsHasInstance flag like InternalFunction?

Oops, will fix.
Comment 7 Darin Adler 2008-10-22 17:16:23 PDT
http://trac.webkit.org/changeset/37799