Last Comment Bug 718128 - Implement ArrayBuffer.slice
: Implement ArrayBuffer.slice
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
http://www.khronos.org/registry/typed...
Depends on: 723728
Blocks: webgl-conformance
  Show dependency treegraph
 
Reported: 2012-01-13 18:12 PST by David Mandelin [:dmandelin]
Modified: 2014-09-02 14:29 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (12.98 KB, patch)
2012-01-13 18:14 PST, David Mandelin [:dmandelin]
jwalden+bmo: review-
Details | Diff | Splinter Review
Patch v2 (10.08 KB, patch)
2012-01-18 18:23 PST, David Mandelin [:dmandelin]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description David Mandelin [:dmandelin] 2012-01-13 18:12:30 PST
Per the linked URL and the test cases in

  http://trac.webkit.org/browser/trunk/LayoutTests/fast/canvas/webgl/array-unit-tests.html
Comment 1 David Mandelin [:dmandelin] 2012-01-13 18:14:02 PST
Created attachment 588580 [details] [diff] [review]
Patch
Comment 2 David Mandelin [:dmandelin] 2012-01-13 18:14:57 PST
Btw, the test case is derived from the WebKit unit tests. Does that need some kind of copyright notice?
Comment 3 Kenneth Russell 2012-01-13 18:18:20 PST
(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 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-18 16:30:59 PST
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?
Comment 5 David Mandelin [:dmandelin] 2012-01-18 18:22:56 PST
(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.
Comment 6 David Mandelin [:dmandelin] 2012-01-18 18:23:56 PST
Created attachment 589748 [details] [diff] [review]
Patch v2
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-20 16:04:37 PST
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.
Comment 8 David Mandelin [:dmandelin] 2012-01-20 17:27:45 PST
(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.
Comment 9 David Mandelin [:dmandelin] 2012-01-20 17:27:59 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/374975f24277
Comment 10 Ed Morley [:emorley] 2012-01-22 12:34:14 PST
https://hg.mozilla.org/mozilla-central/rev/374975f24277
Comment 11 Florian Scholz [:fscholz] (MDN) 2014-09-02 14:29:58 PDT
For the record, this should have been documented. Added now.

Firefox 12 for developers
https://developer.mozilla.org/en-US/Firefox/Releases/12#JavaScript

Reference page
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/slice

Note You need to log in before you can comment on or make changes to this bug.