Last Comment Bug 749535 - UnwrapObject slows down typed array APIs a lot
: UnwrapObject slows down typed array APIs a lot
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Eric Faust [:efaust]
:
Mentors:
Depends on:
Blocks: WebJSPerf ParisBindings
  Show dependency treegraph
 
Reported: 2012-04-26 23:53 PDT by Boris Zbarsky [:bz]
Modified: 2012-08-02 13:03 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Part 1- Add JS_UnwrapObjectAs*Array (7.44 KB, patch)
2012-07-30 18:11 PDT, Eric Faust [:efaust]
no flags Details | Diff | Splinter Review
Part 2- Rewrite dom::TypedArray to use JS_UnwrapObjectAs*Array (31.72 KB, patch)
2012-07-30 18:12 PDT, Eric Faust [:efaust]
bzbarsky: review+
Details | Diff | Splinter Review
Part 3- Modify Paris Bindings to remove JS_Is*Array call from argument unwrapping. (2.91 KB, patch)
2012-07-30 18:13 PDT, Eric Faust [:efaust]
bzbarsky: review+
Details | Diff | Splinter Review
Part 1- Add JS_UnwrapObjectAs*Array (7.54 KB, text/plain)
2012-07-30 18:20 PDT, Eric Faust [:efaust]
no flags Details
Part 1- Add JS_UnwrapObjectAs*Array (7.54 KB, patch)
2012-07-30 18:23 PDT, Eric Faust [:efaust]
bhackett1024: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2012-04-26 23:53:07 PDT
I'm doing some profiles of WebGL, and the UnwrapObject call typed arrays do on every single attempt to do anything with them is being pretty expensive: in the profile I'm looking at right now, 15% of the total time is spent calling it.

The reasons are twofold:

1)  Every single typed array API call calls UnwrapObject.  So using a typed array for just about anything involves 3 separate calls to this function: once for JS_Is*, once to get the length, and once to get the data pointer.

2)  The function itself is somewhat slow.  Most importantly, the "not a wrapper" fast path is not inlined, so The fast path (not a wrapper) is not inlined, for example.  But it also has the whole "get the jsclass" dance, which is not that fast either.

The typed array code can work around #2 by checking isWrapper() itself, thereby inlining the common-case fast path.

I don't know what to say about #1.  There seem to be two obvious options if we want to deal with it:

A)  Adding some unsafe APIs that will assert that the passed-in object is not a wrapper (thus allowing callers to do the unwrapping themselves before calling into these APIS).
B)  Adding an API that does the three things above in a single function call.  Frankly, I'd prefer this for my purposes, since it will incidentally reduce the number of times I have to pay the cost of making a function call to get something usable out of the typed array object.

Possible example of such an API:

  JSObject* JS_GetObjectAsFloat32Array(JSContext *cx, JSObject *obj,
                                       uint32_t* length, float** data);

returning null if can't unwrap or not a Float32Array, returning the underlying unwrapped array object otherwise and filling in the two out params.

Yes, I know it's ugly.  :(
Comment 1 Boris Zbarsky [:bz] 2012-04-26 23:58:07 PDT
Oh, and for scale, and for the reason I want to avoid the extra function calls... in the same testcase, the total time spent calling JS_GetTypedArrayLength, JS_IsFloat32Array, and JS_GetFloat32ArrayData is about 23% of total runtime.  So the UnwrapObject is about 2/3 of that, and the whole typed array part is about 1/4 of _everything_.
Comment 2 Boris Zbarsky [:bz] 2012-04-27 00:02:32 PDT
Actually, make that 22%; I missed the 4% for the dynamic loader stubs due to all of these being cross-library calls....
Comment 3 Boris Zbarsky [:bz] 2012-04-27 00:02:48 PDT
I meant 27%, of course.  Time for bed!
Comment 4 Andreas Gal :gal 2012-04-27 03:13:01 PDT
This is a very nice API IMO:

