Last Comment Bug 779601 - Minor cleanup for JS_ValueToECMAInt32 and friends
: Minor cleanup for JS_ValueToECMAInt32 and friends
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jason Orendorff [:jorendorff]
: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-01 12:57 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-08-30 08:52 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.95 KB, patch)
2012-08-01 13:22 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (9.12 KB, patch)
2012-08-01 14:21 PDT, Jason Orendorff [:jorendorff]
bhackett1024: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-08-01 12:57:46 PDT

    
Comment 1 Jason Orendorff [:jorendorff] 2012-08-01 13:22:34 PDT
Created attachment 648058 [details] [diff] [review]
v1
Comment 2 Jason Orendorff [:jorendorff] 2012-08-01 13:39:16 PDT
Comment on attachment 648058 [details] [diff] [review]
v1

Adding more changes after talking this over with bhackett.
Comment 3 Jason Orendorff [:jorendorff] 2012-08-01 14:21:22 PDT
Created attachment 648081 [details] [diff] [review]
v2
Comment 4 Brian Hackett (:bhackett) 2012-08-01 14:31:22 PDT
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
Comment 5 Jason Orendorff [:jorendorff] 2012-08-02 13:01:17 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5f46bf6c00e
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2012-08-02 16:52:24 PDT
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...
Comment 7 Terrence Cole [:terrence] 2012-08-02 18:20:19 PDT
(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.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-08-02 19:08:11 PDT
https://hg.mozilla.org/mozilla-central/rev/a5f46bf6c00e
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-08-02 19:19:05 PDT
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?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-08-30 08:52:08 PDT
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.)

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