Many toNumber() callers should use a JSValue::getNumber() which ASSERTs instead of branches Right now we call toNumber() in too many places where we already know that something is a number (type() == NumberType). We could call getNumber(), but even that is slower than we need, since it returns NaN when it's not a number. We should either change getNumber or make a new function which ASSERTs that the JSValue is either an immediate number or a NumberImp* and returns the associated double without making a virtual call into toNumber() This is pretty low-hanging fruit and has shown up on some samples in places like addToNumber which calls toNumber() after checking first type() == NumberType. There are other callers like this as well.
Created attachment 18089 [details] Add ASSERTing getNumber and change old getNumber to getNumberOrNaN JavaScriptCore/API/JSCallbackObjectFunctions.h | 2 +- JavaScriptCore/bindings/c/c_utility.cpp | 2 +- JavaScriptCore/kjs/date_object.cpp | 8 ++++---- JavaScriptCore/kjs/nodes.cpp | 4 ++-- JavaScriptCore/kjs/operations.cpp | 6 +++--- JavaScriptCore/kjs/value.cpp | 8 +++++++- JavaScriptCore/kjs/value.h | 13 ++++++++++--- 7 files changed, 28 insertions(+), 15 deletions(-)
Sunspider reported this as a wash. It actually showed the patched version as possibly 1.001x as slow.
Comment on attachment 18089 [details] Add ASSERTing getNumber and change old getNumber to getNumberOrNaN I don't like the naming here. This changes getNumber to be different from getString in this respect, which doesn't make sense to me. It's fine if we want a faster version that requires that we already know it's a number, but I don't think it should take over the "getNumber" name nor do I think that the "OrNaN" suffix makes it clear what the distinction is. Since this is not a performance win, it has to pass a pretty high bar of "clarity win". This patch also includes the StringImp::value change in strictEqual -- seems like a good change but not consistent with the title of this bug. Maybe the name thing would be resolved if we went through the getXXX functions and created fast ones that assume the type for all of them? Booleans too. 111 double n1 = v1->toNumber(exec); 112 double n2 = v2->toNumber(exec); 111 double n1 = v1->getNumber(); 112 double n2 = v2->getNumber(); 113113 if (n1 == n2) 114114 return true; 115115 return false; There should be no local variables in that code -- it can all be in one line without named variables. 118118 else if (t2 == BooleanType) 119119 return v1->toBoolean(exec) == v2->toBoolean(exec); It's bizarre that this checks t2 instead of t1.
Comment on attachment 18089 [details] Add ASSERTing getNumber and change old getNumber to getNumberOrNaN I think I'll say review- to get this out of the review queue. I had some critical comments and also there's no change log here.
This code has changed too much for these changes to apply, but generally these changes have all already been made - where we do not require toNumber conversion we do now just call getNumber.