Self-host %TypedArray%.prototype.subarray

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Do less in C++, do more in JS.
(Assignee)

Comment 1

3 years ago
Created attachment 8573075 [details] [diff] [review]
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.
Attachment #8573075 - Flags: review?(till)
(Assignee)

Comment 2

3 years ago
Created attachment 8573096 [details] [diff] [review]
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.
Attachment #8573096 - Flags: review?(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.)
Attachment #8573075 - Flags: review?(till) → review+
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.
Attachment #8573096 - Flags: review?(till) → review+
(Assignee)

Comment 5

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/9a2dcf4b35f1
https://hg.mozilla.org/mozilla-central/rev/1926f1d553bb
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39

Updated

3 years ago
Depends on: 1145058
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.
See Also: → bug 1147642
Depends on: 1147642
See Also: bug 1147642
Backed out 1926f1d553bb from cypress: https://hg.mozilla.org/projects/cypress/rev/b7414c6c13c2
Depends on: 1148970
(Assignee)

Updated

3 years ago
No longer depends on: 1148970
This is still causing bug 1147642, which is breaking content. Please back this out.
Keywords: checkin-needed
(Assignee)

Comment 10

3 years ago
Thanks for letting me know.  I'll look.  (Although I strongly doubt this bug's work is directly at fault in this.)
Nevermind, sorry, works now.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.