Implement ArrayBuffer.slice

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

({dev-doc-complete})

Trunk
mozilla12
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Comment 1

8 years ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #588580 - Flags: review?(jwalden+bmo)
Assignee

Comment 2

8 years ago
Btw, the test case is derived from the WebKit unit tests. Does that need some kind of copyright notice?

Comment 3

8 years ago
(In reply to David Mandelin from comment #2)
> Btw, the test case is derived from the WebKit unit tests. Does that need
> some kind of copyright notice?

If you'd like you could replicate the copyright notice from https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/sdk/tests/conformance/typedarrays/array-unit-tests.html -- as it happens the ArrayBuffer.slice() unit tests were added by one of the Chromium authors and will be incorporated into the Khronos version. However, the WebKit layout tests deliberately don't have any copyright notices, so I wouldn't worry about it.
Comment on attachment 588580 [details] [diff] [review]
Patch

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

r- mostly for the confusing overshare here, I think.

::: js/src/jstypedarray.cpp
@@ +107,5 @@
>      return false;
>  }
>  
>  /*
> + * Shared subsequence logic used by ArrayBuffer slice and TypedArray subarray.

I find it much more confusing to share logic like this between two unrelated methods, than to not share it.  Sharing like this makes it much harder to determine algorithmic correctness, because you can't simply match up algorithm steps to the code one-to-one.  I'd like the implementations of these two methods to be separate.  I'd also extend this to the index-parsing bits, too, myself.

@@ +130,5 @@
> +    int32_t length = int32_t(getLen(obj));
> +    int32_t begin = 0, end = length;
> +
> +    if (args.length() > 0) {
> +        if (!NonstandardToInt32(cx, args[0], &begin))

This should be ToInt32, same again later.

More generally, however, why are slice's arguments not unsigned long = uint32_t?  The current signature seems to indicate that there's no way to make a slice directly including elements of an ArrayBuffer whose index exceeds 2**31 - 1.  (With enough careful pre-querying, you could use negative indexes as a hackaround.  That's pretty much unbelievably ugly, I'd say.)  We should make the change in our own code, and we should get the change made upstream in the spec.  We can enforce whatever length requirements we want (like permitting arrays only up to UINT32_MAX / 2 length) on our own terms, without kind of relying on the specs for justification.

@@ +249,1 @@
>      getElementsHeader()->length = size;

If |getElementsHeader()| is identical to |newheader| in the first case of the |if| above, let's add |ObjectElements *header;| above the if, make the |newheader| init be of |header| instead, and add |header = getElementsHeader();| in the |else|.  Then you can remove all the |getElementsHeader()| calls here and in the rest of the method, and use |header| instead.  This manual CSEing helps readability in my opinion, because then you're not relying on |getElementsHeader()| being "small enough to inline", or anything like that.

@@ +312,5 @@
> +JSObject *
> +ArrayBuffer::createSlice(JSContext *cx, JSObject *arrayBuffer, uint32_t begin, uint32_t end)
> +{
> +    JS_ASSERT(arrayBuffer->isArrayBuffer());
> +    JS_ASSERT(0 <= begin);

This assertion is vacuous -- |begin| is unsigned.  It'll even cause compiler warnings in compilers that warn about vacuous conditions.  Please remove it.

@@ +314,5 @@
> +{
> +    JS_ASSERT(arrayBuffer->isArrayBuffer());
> +    JS_ASSERT(0 <= begin);
> +    JS_ASSERT(begin <= arrayBuffer->arrayBufferByteLength());
> +    JS_ASSERT(0 <= end);

Remove this one, too.

@@ +2215,5 @@
>  };
>  
> +JSFunctionSpec ArrayBuffer::jsfuncs[] = {                                     \
> +    JS_FN("slice", ArrayBuffer::fun_slice, 2, JSFUN_GENERIC_NATIVE),          \
> +    JS_FS_END                                                                 \

Erm, why the \ at the end of all these lines?  Remove 'em?
Attachment #588580 - Flags: review?(jwalden+bmo) → review-
Assignee

Comment 5

7 years ago
(In reply to Jeff Walden (remove +bmo to email) from comment #4)
> Comment on attachment 588580 [details] [diff] [review]
> Patch
> 
> Review of attachment 588580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- mostly for the confusing overshare here, I think.
> 
> ::: js/src/jstypedarray.cpp
> @@ +107,5 @@
> >      return false;
> >  }
> >  
> >  /*
> > + * Shared subsequence logic used by ArrayBuffer slice and TypedArray subarray.
> 
> I find it much more confusing to share logic like this between two unrelated
> methods, than to not share it.  Sharing like this makes it much harder to
> determine algorithmic correctness, because you can't simply match up
> algorithm steps to the code one-to-one.  I'd like the implementations of
> these two methods to be separate.  I'd also extend this to the index-parsing
> bits, too, myself.

I would prefer to keep the index munging shared, but as it so happens, I had one patch that does the sharing, so I just popped that off, so you get exactly what you asked for. :-) 

> @@ +130,5 @@
> > +    int32_t length = int32_t(getLen(obj));
> > +    int32_t begin = 0, end = length;
> > +
> > +    if (args.length() > 0) {
> > +        if (!NonstandardToInt32(cx, args[0], &begin))
> 
> This should be ToInt32, same again later.

That's really bug 708377, but I changed it.

> More generally, however, why are slice's arguments not unsigned long =
> uint32_t?  The current signature seems to indicate that there's no way to
> make a slice directly including elements of an ArrayBuffer whose index
> exceeds 2**31 - 1.  (With enough careful pre-querying, you could use
> negative indexes as a hackaround.  That's pretty much unbelievably ugly, I'd
> say.)  We should make the change in our own code, and we should get the
> change made upstream in the spec.  We can enforce whatever length
> requirements we want (like permitting arrays only up to UINT32_MAX / 2
> length) on our own terms, without kind of relying on the specs for
> justification.

I just want to get it working per spec for now. You can talk to Dave Herman about the spec later.

> @@ +249,1 @@
> >      getElementsHeader()->length = size;
> 
> If |getElementsHeader()| is identical to |newheader| in the first case of
> the |if| above, let's add |ObjectElements *header;| above the if, make the
> |newheader| init be of |header| instead, and add |header =
> getElementsHeader();| in the |else|.  Then you can remove all the
> |getElementsHeader()| calls here and in the rest of the method, and use
> |header| instead.  This manual CSEing helps readability in my opinion,
> because then you're not relying on |getElementsHeader()| being "small enough
> to inline", or anything like that.

I just did the manual CSE--you can get it to work with the complete change you mention, but it's very sensitive to the exact order in which you call getElementsHeader()--if you call it at the end then it just works.

> @@ +312,5 @@
> > +JSObject *
> > +ArrayBuffer::createSlice(JSContext *cx, JSObject *arrayBuffer, uint32_t begin, uint32_t end)
> > +{
> > +    JS_ASSERT(arrayBuffer->isArrayBuffer());
> > +    JS_ASSERT(0 <= begin);
> 
> This assertion is vacuous -- |begin| is unsigned.  It'll even cause compiler
> warnings in compilers that warn about vacuous conditions.  Please remove it.
> 
> @@ +314,5 @@
> > +{
> > +    JS_ASSERT(arrayBuffer->isArrayBuffer());
> > +    JS_ASSERT(0 <= begin);
> > +    JS_ASSERT(begin <= arrayBuffer->arrayBufferByteLength());
> > +    JS_ASSERT(0 <= end);
> 
> Remove this one, too.
> 
> @@ +2215,5 @@
> >  };
> >  
> > +JSFunctionSpec ArrayBuffer::jsfuncs[] = {                                     \
> > +    JS_FN("slice", ArrayBuffer::fun_slice, 2, JSFUN_GENERIC_NATIVE),          \
> > +    JS_FS_END                                                                 \
> 
> Erm, why the \ at the end of all these lines?  Remove 'em?

Done.
Assignee

Comment 6

7 years ago
Posted patch Patch v2Splinter Review
Attachment #588580 - Attachment is obsolete: true
Attachment #589748 - Flags: review?(jwalden+bmo)
Comment on attachment 589748 [details] [diff] [review]
Patch v2

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

::: js/src/jit-test/tests/basic/testArrayBufferSlice.js
@@ +1,4 @@
> +function testSlice() {
> +    function test(subBuf, starts, size) {
> +        byteLength = size;
> +        subBuffer = eval(subBuf);

var

Perhaps we should get in the habit of writing tests in strict mode, unless there's reason not to use it, to prevent typos like this...

::: js/src/jstypedarray.cpp
@@ +166,5 @@
> +            if (begin < 0)
> +                begin = 0;
> +        } else if (begin > length) {
> +            begin = length;
> +        }

Hmm.  I disliked commoning out bits that depend on argument ordering and indexing and all that, to the extent that was done in the previous version of the patch.  But abstracting out specifically "convert this to a clamped index" doesn't sound bad.  Something like |bool ToClampedIndex(JSContext *cx, const Value &v, int32_t length, int32_t *out)| maybe?  (Just make sure it has a sufficiently detailed description accompanying it -- the exact mechanism's complicated.)

@@ +231,3 @@
>      }
> +    if (contents)
> +        memcpy(elements, contents, size);

I think cdleary just added PodCopy for this purpose, with the added benefit that it asserts non-overlappingness of its arguments.

@@ +231,5 @@
>      }
> +    if (contents)
> +        memcpy(elements, contents, size);
> +    else
> +        memset(elements, 0, size);

PodZero for this.
Attachment #589748 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 8

7 years ago
(In reply to Jeff Walden (remove +bmo to email) from comment #7)
> Comment on attachment 589748 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 589748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit-test/tests/basic/testArrayBufferSlice.js
> @@ +1,4 @@
> > +function testSlice() {
> > +    function test(subBuf, starts, size) {
> > +        byteLength = size;
> > +        subBuffer = eval(subBuf);
> 
> var
> 
> Perhaps we should get in the habit of writing tests in strict mode, unless
> there's reason not to use it, to prevent typos like this...
> 
> ::: js/src/jstypedarray.cpp
> @@ +166,5 @@
> > +            if (begin < 0)
> > +                begin = 0;
> > +        } else if (begin > length) {
> > +            begin = length;
> > +        }
> 
> Hmm.  I disliked commoning out bits that depend on argument ordering and
> indexing and all that, to the extent that was done in the previous version
> of the patch.  But abstracting out specifically "convert this to a clamped
> index" doesn't sound bad.  Something like |bool ToClampedIndex(JSContext
> *cx, const Value &v, int32_t length, int32_t *out)| maybe?  (Just make sure
> it has a sufficiently detailed description accompanying it -- the exact
> mechanism's complicated.)

Done and done.

> @@ +231,3 @@
> >      }
> > +    if (contents)
> > +        memcpy(elements, contents, size);
> 
> I think cdleary just added PodCopy for this purpose, with the added benefit
> that it asserts non-overlappingness of its arguments.
> 
> @@ +231,5 @@
> >      }
> > +    if (contents)
> > +        memcpy(elements, contents, size);
> > +    else
> > +        memset(elements, 0, size);
> 
> PodZero for this.

I didn't make those changes because I think it's not the right thing to do here--it makes the semantics depend on the type of |elements|, when what we really want to do is copy or zero |size| bytes.
https://hg.mozilla.org/mozilla-central/rev/374975f24277
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 723728
You need to log in before you can comment on or make changes to this bug.