JSObject* JS_GetObjectAsFloat32Array(JSContext *cx, JSObject *obj,
                                       uint32_t* length, float** data);

Same for Float64 and Int32 would be cool. Maybe even a flag so that we can allocate memory that gets freed if we have a regular object where we have to run through it and convert the data so the API never fails.
Comment 5 Boris Zbarsky [:bz] 2012-04-27 08:49:35 PDT
> Same for Float64 and Int32 would be cool.

I'd need it for all the typed array types; Float32 was just an example.  Also for arraybufferview and arraybuffer (handing out uint8_t**, ideally).
Comment 6 Eric Faust [:efaust] 2012-07-30 18:11:28 PDT
Created attachment 647382 [details] [diff] [review]
Part 1- Add JS_UnwrapObjectAs*Array
Comment 7 Eric Faust [:efaust] 2012-07-30 18:12:35 PDT
Created attachment 647384 [details] [diff] [review]
Part 2- Rewrite dom::TypedArray to use JS_UnwrapObjectAs*Array
Comment 8 Eric Faust [:efaust] 2012-07-30 18:13:30 PDT
Created attachment 647386 [details] [diff] [review]
Part 3- Modify Paris Bindings to remove JS_Is*Array call from argument unwrapping.
Comment 9 Eric Faust [:efaust] 2012-07-30 18:20:45 PDT
Created attachment 647389 [details]
Part 1- Add JS_UnwrapObjectAs*Array

Make a small fix.
Comment 10 Eric Faust [:efaust] 2012-07-30 18:23:56 PDT
Created attachment 647390 [details] [diff] [review]
Part 1- Add JS_UnwrapObjectAs*Array

Make small change compile >.>
Comment 11 Boris Zbarsky [:bz] 2012-07-30 21:44:21 PDT
Comment on attachment 647384 [details] [diff] [review]
Part 2- Rewrite dom::TypedArray to use JS_UnwrapObjectAs*Array

Please don't make the various random whitespace changes in the canvas code.

I'd prefer s/UnboxInputs/UnboxArray/, I think.

You don't need a boolean inited member, looks like; you can just make inited() return !!mObj.

You also don't need the '_' on the ends of the member names.

Do you really need to initialize members in the constructor if you'll immediately turn around and set them?  I guess it saves us in opt builds, maybe, in case when the inited() asserts would fire...

I really appreciate the type for arraybufferview being made uint8_t!

r=me with the above nits picked.
Comment 12 Boris Zbarsky [:bz] 2012-07-30 21:49:27 PDT
Comment on attachment 647386 [details] [diff] [review]
Part 3- Modify Paris Bindings to remove JS_Is*Array call from argument unwrapping.

The const_cast bit is just bug 766583; I wouldn't worry about commenting it here.

The construction happening in the failure case is really only a problem during overload resolution, and it's not really any slower than the old check was, right?

So r=me with the XXX comment probably removed.
Comment 13 Brian Hackett (:bhackett) 2012-07-31 05:10:49 PDT
Comment on attachment 647390 [details] [diff] [review]
Part 1- Add JS_UnwrapObjectAs*Array

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

::: js/src/jsfriendapi.h
@@ +1094,5 @@
>  JS_IsFloat64Array(JSObject *obj, JSContext *cx);
>  
> +
> +/*
> + * Unwrap Typed arrays all at once.

Can you comment that these do not throw on a NULL return?
Comment 14 Boris Zbarsky [:bz] 2012-07-31 07:50:41 PDT
And perhaps what a NULL return value means?
Comment 17 Alon Zakai (:azakai) 2012-08-01 09:59:23 PDT
This looks awesome, do we have any benchmarks of how things are after this change?
Comment 19 Alon Zakai (:azakai) 2012-08-02 13:03:15 PDT
Looks impressive. On previous nightly and aurora I see 35-41. On today's nightly I see 25-29.

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