Minor cleanup for JS_ValueToECMAInt32 and friends

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Assignee

Comment 1

7 years ago
Posted patch v1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attachment #648058 - Flags: review?(luke)
Assignee

Comment 2

7 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

7 years ago
Posted patch v2Splinter Review
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+
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: 7 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.