Bug 19721 – speed up JavaScriptCore by not wrapping strings in objects just to call functions on them

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Darin Adler]

We can speed up JavaScriptCore by making wrappers for primitive objects less often.

[Darin Adler]

Created attachment 21878 [details]
patch for a step along the way

[Oliver Hunt]

How would this interact with our need to fix primitive types toString/valueOf implementations be user overridable (eg. String.prototype.toString = function() {return "foo"; } -- that's a different bug, but i'd want it to still be possible to fix it with this patch.

[Darin Adler]

(In reply to comment #2)
> How would this interact with our need to fix primitive types toString/valueOf
> implementations be user overridable (eg. String.prototype.toString = function()
> {return "foo"; } -- that's a different bug, but i'd want it to still be
> possible to fix it with this patch.

This patch should not make it any harder to fix. It sounds really simple to me.

[Geoffrey Garen]

Comment on attachment 21878 [details]
patch for a step along the way

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

[Darin Adler]

(In reply to comment #4)
> I'm a little worried about how many places in the code need to know to call
> toThisObject.

It's annoying. And it's also annoying that it's hard to know when toThisObject is needed and when toObject would suffice.

> I guess the JSValue* in some function signatures mitigates the
> problem, but I wish we could do better.

It's very difficult to forget that you're dealing with a value that may not be an object. But it's easy to forget that you could be dealing with a window object that needs to be wrapped with a shell. I'm pretty sure I got it right, but I'm not sure how safe it will be in the future.

> The answer to your questions is that we should just nix implementsHasInstance()
> and all uses of it.

I'll look into that in the future.

> -    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".

Not exactly. Because inherits isn't a virtual function. But classInfo is. I did what you suggested by moving classInfo into JSCell.

> -    if (v->isObject() && static_cast<JSObject*>(v)->implementsCall()) {
> 
> Why is it suddenly OK to install a timer for something that is not a function?

I changed the behavior back by adding a getCallData there.

> Please add a SunSpider result to the ChangeLog.

SunSpider says this has no measurable performance impact, and I now say so in the ChangeLog.

[Darin Adler]

Comment on attachment 21878 [details]
patch for a step along the way

Committed revision 34754.

Clearing review flag now.

[Darin Adler]

Here's an interesting discovery. To call a function on a string, we actually make *2* string wrappers. The first is made to perform the "get" operation, and the second is made to call the retrieved function.

[Maciej Stachowiak]

Functions implement hasInstance because JS functions can be ued as constructors. We could of course choose to have hasInstance just always return false for objects that can't be constructors.

[Geoffrey Garen]

hasInstance is generic enough that it would suffice just to run the normal hasInstance function for any object.

[Darin Adler]

Created attachment 21941 [details]
patch

[Geoffrey Garen]

Comment on attachment 21941 [details]
patch

[phone]  Darin Adler  <[e-mail]>
+
+        Reviewed by NOBODY (OOPS!).
+
+        * ChangeLog:
+        * JavaScriptCore.exp:
+        * VM/JSPropertyNameIterator.cpp:
+        * VM/Machine.cpp:


Looks like you got two ChangeLogs for the price of one.

+JSObject* JSImmediate::prototype(const JSValue* v, ExecState* exec)
+{
+    ASSERT(isImmediate(v));
+    if (v == jsNull())
+        return new (exec) JSNotAnObject(throwError(exec, TypeError, "Null value"));
+    if (v == jsUndefined())
+        return new (exec) JSNotAnObject(throwError(exec, TypeError, "Undefined value"));
+    if (isBoolean(v))
+        return exec->lexicalGlobalObject()->booleanPrototype();
+    ASSERT(isNumber(v));
+    return exec->lexicalGlobalObject()->numberPrototype();
+}

It surprised me that you put the ifs for error cases before the ifs for normal cases. I would expect number to be most likely, followed by boolean, undefined, null. Maybe that order would speed things up a little.

I will trade not asking you to split this patch up in exchange for asking you for a couple of string replacement regression tests (unless there's a bunch of coverage in the Mozilla tests?)

r=me
Geoff

[Darin Adler]

(In reply to comment #11)
> +JSObject* JSImmediate::prototype(const JSValue* v, ExecState* exec)
> +{
> +    ASSERT(isImmediate(v));
> +    if (v == jsNull())
> +        return new (exec) JSNotAnObject(throwError(exec, TypeError, "Null
> value"));
> +    if (v == jsUndefined())
> +        return new (exec) JSNotAnObject(throwError(exec, TypeError, "Undefined
> value"));
> +    if (isBoolean(v))
> +        return exec->lexicalGlobalObject()->booleanPrototype();
> +    ASSERT(isNumber(v));
> +    return exec->lexicalGlobalObject()->numberPrototype();
> +}
> 
> It surprised me that you put the ifs for error cases before the ifs for normal
> cases. I would expect number to be most likely, followed by boolean, undefined,
> null. Maybe that order would speed things up a little.

Good idea. I just copied this from JSImmediate::toObject. I'll reorder both.

> I will trade not asking you to split this patch up in exchange for asking you
> for a couple of string replacement regression tests (unless there's a bunch of
> coverage in the Mozilla tests?)

There are lots of replace tests in LayoutTests, particularly in fast/js/kde/RegExp.html, fast/js/kde/StringObject.html, fast/js/regexp*.html, and fast/js/string-replace*.html. But I don't object to adding some new ones.

[Darin Adler]

Committed revision 34821.