[webkit-reviews] review granted: [Bug 19721] speed up JavaScriptCore by not wrapping strings in objects just to call functions on them : [Attachment 21878] patch for a step along the way

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 19721: speed up JavaScriptCore by not wrapping strings in objects just to
call functions on them
https://bugs.webkit.org/show_bug.cgi?id=19721

Attachment 21878: patch for a step along the way
https://bugs.webkit.org/attachment.cgi?id=21878&action=edit

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
This patch does a lot at once, but I like everything it does, so I guess I
shouldn't complain.

+    JSObject* thisObj = thisValue->toThisObject(exec);

I'm a little worried about how many places in the code need to know to call
toThisObject. I guess the JSValue* in some function signatures mitigates the
problem, but I wish we could do better.

+    // FIXME: What does implementing instanceof have to do with being a
function?
+    // Is this really right for JSFunction? Is it right for other classes
derived from this one?
+    // Maybe add a class called Constructor that is derived from this one for
use of the classes
+    // that do implement instanceof.

The answer to your questions is that we should just nix implementsHasInstance()
and all uses of it. Yes, a host object can choose not to "implement
hasInstance" -- whatever that means -- but why would we ever want to do that?
(Or, to answer my own question, "We never want to do that.")

-    if (!thisObj->inherits(&JSArray::info))
+    if (!thisValue->isObject(&JSArray::info))

All these conversions to "isObject" are a net performance loss, because
isObject calls two virtual functions, whereas inherits was one virtual function
call.  You could easily fix this by changing JSCell::isObject to be virtual,
returning false, and then renaming "inherits" to "isObject".

-    if (v->isObject() && static_cast<JSObject*>(v)->implementsCall()) {

Why is it suddenly OK to install a timer for something that is not a function?

Please add a SunSpider result to the ChangeLog.

r=me