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)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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...
Good idea. Do you want to whip it up, put "good first bug" on it, or one of us to do it?
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.
(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?
That involves somehow exposing either ToNumber or ToNumberSlow, doesn't it?  I'm ok with that, of course...
(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.
Sounds good to me!
Attachment #610021 - Flags: review?(dmandelin)
Assignee: general → bzbarsky
Whiteboard: [need review]
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?
> 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?
Attachment #610021 - Flags: review?(dmandelin) → review+
(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.
I actually have this done now
Attachment #610354 - Flags: review?(dmandelin)
Attachment #610021 - Attachment is obsolete: true
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+
Thanks for the quick review!

http://hg.mozilla.org/integration/mozilla-inbound/rev/b45903ca2c7a
Whiteboard: [need review]
Target Milestone: --- → mozilla14
Or not, because inbound is closed...
Whiteboard: [need landing]
Target Milestone: mozilla14 → ---
http://hg.mozilla.org/integration/mozilla-inbound/rev/61ea97629f0f
Whiteboard: [need landing]
Target Milestone: --- → mozilla14
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.

Attachment

General

Created:
Updated:
Size: