Summary: | Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||
Component: | JavaScriptCore | Assignee: | Maciej Stachowiak <mjs> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | ||||||
Priority: | P3 | ||||||
Version: | 523.x (Safari 3) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.4 | ||||||
Attachments: |
|
Description
Maciej Stachowiak
2007-10-25 19:07:24 PDT
Created attachment 16871 [details]
patch
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.
jsUndefined() query, rather. Gosh I'm tired. 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 on attachment 16871 [details]
patch
r=me assuming all the layout tests pass
|