First steps towards implementing ArrayBuffer &co in V8

First steps towards implementing ArrayBuffer &co in V8


BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14091

[Dmitry Lomov (no reviews), 2013-03-25 18:00:39]

First steps towards V8 implementation of ArrayBuffers. 
Just an ArrayBuffer constructor + byteLength property.
(under flag and with weird name __ArrayBuffer for now)

[Dmitry Lomov (no reviews), 2013-03-26 15:59:34]

More tests and implementation of ArrayBuffer.slice

[rossberg, 2013-03-27 17:33:14]

https://codereview.chromium.org/13064003/diff/11001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/13064003/diff/11001/src/objects-printer.cc#ne...
src/objects-printer.cc:804: PrintF(out, " - byte_length =");
Space after =

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode830
src/runtime.cc:830: }
Nit: indentation is off

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode846
src/runtime.cc:846: if (Smi::IsValid(allocated_length)) {
You can use isolate->heap()->NumberFromDouble here.

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode876
src/runtime.cc:876: CONVERT_ARG_HANDLE_CHECKED(JSArrayBuffer, result, 1);
Nit: I wouldn't call this 'result'. How about 'target'? (And 'source' instead of
'holder'.)

https://codereview.chromium.org/13064003/diff/11001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/13064003/diff/11001/src/runtime.h#newcode347
src/runtime.h:347: F(ArrayBufferSliceImpl, 3, 1) \
Nit: empty line after the section.

https://codereview.chromium.org/13064003/diff/11001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13064003/diff/11001/src/typedarray.js#newcode63
src/typedarray.js:63: var relativeEnd = IS_UNDEFINED(end) ? this.byteLength :
end;
Don't you have to convert 'end' to integer?

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:40: TestByteLength(-2.567, 0);
Can we afford to include a TestByteLength(l, l) with l > 2^32 ?

Also, TestByteLength(0, 0).

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:59: function CheckSlice(expectedResultLen,
initialLen, start, end) {
Nit: TestSlice, for consistency.

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:87: TestArrayBufferSlice();
Maybe add a sanity check for calling slice with weird or missing arguments.

[Dmitry Lomov (no reviews), 2013-03-27 18:45:00]

PTAL

https://codereview.chromium.org/13064003/diff/11001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/13064003/diff/11001/src/objects-printer.cc#ne...
src/objects-printer.cc:804: PrintF(out, " - byte_length =");
On 2013/03/27 17:33:14, rossberg wrote:
> Space after =

Done.

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode830
src/runtime.cc:830: }
On 2013/03/27 17:33:14, rossberg wrote:
> Nit: indentation is off

Done.

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode846
src/runtime.cc:846: if (Smi::IsValid(allocated_length)) {
On 2013/03/27 17:33:14, rossberg wrote:
> You can use isolate->heap()->NumberFromDouble here.

Done.

https://codereview.chromium.org/13064003/diff/11001/src/runtime.cc#newcode876
src/runtime.cc:876: CONVERT_ARG_HANDLE_CHECKED(JSArrayBuffer, result, 1);
On 2013/03/27 17:33:14, rossberg wrote:
> Nit: I wouldn't call this 'result'. How about 'target'? (And 'source' instead
of
> 'holder'.)

Done.

https://codereview.chromium.org/13064003/diff/11001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/13064003/diff/11001/src/runtime.h#newcode347
src/runtime.h:347: F(ArrayBufferSliceImpl, 3, 1) \
On 2013/03/27 17:33:14, rossberg wrote:
> Nit: empty line after the section.

Done.

https://codereview.chromium.org/13064003/diff/11001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13064003/diff/11001/src/typedarray.js#newcode63
src/typedarray.js:63: var relativeEnd = IS_UNDEFINED(end) ? this.byteLength :
end;
On 2013/03/27 17:33:14, rossberg wrote:
> Don't you have to convert 'end' to integer?
Good catch, done and added a test

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:40: TestByteLength(-2.567, 0);
On 2013/03/27 17:33:14, rossberg wrote:
> Can we afford to include a TestByteLength(l, l) with l > 2^32 ?

