Last Comment Bug 739541 - Please add some sort of JSAPI with the performance characteristics of ToNumber()
: Please add some sort of JSAPI with the performance characteristics of ToNumber()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-03-27 01:07 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2012-03-29 08:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32. (5.96 KB, patch)
2012-03-27 22:47 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dmandelin: review+
Details | Diff | Review
Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32. (6.97 KB, patch)
2012-03-28 16:19 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
dmandelin: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-27 01:07:39 PDT
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 David Mandelin [:dmandelin] 2012-03-27 11:40:44 PDT
Good idea. Do you want to whip it up, put "good first bug" on it, or one of us to do it?
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-27 13:25:22 PDT
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 David Mandelin [:dmandelin] 2012-03-27 14:39:09 PDT
(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?
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-27 14:42:34 PDT
That involves somehow exposing either ToNumber or ToNumberSlow, doesn't it?  I'm ok with that, of course...
Comment 5 David Mandelin [:dmandelin] 2012-03-27 15:09:31 PDT
(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.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-27 22:47:33 PDT
Created attachment 610021 [details] [diff] [review]
Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32.

Sounds good to me!
Comment 7 :Ms2ger 2012-03-28 03:21:22 PDT
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?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-28 03:35:58 PDT
> 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?
Comment 9 David Mandelin [:dmandelin] 2012-03-28 16:10:48 PDT
(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.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-28 16:19:32 PDT
Created attachment 610354 [details] [diff] [review]
Add faster versions of JS_ValueToNumber and JS_ValueToECMAInt32.

I actually have this done now
Comment 11 David Mandelin [:dmandelin] 2012-03-28 16:21:54 PDT
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. :-)
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-28 16:36:03 PDT
Thanks for the quick review!

http://hg.mozilla.org/integration/mozilla-inbound/rev/b45903ca2c7a
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-28 16:44:16 PDT
Or not, because inbound is closed...
Comment 14 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-28 18:45:52 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/61ea97629f0f
Comment 15 Matt Brubeck (:mbrubeck) 2012-03-29 08:50:45 PDT
https://hg.mozilla.org/mozilla-central/rev/61ea97629f0f

Note You need to log in before you can comment on or make changes to this bug.