1139769 - Self-host %TypedArray%.prototype.subarray

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

Do less in C++, do more in JS.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Add TypedArrayBuffer, TypedArrayByteOffset, and TypedArrayElementShift intrinsics

These are needed in the guts of subarray, and I'd be surprised if they're not needed elsewhere eventually as well.  Right now they're purely intrinsics, no JIT inlining or anything.  We can add that later as needed.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Actually self-host %TypedArray%.prototype.subarray

The subarray operation allocates a new object, so it seems like a somewhat unlikely operation to be the gating factor on performance, so I didn't bother perf-testing this.  In any event, I'm not all that sure what I could do to make this faster if I wanted to.  Inline a few intrinsics, I guess, but that's pretty small potatoes, and unlikely to make the sort of big differences that would have to be present for us to care.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8573075 [details] [diff] [review]
Add TypedArrayBuffer, TypedArrayByteOffset, and TypedArrayElementShift intrinsics

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

r=me

::: js/src/vm/SelfHosting.cpp
@@ +999,5 @@
>  
>      JS_FN("IsTypedArray",            intrinsic_IsTypedArray,            1,0),
> +    JS_FN("TypedArrayBuffer",        intrinsic_TypedArrayBuffer,        1,0),
> +    JS_FN("TypedArrayByteOffset",    intrinsic_TypedArrayByteOffset,    1,0),
> +    JS_FN("TypedArrayElementShift",  intrinsic_TypedArrayElementShift,  1,0),

Same comment as for copyWithin: up to you whether you follow the "_Foo" convention.

(Yes, I realize that pointing this out is slightly silly as you clearly decided not to follow it. Mentioning it more for completeness' sake than anything else.)

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8573096 [details] [diff] [review]
Actually self-host %TypedArray%.prototype.subarray

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

Very nice! r=me.

::: js/src/builtin/TypedArray.js
@@ +749,4 @@
>      return false;
>  }
>  
> +// ES6 draft 20150220 %TypedArray%.prototype.subarray

nit: update to current draft.

::: js/src/vm/TypedArrayObject.cpp
@@ -783,5 @@
> -TypedArrayObject::subarray(JSContext *cx, unsigned argc, Value *vp)
> -{
> -    CallArgs args = CallArgsFromVp(argc, vp);
> -    return CallNonGenericMethod<TypedArrayObject::is,
> -                                TypedArrayMethods<TypedArrayObject>::subarray>(cx, args);

Again, would be nice to either move this to SharedTypedArrayObject.cpp, or at least add a comment explaining it's not used by TypedArray.

Comment 5

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a2dcf4b35f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1926f1d553bb

I didn't move the old C++ method into shared typed array code because we want to replace it with a self-hosted implementation eventually.  No particular reason to clutter up blame by moving it if we can help it.  Did add a comment saying it was only used by shared typed arrays, tho.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a2dcf4b35f1
https://hg.mozilla.org/mozilla-central/rev/1926f1d553bb

Comment 7

[Justin D'Arcangelo [:justindarc]]

The patch in Attachment 8573096 [details] [diff] has caused a regression that causes the JSZip library to generate corrupt ZIP files: http://stuk.github.io/jszip/

I'll post a follow-up bug with a test case shortly.

Comment 8

[Doug Sherk (:drs) (inactive)]

Backed out 1926f1d553bb from cypress: https://hg.mozilla.org/projects/cypress/rev/b7414c6c13c2

Comment 9

[Doug Sherk (:drs) (inactive)]

This is still causing bug 1147642, which is breaking content. Please back this out.

Comment 10

[Jeff Walden [:Waldo]]

Thanks for letting me know.  I'll look.  (Although I strongly doubt this bug's work is directly at fault in this.)

Comment 11

[Doug Sherk (:drs) (inactive)]

Nevermind, sorry, works now.