Bug 15698 – Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Maciej Stachowiak]

Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider

[Maciej Stachowiak]

Created attachment 16871 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 16871 [details]
patch

There must be some script for generating JavaScriptCore.exp, since I see you reordering the symbol I put in the wrong place (maybe you just used sort..)

I don't know why GetterSetterImp's ASSERT on primative calls, but I haven't looked at the code, looks like the other primative calls do too.

wasNotString1 would make more sense to me as v1IsNumber or something, or n1isValid, or something non-negative.

JSObject::getPrimativeNumber seems overly complicated.  I guess defaultValues for primitives can change?  (I can just go look at the code...)

Hum... isn't this wrong?

+    if (JSImmediate::isImmediate(this)) {
+        number = JSImmediate::toDouble(this);


it should be isNumber(this) no?  Otherwise you'll call toDouble on jsUndefined(), no?

Assuming you've run-javascriptcore-tests and fast/js (and you answer my unsigned query), this is fine.  I can mark it r+ when you reappear on IRC.

[Eric Seidel (no email)]

jsUndefined() query, rather.  Gosh I'm tired.

[Maciej Stachowiak]

getPrimitiveNumber() never fails. It *always* converts to number. The boolean return does not indicate whether it succeeded. It only indicates that this value did *not* have a preference for turning into a string instead of a number when converting toPrimitive() with a number hint. (I realize that's a little fruity, but that is pretty much the rule that less and lessEq require.)

To address specific points:

(In reply to comment #2)
> (From update of attachment 16871 [details] [edit])
> There must be some script for generating JavaScriptCore.exp, since I see you
> reordering the symbol I put in the wrong place (maybe you just used sort..)

I used M-x sort-lines in emacs.
 
> I don't know why GetterSetterImp's ASSERT on primative calls, but I haven't
> looked at the code, looks like the other primative calls do too.

GetterSetterImp only exists to have a garbage-collectable thing to put in the property map. It is never exposed to JavaScript, so the toPrimitive and related conversions can never actually be called in practice.

> wasNotString1 would make more sense to me as v1IsNumber or something, or
> n1isValid, or something non-negative.

The problem here is that the meaning is weird. All it says is that the value that was converted *didn't* prefer string. Maybe v1PrefersNumber?

> JSObject::getPrimativeNumber seems overly complicated.  I guess defaultValues
> for primitives can change?  (I can just go look at the code...)

It's just doing the combination of what JSObject::toPrimitive() and toNumber() on the result would do (plus checking whether a string was preferred).
 
> Hum... isn't this wrong?
> 
> +    if (JSImmediate::isImmediate(this)) {
> +        number = JSImmediate::toDouble(this);
> 
> 
> it should be isNumber(this) no?  Otherwise you'll call toDouble on
> jsUndefined(), no?

No, that's correct. All immediates prefer numeric conversion over string, and toDouble gives the right numeric equivalent.

> Assuming you've run-javascriptcore-tests and fast/js (and you answer my
> unsigned query), this is fine.  I can mark it r+ when you reappear on IRC.

I did run JavaScriptCore tests. About to run layout tests.

[Oliver Hunt]

Comment on attachment 16871 [details]
patch

r=me assuming all the layout tests pass

[Mark Rowe (bdash)]

This was landed in r27086.