UnwrapObject slows down typed array APIs a lot

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: efaust)

Tracking

(Blocks: 2 bugs, {perf})

unspecified
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:-)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.  :(
Keywords: perf
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_.
Blocks: 580070
Actually, make that 22%; I missed the 4% for the dynamic loader stubs due to all of these being cross-library calls....
I meant 27%, of course.  Time for bed!

Comment 4

5 years ago
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.
> 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).
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → -
Blocks: 579390
(Assignee)

Comment 6

5 years ago
Created attachment 647382 [details] [diff] [review]
Part 1- Add JS_UnwrapObjectAs*Array
Assignee: general → efaust
Status: NEW → ASSIGNED
Attachment #647382 - Flags: review?(bhackett1024)
(Assignee)

Comment 7

5 years ago
Created attachment 647384 [details] [diff] [review]
Part 2- Rewrite dom::TypedArray to use JS_UnwrapObjectAs*Array
Attachment #647384 - Flags: review?(bzbarsky)
(Assignee)

Comment 8

5 years ago
Created attachment 647386 [details] [diff] [review]
Part 3- Modify Paris Bindings to remove JS_Is*Array call from argument unwrapping.
Attachment #647386 - Flags: review?(peterv)
(Assignee)

Comment 9

5 years ago
Created attachment 647389 [details]
Part 1- Add JS_UnwrapObjectAs*Array

Make a small fix.
Attachment #647382 - Attachment is obsolete: true
Attachment #647382 - Flags: review?(bhackett1024)
Attachment #647389 - Flags: review?(bhackett1024)
(Assignee)

Comment 10

5 years ago
Created attachment 647390 [details] [diff] [review]
Part 1- Add JS_UnwrapObjectAs*Array

Make small change compile >.>
Attachment #647389 - Attachment is obsolete: true
Attachment #647389 - Flags: review?(bhackett1024)
Attachment #647390 - Flags: review?(bhackett1024)
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.
Attachment #647384 - Flags: review?(bzbarsky) → review+
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.
Attachment #647386 - Flags: review?(peterv) → review+
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?
Attachment #647390 - Flags: review?(bhackett1024) → review+
And perhaps what a NULL return value means?
(Assignee)

Comment 15

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/46f9f5e68082
https://hg.mozilla.org/mozilla-central/rev/680f63b02698
https://hg.mozilla.org/mozilla-central/rev/9abc2abb27b3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
This looks awesome, do we have any benchmarks of how things are after this change?
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.
Looks impressive. On previous nightly and aurora I see 35-41. On today's nightly I see 25-29.
You need to log in before you can comment on or make changes to this bug.