Closed
Bug 1140152
Opened 9 years ago
Closed 6 years ago
%TypedArray%.prototype.slice must copy elements' underlying bit patterns
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Waldo, Assigned: anba)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 4 obsolete files)
44.92 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
The recent requirement that copyWithin, set, and slice copy elements' underlying bit pattterns, means that it's no longer permissible to self-host a loop to copy elements one by one. The first two, as implemented in C++, do memcpy/memmove when needed and so have no problems. (And the forthcoming self-hosted versions are careful to keep doing so.) The third, however, is self-hosted code that doesn't use an intrinsic to perform the copying, so it has wrong behavior for this. Testcase: var u32 = new Uint32Array([0xFFFFFFFF, 0xFFFFFFFF]); var f64 = new Float64Array(u32.buffer); assertEq(f64[0] !== f64[0], true, "must contain NaN"); var viewedSlice = new Uint32Array(f64.slice().buffer); assertEq(viewedSlice[0], 0xFFFFFFFF); assertEq(viewedSlice[1], 0xFFFFFFFF); https://bugs.ecmascript.org/show_bug.cgi?id=3694
Reporter | ||
Comment 2•9 years ago
|
||
We do not. There's a MoveTypedArrayElements intrinsic that does a memmove *within* a typed array's elements, that I added for copyWithin. But there's nothing to do cross-copying yet. It shouldn't be difficult to add such an intrinsic; MoveTypedArrayElements provides a template for how to do it. That said, I'm currently working on self-hosting %TypedArray%.prototype.set (no bug on file yet), and that's going to need an intrinsic along these lines, so you could also just wait on that. The conflicts from either side landing its thing first are minimal, so no reason for either side to wait on the other from a technical point of view.
Assignee | ||
Comment 3•8 years ago
|
||
Prerequisite for the patch, actually part of bug 1122396.
Assignee: dirkjan → andrebargull
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
Proof of concept patch. This is heavily based on intrinsic_SetFromTypedArrayApproach, under the assumption the unsafe parts in intrinsic_SetFromTypedArrayApproach are correct. So, does this approach look okay to you? Still needs tests, obviously. :-) Performance compared to current tip is slightly worse for small typed arrays (< 100 elements), copying larger typed array (> 1000 elements) is noticeably faster.
Attachment #8774858 -
Flags: feedback?(jwalden+bmo)
Updated•8 years ago
|
Blocks: es6typedarray
Assignee | ||
Updated•8 years ago
|
Attachment #8774854 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8774858 -
Attachment is obsolete: true
Attachment #8774858 -
Flags: feedback?(jwalden+bmo)
Assignee | ||
Comment 6•8 years ago
|
||
This is based on the approach used in intrinsic_SetFromTypedArrayApproach, so I assume it uses the correct way to handle cross-compartment typed arrays. And yay for not having to write new tests, I could simply import some existing tests originally written for es6draft. \o/
Attachment #8806014 -
Flags: review?(jdemooij)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8806014 [details] [diff] [review] bug1140152.patch Cancelling review until the possible cross-compartment issues mentioned in bug 1140752, comment 12 are solved.
Attachment #8806014 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8133cc6144f27a0e4a4577b06bf16219fba384d
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 9•7 years ago
|
||
There's a minor bug in the patch: We cannot use memmove when the source and the target use the same ArrayBuffer, because of the way memmove special cases when source and target use overlapping memory regions. (This case is only possible when the @@species constructor returns the input typed array!)
Reporter | ||
Comment 10•7 years ago
|
||
So as far as .set self-hosting tactics and self-hosting this, and the scary-ish non-JSAutoCompartment dance. I *did* talk to bholley about this at Mozlando, after far too great a delay, and at the time he convinced me that could be simplified. Then I looked at this again, and particularly how we have a self-hosted intrinsic for inlining the function-call with non-callVM ABI, when the arguments have copacetic, non-wrapped types. And I became re-convinced that bholley's hopes are just not realizable here. So, I think an approach patterned on what the as-yet-unenabled .set code uses is correct and the right thing to do. And if I weren't leaving for four months now, I would absolutely favorably review a patch along those lines. If someone else were to pick up the review in the meantime, however, I could understand if they reached a different conclusion -- but I'm not sure *how* they could have a patch that did realize such alternate conclusion.
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 11•6 years ago
|
||
vm/SelfHosting.cpp - Added missing null-pointer check after CheckedUnwrap call (this shouldn't happen, but is probably similar to the case in the next if-statement in DangerouslyUnwrapTypedArray, so I think it makes sense to add this check). - The intrinsic_TypedArrayBitwiseSlice function is based on the existing (not yet enabled) .set code, except for the same-buffer check, which is necessary for spec-compliance, see the slice-memcpy.js test case. Yay! :-/ tests/ecma_6/TypedArray/slice-bitwise.js - Tests to ensure bitwise copying is performed (only observable with NaN values). tests/ecma_6/TypedArray/slice-conversion.js - Tests to ensure memcpy'ing between different (compatible) typed array element types works correctly. tests/ecma_6/TypedArray/slice-memcpy.js - And tests for when we can't use memcpy (or memmove).
Attachment #8806014 -
Attachment is obsolete: true
Attachment #8926501 -
Flags: review?(jwalden+bmo)
Comment 12•6 years ago
|
||
Comment on attachment 8926501 [details] [diff] [review] bug1140152.patch Review of attachment 8926501 [details] [diff] [review]: ----------------------------------------------------------------- r=me, this all looks very reasonable. ::: js/src/vm/SelfHosting.cpp @@ +1240,5 @@ > JSObject* unwrapped = CheckedUnwrap(obj); > + if (!unwrapped) { > + ReportAccessDenied(cx); > + return nullptr; > + } Nit: add empty line after block. (Also, nice catch!) @@ +1623,5 @@ > + MOZ_ASSERT(args[2].isInt32()); > + MOZ_ASSERT(args[3].isInt32()); > + > + Rooted<TypedArrayObject*> source(cx, &args[0].toObject().as<TypedArrayObject>()); > + MOZ_ASSERT(!source->hasDetachedBuffer(), "source is not detached"); Nit: you don't need to add comments to asserts (outside of self-hosted code). In cases like this, where the comment really only restates exactly what the code says, I personally think it's better to omit it. Entirely fine to land with this and the other comments below, but also feel free to remove them.
Attachment #8926501 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 13•6 years ago
|
||
Thanks for stealing the review! :-) Updated patch to apply cleanly on inbound and to apply review comments. Carrying r+.
Attachment #8926501 -
Attachment is obsolete: true
Attachment #8972666 -
Flags: review+
Assignee | ||
Comment 14•6 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f58244316f5cec99d0edfdba74934b2524ccb739
Keywords: checkin-needed
Comment 15•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd12b33bb11 Copy elements' underlying bit patterns in TypedArray.prototype.slice. r=till
Keywords: checkin-needed
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ecd12b33bb11
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•