749535 - UnwrapObject slows down typed array APIs a lot

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

I meant 27%, of course.  Time for bed!

Comment 4

[Andreas Gal :gal]

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 [:bzbarsky]]

> 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]]

Attachment: Part 1- Add JS_UnwrapObjectAs*Array

Comment 7

[Eric Faust [:efaust]]

Attachment: Part 2- Rewrite dom::TypedArray to use JS_UnwrapObjectAs*Array

Comment 8

[Eric Faust [:efaust]]

Attachment: Part 3- Modify Paris Bindings to remove JS_Is*Array call from argument unwrapping.

Comment 9

[Eric Faust [:efaust]]

Attachment: Part 1- Add JS_UnwrapObjectAs*Array

Make a small fix.

Comment 10

[Eric Faust [:efaust]]

Attachment: Part 1- Add JS_UnwrapObjectAs*Array

Make small change compile >.>

Comment 11

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [Laid off!]]

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 [:bzbarsky]]

And perhaps what a NULL return value means?

Comment 15

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46f9f5e68082
https://hg.mozilla.org/integration/mozilla-inbound/rev/680f63b02698
https://hg.mozilla.org/integration/mozilla-inbound/rev/9abc2abb27b3

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/46f9f5e68082
https://hg.mozilla.org/mozilla-central/rev/680f63b02698
https://hg.mozilla.org/mozilla-central/rev/9abc2abb27b3

Comment 17

[Alon Zakai (:azakai)]

This looks awesome, do we have any benchmarks of how things are after this change?

Comment 18

[Boris Zbarsky [:bzbarsky]]

We should measure on http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/162bb59c573e/uniform-float-taking-typed-array.html, probably.

Comment 19

[Alon Zakai (:azakai)]

Looks impressive. On previous nightly and aurora I see 35-41. On today's nightly I see 25-29.