Minor cleanup for JS_ValueToECMAInt32 and friends

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 648058 [details] [diff] [review]
v1
Assignee: general → jorendorff
Attachment #648058 - Flags: review?(luke)
(Assignee)

Comment 2

5 years ago
Comment on attachment 648058 [details] [diff] [review]
v1

Adding more changes after talking this over with bhackett.
Attachment #648058 - Flags: review?(luke)
(Assignee)

Comment 3

5 years ago
Created attachment 648081 [details] [diff] [review]
v2
Attachment #648058 - Attachment is obsolete: true
Attachment #648081 - Flags: review?(bhackett1024)
Comment on attachment 648081 [details] [diff] [review]
v2

Review of attachment 648081 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2741,5 @@
>  JS_ValueToECMAInt32(JSContext *cx, jsval v, int32_t *ip);
>  
>  #ifdef __cplusplus
>  namespace js {
> +/* DO NOT CALL THIS.  Use JS::ToInt32. */

JS::ToUint16
Attachment #648081 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f46bf6c00e
How expensive is the root stuff in ToNumber and ToInt32?  Is there any way we could get rid of it?  Those are pretty hot paths in binding code...
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #6)
> How expensive is the root stuff in ToNumber and ToInt32?  Is there any way
> we could get rid of it?  Those are pretty hot paths in binding code...

Yes, absolutely.  We would like to take a HandleValue here and use the existing root rather than creating a new one.  Work should be starting soon under bug 773686 now that bug 777219 has landed.  We could certainly do this bit first, especially if you have time to help from the DOM side.
https://hg.mozilla.org/mozilla-central/rev/a5f46bf6c00e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
I can definitely help on the DOM side, modulo the whole "gone Aug 11-20 thing".

On the DOM side, the common case is that the value is coming out of a JSNative's vp or he equivalent for the new setup efaust added for ionmonkey.  A slightly more rare case is that I'm working with a property I just got off an object via JS_GetProperty or JS_GetElement.  So we could presumably push the bits from here out to the DOM code for now, pending those three things getting converted, but to get a real win perf-wise they'd need to be converted too, right?
Yes, they'd need to be converted.  (HANDLE ALL THE THINGS.  Ahem.)  If we're changing JSNative, tho, we should probably just change it to JS::CallArgs, which is not far from including all the handle stuff.  (I see that JS::CallReceiver which it derives from is handle-ized, but CallArgs still appears to need a little work yet.)
You need to log in before you can comment on or make changes to this bug.