Closed
Bug 1139769
Opened 9 years ago
Closed 9 years ago
Self-host %TypedArray%.prototype.subarray
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
Attachments
(2 files)
3.52 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
3.56 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
Do less in C++, do more in JS.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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 3•9 years ago
|
||
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 4•9 years ago
|
||
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•9 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.
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9a2dcf4b35f1 https://hg.mozilla.org/mozilla-central/rev/1926f1d553bb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 7•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 8•9 years ago
|
||
Backed out 1926f1d553bb from cypress: https://hg.mozilla.org/projects/cypress/rev/b7414c6c13c2
Comment 9•9 years ago
|
||
This is still causing bug 1147642, which is breaking content. Please back this out.
Keywords: checkin-needed
Assignee | ||
Comment 10•9 years ago
|
||
Thanks for letting me know. I'll look. (Although I strongly doubt this bug's work is directly at fault in this.)
You need to log in
before you can comment on or make changes to this bug.
Description
•