1234038 - Self-host ArrayBuffer.prototype.slice.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

derived from bug 1165053.

Before supporting @@species in ArrayBuffer.prototype.slice, it should be better to self-host it.

Comment 1

[Tooru Fujisawa [:arai]]

GetBuiltinConstructor("ArrayBuffer") is needed :)

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Self-host ArrayBuffer.prototype.slice.

Moved implementation to TypedArray.js, with 3 additional selfhost intrinsics:
  * ArrayBufferByteLength -- get [[ArrayBufferByteLength]] slot
  * ArrayBufferCopyData -- perform CopyDataBlockBytes-like things on arraybuffers
  * CallArrayBufferMethodIfWrapped -- almost same as CallTypedArrayMethodIfWrapped, but for ArrayBuffer

Also, added 3 messages related to species constructor (24.1.4.3 steps 13, 15, and 16 [1]), so, they won't be thrown for now.

Almost green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46afb6a71ba3

[1] https://tc39.github.io/ecma262/#sec-arraybuffer.prototype.slice

Comment 3

[Lars T Hansen [:lth]]

Comment on attachment 8700418 [details] [diff] [review]
Self-host ArrayBuffer.prototype.slice.

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

::: js/src/builtin/TypedArray.js
@@ +1169,5 @@
> +
> +// ES 2016 draft Dec 10, 2015 24.1.4.3.
> +function ArrayBufferSlice(start, end) {
> +    // Step 1.
> +    var O = this;

Out of curiosity - it doesn't matter here - why "var" instead of "let" or (better) "const", for these immutable bindings?

@@ +1189,5 @@
> +    // Step 6.
> +    var relativeStart = ToInteger(start);
> +
> +    // Step 7.
> +    var first = relativeStart < 0 ? std_Math_max(len + relativeStart, 0)

Actually step 8 because you skipped ReturnIfAbrupt().  Several similar errors below.

::: js/src/vm/SelfHosting.cpp
@@ +658,5 @@
> +    size_t byteLength = args[0].toObject().as<ArrayBufferObject>().byteLength();
> +    if (byteLength <= INT32_MAX)
> +        args.rval().setInt32(byteLength);
> +    else
> +        args.rval().setNumber((double)byteLength);

I believe the bytelength actually needs to be constrained to INT32_MAX for correctness.  Please check, and/or ask Waldo.  There is certainly such a constraint on TypedArray, and eg Int8Array would not make sense if the constraint did not apply to ArrayBuffer too.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Lars T Hansen [:lth] from comment #3)
> ::: js/src/vm/SelfHosting.cpp
> @@ +658,5 @@
> > +    size_t byteLength = args[0].toObject().as<ArrayBufferObject>().byteLength();
> > +    if (byteLength <= INT32_MAX)
> > +        args.rval().setInt32(byteLength);
> > +    else
> > +        args.rval().setNumber((double)byteLength);
> 
> I believe the bytelength actually needs to be constrained to INT32_MAX for
> correctness.  Please check, and/or ask Waldo.  There is certainly such a
> constraint on TypedArray, and eg Int8Array would not make sense if the
> constraint did not apply to ArrayBuffer too.

Logic to create ArrayBuffers of unconstrained length, verifies the byte length is <= INT32_MAX.  Ditto for creating typed arrays conceptually backed by ArrayBuffers whose byte lengths might transgress such a limit.  This partly enables storing length, byte length, and byte offset as int32_t.  There's also one additional, deeper reason for this that currently escapes my mind, that I'd find if I searched a little.  (I've thought about extending this restriction to uint32_t to make negatives impossible, but this extra reason I'm forgetting makes it tricky enough I haven't done it.)

In this case, the created ArrayBuffer isn't quite of unconstrained length, because it's slicing an existing ArrayBuffer, so its byte length must be <= the byte length of every other ArrayBuffer, ergo <= INT32_MAX already.  So just do

    args.rval().setInt32(mozilla::AssertedCast<int32_t>(byteLength));

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Lars T Hansen [:lth] from comment #3)
> Comment on attachment 8700418 [details] [diff] [review]
> Self-host ArrayBuffer.prototype.slice.
> 
> Review of attachment 8700418 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/TypedArray.js
> @@ +1169,5 @@
> > +
> > +// ES 2016 draft Dec 10, 2015 24.1.4.3.
> > +function ArrayBufferSlice(start, end) {
> > +    // Step 1.
> > +    var O = this;
> 
> Out of curiosity - it doesn't matter here - why "var" instead of "let" or
> (better) "const", for these immutable bindings?

Oh, I haven't thought of that.  I just followed that other self-hosted functions are using var.


> @@ +1189,5 @@
> > +    // Step 6.
> > +    var relativeStart = ToInteger(start);
> > +
> > +    // Step 7.
> > +    var first = relativeStart < 0 ? std_Math_max(len + relativeStart, 0)
> 
> Actually step 8 because you skipped ReturnIfAbrupt().  Several similar
> errors below.

I should've noted that I referred ES 2016 draft spec, it changes |ReturnIfAbrupt| with |?| notation, and step numbers are changed from ES6.

https://tc39.github.io/ecma262/#sec-arraybuffer.prototype.slice


> ::: js/src/vm/SelfHosting.cpp
> @@ +658,5 @@
> > +    size_t byteLength = args[0].toObject().as<ArrayBufferObject>().byteLength();
> > +    if (byteLength <= INT32_MAX)
> > +        args.rval().setInt32(byteLength);
> > +    else
> > +        args.rval().setNumber((double)byteLength);
> 
> I believe the bytelength actually needs to be constrained to INT32_MAX for
> correctness.  Please check, and/or ask Waldo.  There is certainly such a
> constraint on TypedArray, and eg Int8Array would not make sense if the
> constraint did not apply to ArrayBuffer too.

thank you lth and Waldo, I'll fix it :)

Comment 6

[Lars T Hansen [:lth]]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> Thank you for reviewing :)
> 
> (In reply to Lars T Hansen [:lth] from comment #3)
> > Comment on attachment 8700418 [details] [diff] [review]
> > Self-host ArrayBuffer.prototype.slice.
> > 
> > @@ +1189,5 @@
> > > +    // Step 6.
> > > +    var relativeStart = ToInteger(start);
> > > +
> > > +    // Step 7.
> > > +    var first = relativeStart < 0 ? std_Math_max(len + relativeStart, 0)
> > 
> > Actually step 8 because you skipped ReturnIfAbrupt().  Several similar
> > errors below.
> 
> I should've noted that I referred ES 2016 draft spec, it changes
> |ReturnIfAbrupt| with |?| notation, and step numbers are changed from ES6.
> 
> https://tc39.github.io/ecma262/#sec-arraybuffer.prototype.slice

Actually I now see that your patch has a comment to that effect above the function, I just did not notice it.  My fault.

IMO it would be better to stick to a currently ratified spec text instead of a future draft when implementing a function, whenever reasonable, ie, it would be better to reference ES2015 here.  But I don't know what the tradition is for that within the team; perhaps better to ask somebody with longer tenure if you want to address this remark (not required).

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed77e0bf3f711d794de5ca572f2230abef9ea497
Bug 1234038 - Self-host ArrayBuffer.prototype.slice. r=lth

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed77e0bf3f71