Bug 21294

Summary: Devirtualize getOwnPropertySlot()
Product: WebKit Reporter: Cameron Zwarich (cpst) <zwarich>
Component: JavaScriptCoreAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: darin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 20813    
Attachments:
Description Flags
patch, working but ChangeLog not done yet
none
patch sam: review+

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