Closed Bug 1140152 Opened 5 years ago Closed 2 years ago

%TypedArray%.prototype.slice must copy elements' underlying bit patterns

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

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)

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
So do we have this intrinsic already?
Assignee: nobody → dirkjan
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.
Prerequisite for the patch, actually part of bug 1122396.
Assignee: dirkjan → andrebargull
Status: NEW → ASSIGNED
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)
No longer blocks: es6
Depends on: 1122396
Duplicate of this bug: 1271646
Attachment #8774854 - Attachment is obsolete: true
Attachment #8774858 - Attachment is obsolete: true
Attachment #8774858 - Flags: feedback?(jwalden+bmo)
Attached patch bug1140152.patch (obsolete) — Splinter Review
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)
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)
Blocks: test262
Flags: needinfo?(jwalden+bmo)
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!)
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)
Attached patch bug1140152.patch (obsolete) — Splinter Review
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)
Blocks: 1457256
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+
Attached patch bug1140152.patchSplinter Review
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+
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
https://hg.mozilla.org/mozilla-central/rev/ecd12b33bb11
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Duplicate of this bug: 1457256
You need to log in before you can comment on or make changes to this bug.