WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
15698
Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider
https://bugs.webkit.org/show_bug.cgi?id=15698
Summary
Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider
Maciej Stachowiak
Reported
2007-10-25 19:07:24 PDT
Optimize toPrimitive / toNumber conversions for 0.5% gain on SunSpider
Attachments
patch
(11.42 KB, patch)
2007-10-25 19:07 PDT
,
Maciej Stachowiak
oliver
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Maciej Stachowiak
Comment 1
2007-10-25 19:07:57 PDT
Created
attachment 16871
[details]
patch
Eric Seidel (no email)
Comment 2
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.
Eric Seidel (no email)
Comment 3
2007-10-25 19:25:50 PDT
jsUndefined() query, rather. Gosh I'm tired.
Maciej Stachowiak
Comment 4
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.
Oliver Hunt
Comment 5
2007-10-25 21:03:30 PDT
Comment on
attachment 16871
[details]
patch r=me assuming all the layout tests pass
Mark Rowe (bdash)
Comment 6
2007-10-25 21:48:00 PDT
This was landed in
r27086
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug