Closed Bug 749535 Opened 12 years ago Closed 12 years ago

UnwrapObject slows down typed array APIs a lot


(Core :: JavaScript Engine, defect)

Not set



blocking-kilimanjaro -


(Reporter: bzbarsky, Assigned: efaust)


(Blocks 1 open bug)


(Keywords: perf)


(3 files, 2 obsolete files)

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_.
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!
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: ? → -
Assignee: general → efaust
Attachment #647382 - Flags: review?(bhackett1024)
Attached file Part 1- Add JS_UnwrapObjectAs*Array (obsolete) —
Make a small fix.
Attachment #647382 - Attachment is obsolete: true
Attachment #647382 - Flags: review?(bhackett1024)
Attachment #647389 - Flags: review?(bhackett1024)
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?
This looks awesome, do we have any benchmarks of how things are after this change?
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.