Bug 16570 – Many toNumber() callers should use a JSValue::getNumber() which ASSERTs instead of branches

• Status: RESOLVED
• Resolution: INVALID
• Component: JavaScriptCore

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

Sunspider reported this as a wash.  It actually showed the patched version as possibly 1.001x as slow.

[Darin Adler]

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.

[Darin Adler]

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.

[Gavin Barraclough]

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.