Closed
Bug 739541
Opened 12 years ago
Closed 12 years ago
Please add some sort of JSAPI with the performance characteristics of ToNumber()
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
6.97 KB,
patch
|
dmandelin
:
review+
|
Details | Diff | Splinter Review |
In particular, an inline fast path for the isNumber() case. I can, of course, roll my own if I have to, but this seems like it would be better as part of the public API. As a specific example, if I have a new-binding DOM function that takes 6 float arguments, each call has 60ns of overhead. If I replace the JS_ValueToNumber calls in the binding with calls to MyValueToNumber, defined as so: inline bool MyValueToNumber(JSContext *cx, jsval v, double *dp) { if (v.isNumber()) { *dp = v.toNumber(); return true; } return JS_ValueToNumber(cx, v, dp); } that drops to 40ns of overhead...
Comment 1•12 years ago
|
||
Good idea. Do you want to whip it up, put "good first bug" on it, or one of us to do it?
Assignee | ||
Comment 2•12 years ago
|
||
The hard of this is the API design, not the code... I'm happy to write the code, but I'm not sure what the API should look like.
Comment 3•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #2) > The hard of this is the API design, not the code... I'm happy to write the > code, but I'm not sure what the API should look like. How about just turning JS_ValueToNumber into your version?
Assignee | ||
Comment 4•12 years ago
|
||
That involves somehow exposing either ToNumber or ToNumberSlow, doesn't it? I'm ok with that, of course...
Comment 5•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #4) > That involves somehow exposing either ToNumber or ToNumberSlow, doesn't it? > I'm ok with that, of course... I think it's OK too. JS::ToNumber does something out of the spec, and it's namespaced, so it doesn't harmful to expose it. I guess you'll actually be exposing ToNumberSlow, which is implementation, but it doesn't seem like a very tempting thing to call, so I doubt it would cause us much trouble. In a future C++ API, we'd probably have something like Value::toNumber as the API, which could call a private method toNumberImpl for the slow case. The corresponding thing to do in JSAPI would be to make a friend API for the slow case, but it doesn't seem worth bothering with for this particular feature. So I say just expose ToNumber(Slow) as you need for now, and if more of this stuff comes in we'll pick a better place for the implementation bits.
Assignee | ||
Updated•12 years ago
|
Assignee: general → bzbarsky
Whiteboard: [need review]
Assignee | ||
Updated•12 years ago
|
Blocks: ParisBindings
Comment 7•12 years ago
|
||
Comment on attachment 610021 [details] [diff] [review] Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32. Review of attachment 610021 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi.h @@ +2248,5 @@ > +/* > + * DO NOT CALL THIS. Use JS::ToNumber > + */ > +extern JS_PUBLIC_API(bool) > +ToNumberSlow(JSContext *cx, js::Value v, double *dp); I'm surprised this works with js::Value instead of JS:: @@ +2259,5 @@ > +ToNumber(JSContext *cx, const Value &v, double *out) > +{ > + // Can't check for cx->rt->gcRunning here, unfortunately > + CHECK_REQUEST(cx); > + // Can't use assertSameCompartment here, unfortunately Can't you add a AssertArgumentsAreSane(JSContext *cx, const JS::Value &v) that's inline/no-op in opt builds and extern/implemented in jsapi.cpp in debug?
Assignee | ||
Comment 8•12 years ago
|
||
> I'm surprised this works with js::Value instead of JS:: Hmm. I can change that as needed, I guess... > Can't you add a AssertArgumentsAreSane Yes. dmandelin, do you want me to?
Updated•12 years ago
|
Attachment #610021 -
Flags: review?(dmandelin) → review+
Comment 9•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #8) > > I'm surprised this works with js::Value instead of JS:: > > Hmm. I can change that as needed, I guess... > > > Can't you add a AssertArgumentsAreSane > > Yes. dmandelin, do you want me to? I don't think it's necessary right now (getting the perf you need is more important), but if either of you or Ms2ger wants to do it, that's fine too.
Assignee | ||
Comment 10•12 years ago
|
||
I actually have this done now
Attachment #610354 -
Flags: review?(dmandelin)
Assignee | ||
Updated•12 years ago
|
Attachment #610021 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Comment on attachment 610354 [details] [diff] [review] Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32. Review of attachment 610354 [details] [diff] [review]: ----------------------------------------------------------------- I guess we can sleep a little bit better this way. :-)
Attachment #610354 -
Flags: review?(dmandelin) → review+
Assignee | ||
Comment 12•12 years ago
|
||
Thanks for the quick review! http://hg.mozilla.org/integration/mozilla-inbound/rev/b45903ca2c7a
Whiteboard: [need review]
Target Milestone: --- → mozilla14
Assignee | ||
Comment 13•12 years ago
|
||
Or not, because inbound is closed...
Whiteboard: [need landing]
Target Milestone: mozilla14 → ---
Assignee | ||
Comment 14•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/61ea97629f0f
Whiteboard: [need landing]
Target Milestone: --- → mozilla14
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/61ea97629f0f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•