We can speed up JavaScriptCore by making wrappers for primitive objects less often.
Created attachment 21878 [details] patch for a step along the way
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.
(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 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
(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 on attachment 21878 [details] patch for a step along the way Committed revision 34754. Clearing review flag now.
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.
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.
hasInstance is generic enough that it would suffice just to run the normal hasInstance function for any object.
Created attachment 21941 [details] patch
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
(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.
Committed revision 34821.