Closed Bug 1139759 Opened 9 years ago Closed 9 years ago

Self-host %TypedArray%.prototype.copyWithin

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

Attachments

(2 files)

At one time I thought this would be good for security, in terms of letting us just use JS to implement these things.  But with copyWithin's requirement to copy actual bit patterns, that's no longer possible (or at least it's not possible for Float32Array/Float64Array).  (Sure, we could get the ArrayBuffer, wrap it in an integral view, then act on that.  But that's gotta be slower than just having an intrinsic, in terms of garbage and so on.)

This reduces the non-selfhostable part down to a very tiny kernel to perform that one operation, and while it's doing scary things, it and its inputs seem small enough to examine and determine correctness fairly easily.
Attached patch PatchSplinter Review
Going with the theme of all the other methods, no detachment checks in the inlined ValidateTypedArray.  Worth adding, but seems simplest to do that to everything all at once, rather than rathole on it here.  If I have time at the end of the quarter, maybe I can return and deal with that.
Attachment #8573037 - Flags: review?(till)
Attached file Micro(ish)benchmark
Running this locally with commands like |for i in `seq 1 20`; do opt/js/src/js -f /tmp/cw.js; done| pre-patch and post-patch gives noisy figures as always, but the general trend seems indicate this patch slightly improves copyWithin performance, or at least doesn't regress it.  Good enough.
Oh, and regarding detachment, I referred to "detachment" in that comment, to be consistent with similar comments.  But because the engine still refers to it everywhere else as neutering, I stuck with that for the other comments.  Again, maybe end of quarter if I have time I can do the s/neuter/detach/g patching.  Not going to sidetrack on that now or here, tho.
Comment on attachment 8573037 [details] [diff] [review]
Patch

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

It's almost like an `unsafe` block in Rust :)

r=me with nits addressed.

::: js/src/builtin/TypedArray.js
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// ES6 draft 20150220 %TypedArray%.prototype.copyWithin

nit: a new draft was released, so might as well update to that.

@@ +17,5 @@
> +
> +    // Steps 3-4, modified for the typed array case.
> +    var len = TypedArrayLength(obj);
> +
> +    assert(0 <= len && len <= 2147483647,

I think I'd slightly prefer hex literals here and below, as those make it much more immediately apparent what's being tested here. To me, at least, YMMV.

::: js/src/jit-test/tests/basic/typed-array-copyWithin.js
@@ +172,5 @@
>        assertEq(e, 42, "should have failed converting target to index");
>      }
>  
> +    function neuterAndConvertTo(x)
> +    {

Nit: I see that there are some pre-existing cases of this style in this file, plus please don't continue that. r=me on fixing up the existing occurrences, but up to you.

::: js/src/vm/SelfHosting.cpp
@@ +960,4 @@
>      JS_FN("IsTypedArray",            intrinsic_IsTypedArray,            1,0),
>      JS_FN("TypedArrayLength",        intrinsic_TypedArrayLength,        1,0),
>  
> +    JS_FN("MoveTypedArrayElements",  intrinsic_MoveTypedArrayElements,  4,0),

I guess at some point I should go through and fix all the intrinsics' names to start with an underscore. Up to you whether you want to follow that convention (which, technically, we have) now or not.

::: js/src/vm/TypedArrayObject.cpp
@@ -772,4 @@
>  };
>  
>  /* static */ bool
> -TypedArrayObject::copyWithin(JSContext *cx, unsigned argc, Value *vp)

Would it make sense to move TypedArrayMethods::copyWithin into SharedTypedArrayObject.cpp? If you think that'd be too onerous, please at least add a comment to it explaining that it's not actually used for TypedArray.
Attachment #8573037 - Flags: review?(till) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #0)
> At one time I thought this would be good for security, in terms of letting
> us just use JS to implement these things.  But with copyWithin's requirement
> to copy actual bit patterns, that's no longer possible (or at least it's not
> possible for Float32Array/Float64Array).  (Sure, we could get the
> ArrayBuffer, wrap it in an integral view, then act on that.  But that's
> gotta be slower than just having an intrinsic, in terms of garbage and so
> on.)

Another potential option might be to reuse an integral view for the float cases. Would obviously require an intrinsic, and I don't know how well it'd interact with TI, but might work.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e3da252b70

For what it's worth, the standard for tests is that style is anything goes -- no rules at all.  I've put a foot down when some particular style was actively misleading, a bare handful of times, but otherwise we have no enforced standards or conventions on test code.
https://hg.mozilla.org/mozilla-central/rev/c9e3da252b70
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1145058
No longer depends on: 1145058
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: