Bug 15698

Summary: Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: JavaScriptCoreAssignee: Maciej Stachowiak <mjs>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P3    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
patch oliver: review+

Description Maciej Stachowiak 2007-10-25 19:07:24 PDT
Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider
Comment 1 Maciej Stachowiak 2007-10-25 19:07:57 PDT
Created attachment 16871 [details]
patch
Comment 2 Eric Seidel (no email) 2007-10-25 19:25:22 PDT
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.
Comment 3 Eric Seidel (no email) 2007-10-25 19:25:50 PDT
jsUndefined() query, rather.  Gosh I'm tired.
Comment 4 Maciej Stachowiak 2007-10-25 20:33:04 PDT
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.
Comment 5 Oliver Hunt 2007-10-25 21:03:30 PDT
Comment on attachment 16871 [details]
patch

r=me assuming all the layout tests pass
Comment 6 Mark Rowe (bdash) 2007-10-25 21:48:00 PDT
This was landed in r27086.