Bug 19721 - speed up JavaScriptCore by not wrapping strings in objects just to call functions on them
Summary: speed up JavaScriptCore by not wrapping strings in objects just to call funct...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-06-22 23:29 PDT by Darin Adler
Modified: 2008-06-26 19:53 PDT (History)
3 users (show)

See Also:


Attachments
patch for a step along the way (428.78 KB, patch)
2008-06-22 23:30 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch (71.07 KB, patch)
2008-06-25 23:59 PDT, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2008-06-22 23:29:24 PDT
We can speed up JavaScriptCore by making wrappers for primitive objects less often.
Comment 1 Darin Adler 2008-06-22 23:30:32 PDT
Created attachment 21878 [details]
patch for a step along the way
Comment 2 Oliver Hunt 2008-06-23 02:41:58 PDT
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.
Comment 3 Darin Adler 2008-06-23 09:55:52 PDT
(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.
Comment 4 Geoffrey Garen 2008-06-23 17:08:12 PDT
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
Comment 5 Darin Adler 2008-06-23 22:19:04 PDT
(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.
Comment 6 Darin Adler 2008-06-23 22:23:48 PDT
Comment on attachment 21878 [details]
patch for a step along the way

Committed revision 34754.

Clearing review flag now.
Comment 7 Darin Adler 2008-06-24 08:51:50 PDT
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.
Comment 8 Maciej Stachowiak 2008-06-24 09:18:02 PDT
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.
Comment 9 Geoffrey Garen 2008-06-24 14:31:48 PDT
hasInstance is generic enough that it would suffice just to run the normal hasInstance function for any object.
Comment 10 Darin Adler 2008-06-25 23:59:55 PDT
Created attachment 21941 [details]
patch
Comment 11 Geoffrey Garen 2008-06-26 16:01:24 PDT
Comment on attachment 21941 [details]
patch

+2008-06-25  Darin Adler  <darin@apple.com>
+
+        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
Comment 12 Darin Adler 2008-06-26 18:03:27 PDT
(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.
Comment 13 Darin Adler 2008-06-26 19:53:59 PDT
Committed revision 34821.