Hmm, I changed what we throw from OutOfMemory (uncatchable) to a RangeError.
Spec does not say anything here, I'd perfer a RangeError I guess
> 
> Also, TestByteLength(0, 0).

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:59: function CheckSlice(expectedResultLen,
initialLen, start, end) {
On 2013/03/27 17:33:14, rossberg wrote:
> Nit: TestSlice, for consistency.

Done.

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:87: TestArrayBufferSlice();
On 2013/03/27 17:33:14, rossberg wrote:
> Maybe add a sanity check for calling slice with weird or missing arguments.

Done.

[rossberg, 2013-03-28 09:45:15]

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
File test/mjsunit/harmony/typedarrays.js (right):

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:40: TestByteLength(-2.567, 0);
On 2013/03/27 18:45:00, Dmitry Lomov (chromium) wrote:
> On 2013/03/27 17:33:14, rossberg wrote:
> > Can we afford to include a TestByteLength(l, l) with l > 2^32 ?
> 
> Hmm, I changed what we throw from OutOfMemory (uncatchable) to a RangeError.
> Spec does not say anything here, I'd perfer a RangeError I guess

Sorry, I don't like it. RangeError is for domain violations as prescribed by the
spec, which this condition isn't. I think OOM is far more adequate.

> > Also, TestByteLength(0, 0).

You seem to have overlooked that part. ;)

https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
test/mjsunit/harmony/typedarrays.js:87: TestArrayBufferSlice();
On 2013/03/27 18:45:00, Dmitry Lomov (chromium) wrote:
> On 2013/03/27 17:33:14, rossberg wrote:
> > Maybe add a sanity check for calling slice with weird or missing arguments.
> 
> Done.

I especially thought of calls with arguments of bogus types, to test the
conversions.

[Dmitry Lomov (no reviews), 2013-03-28 12:28:22]

On 2013/03/28 09:45:15, rossberg wrote:
>
https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
> File test/mjsunit/harmony/typedarrays.js (right):
> 
>
https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
> test/mjsunit/harmony/typedarrays.js:40: TestByteLength(-2.567, 0);
> On 2013/03/27 18:45:00, Dmitry Lomov (chromium) wrote:
> > On 2013/03/27 17:33:14, rossberg wrote:
> > > Can we afford to include a TestByteLength(l, l) with l > 2^32 ?
> > 
> > Hmm, I changed what we throw from OutOfMemory (uncatchable) to a RangeError.
> > Spec does not say anything here, I'd perfer a RangeError I guess
> 
> Sorry, I don't like it. RangeError is for domain violations as prescribed by
the
> spec, which this condition isn't. I think OOM is far more adequate.

As I said, OOM is not catchable by JavaScript, so I'd prefer a catch-able
exception here. and adding a new exception kind seems overkill. RangeError feels
like the most adequate exception here. We can also raise this at TC-39
> 
> > > Also, TestByteLength(0, 0).
> 
> You seem to have overlooked that part. ;)

Oops, I am sorry, new tests somehow failed to upload in the patch. Doing it now.
> 
>
https://codereview.chromium.org/13064003/diff/11001/test/mjsunit/harmony/type...
> test/mjsunit/harmony/typedarrays.js:87: TestArrayBufferSlice();
> On 2013/03/27 18:45:00, Dmitry Lomov (chromium) wrote:
> > On 2013/03/27 17:33:14, rossberg wrote:
> > > Maybe add a sanity check for calling slice with weird or missing
arguments.
> > 
> > Done.
> 
> I especially thought of calls with arguments of bogus types, to test the
> conversions.

[Dmitry Lomov (no reviews), 2013-03-28 12:29:11]

PTAL (added forgotten tests)

[rossberg, 2013-03-28 12:32:51]

LGTM

On 2013/03/28 12:28:22, Dmitry Lomov (chromium) wrote:
> As I said, OOM is not catchable by JavaScript, so I'd prefer a catch-able
> exception here. and adding a new exception kind seems overkill. RangeError
feels
> like the most adequate exception here. We can also raise this at TC-39

Yeah, as discussed off-line, I agree.

[Dmitry Lomov (no reviews), 2013-03-28 12:50:35]

Committed patchset #8 manually as r14091 (presubmit successful).