First cut at impementing ES6 TypedArrays in V8.

First cut at impementing ES6 TypedArrays in V8.



BUG=

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

[Dmitry Lomov (no reviews), 2013-04-12 16:46:35]

Please take a look.

[rossberg, 2013-04-15 13:11:03]

https://codereview.chromium.org/13975012/diff/2001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/13975012/diff/2001/src/objects-debug.cc#newco...
src/objects-debug.cc:738: || byte_offset()->IsUndefined());
Too bad that we cannot rule out undefined here...

https://codereview.chromium.org/13975012/diff/2001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/13975012/diff/2001/src/objects.h#newcode8539
src/objects.h:8539: // [length]: lenght of typed array in elements.
Typo: s/lenght/length/

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode641
src/runtime.cc:641: static size_t NumberToSize(Isolate* isolate,
This should probably move to v8conversions.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode775
src/runtime.cc:775: case 1:
Can we define an enum for that?

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode816
src/runtime.cc:816: ASSERT(false);
You can use UNREACHABLE() here. Also, the NULL return seems redundant.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode827
src/runtime.cc:827: ASSERT(length * elementSize == byte_length);
Given this invariant, why not drop the byte_length argument from the runtime
function?

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode834
src/runtime.cc:834: holder, elementsKind);
Nit: should fit on the previous line

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode839
src/runtime.cc:839: 
Style: add extra newline

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode854
src/runtime.cc:854: 
Style: add another newline

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode87
src/typedarray.js:87: function CreateTypedArrayConstructor(name, elementSize,
arrayId, proto) {
The 'proto' argument is misnamed: it's not the prototype, it's the constructor.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode94
src/typedarray.js:94: if (IS_UNDEFINED(byteOffset)) {
You can use ?: here

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode124
src/typedarray.js:124: %TypedArrayInitialize(this, arrayId, buffer, offset,
newLength, newByteLength);
Line too long

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode150
src/typedarray.js:150: [{name:"Uint8Array",
Style: put in a space after ':'

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode151
src/typedarray.js:151: protoObject:global.__Uint8Array,
s/protoObject/constructor/g

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode208
src/typedarray.js:208: for (var i = 0; i < typedArrayDescs.length; i++) {
Hm, I'd prefer separating initialization of thyped arrays (i.e., the body of
this loop) into a function SetUpTypedArray and then making calls to that with
the proper arguments, instead of the indirection through the descriptor array.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode221
src/typedarray.js:221: InstallGetter(desc.protoObject.prototype, "byteOffset",
TypedArrayGetByteOffset);
Line too long (and below)

[Dmitry Lomov (no reviews), 2013-04-15 15:40:58]

Thanks for review! 
Please take another look.

https://codereview.chromium.org/13975012/diff/2001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/13975012/diff/2001/src/objects.h#newcode8539
src/objects.h:8539: // [length]: lenght of typed array in elements.
On 2013/04/15 13:11:03, rossberg wrote:
> Typo: s/lenght/length/

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode641
src/runtime.cc:641: static size_t NumberToSize(Isolate* isolate,
On 2013/04/15 13:11:03, rossberg wrote:
> This should probably move to v8conversions.

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode775
src/runtime.cc:775: case 1:
On 2013/04/15 13:11:03, rossberg wrote:
> Can we define an enum for that?

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode816
src/runtime.cc:816: ASSERT(false);
On 2013/04/15 13:11:03, rossberg wrote:
> You can use UNREACHABLE() here. Also, the NULL return seems redundant.

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode827
src/runtime.cc:827: ASSERT(length * elementSize == byte_length);
On 2013/04/15 13:11:03, rossberg wrote:
> Given this invariant, why not drop the byte_length argument from the runtime
> function?

Done. The invariant helped me find some bugs in preparation of the patch :)

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode834
src/runtime.cc:834: holder, elementsKind);
On 2013/04/15 13:11:03, rossberg wrote:
> Nit: should fit on the previous line

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode839
src/runtime.cc:839: 
On 2013/04/15 13:11:03, rossberg wrote:
> Style: add extra newline

Done.

https://codereview.chromium.org/13975012/diff/2001/src/runtime.cc#newcode854
src/runtime.cc:854: 
On 2013/04/15 13:11:03, rossberg wrote:
> Style: add another newline

Done.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode87
src/typedarray.js:87: function CreateTypedArrayConstructor(name, elementSize,
arrayId, proto) {
On 2013/04/15 13:11:03, rossberg wrote:
> The 'proto' argument is misnamed: it's not the prototype, it's the
constructor.

Done.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode94
src/typedarray.js:94: if (IS_UNDEFINED(byteOffset)) {
On 2013/04/15 13:11:03, rossberg wrote:
> You can use ?: here

Done.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode124
src/typedarray.js:124: %TypedArrayInitialize(this, arrayId, buffer, offset,
newLength, newByteLength);
On 2013/04/15 13:11:03, rossberg wrote:
> Line too long

Done.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode208
src/typedarray.js:208: for (var i = 0; i < typedArrayDescs.length; i++) {
On 2013/04/15 13:11:03, rossberg wrote:
> Hm, I'd prefer separating initialization of thyped arrays (i.e., the body of
> this loop) into a function SetUpTypedArray and then making calls to that with
> the proper arguments, instead of the indirection through the descriptor array.

Done.

https://codereview.chromium.org/13975012/diff/2001/src/typedarray.js#newcode221
src/typedarray.js:221: InstallGetter(desc.protoObject.prototype, "byteOffset",
TypedArrayGetByteOffset);
On 2013/04/15 13:11:03, rossberg wrote:
> Line too long (and below)

Done.

[rossberg, 2013-04-16 13:30:43]

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc#newcode824
src/runtime.cc:824: ASSERT(byte_length % elementSize == 0);
I'm surprised that you dropped length instead of byte_length. Wouldn't it be
simpler the other way round? Then you don't need this extra invariant.

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc#newcode828
src/runtime.cc:828: {
You can use factory->NewNumber for this.

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js#newcode87
src/typedarray.js:87: function CreateTypedArrayConstructor(name, elementSize,
arrayId, constr) {
Nit: spell out constructor, it still fits the line. :)

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js#newcode183
src/typedarray.js:183: function SetupAllTypedArrays() {
Nit: I'd remove this wrapper.

[Dmitry Lomov (no reviews), 2013-04-16 13:47:13]

Comments addressed (except one)

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc#newcode824
src/runtime.cc:824: ASSERT(byte_length % elementSize == 0);
On 2013/04/16 13:30:43, rossberg wrote:
> I'm surprised that you dropped length instead of byte_length. Wouldn't it be
> simpler the other way round? Then you don't need this extra invariant.

More rigid invariant is better, no? This helps catching bugs early

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc#newcode828
src/runtime.cc:828: {
On 2013/04/16 13:30:43, rossberg wrote:
> You can use factory->NewNumber for this.

Done.

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js
File src/typedarray.js (right):

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js#newcode87
src/typedarray.js:87: function CreateTypedArrayConstructor(name, elementSize,
arrayId, constr) {
On 2013/04/16 13:30:43, rossberg wrote:
> Nit: spell out constructor, it still fits the line. :)

Done.

https://codereview.chromium.org/13975012/diff/9002/src/typedarray.js#newcode183
src/typedarray.js:183: function SetupAllTypedArrays() {
Done. No pereference either way.
On 2013/04/16 13:30:43, rossberg wrote:
> Nit: I'd remove this wrapper.

[rossberg, 2013-04-16 13:59:48]

LGTM

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/13975012/diff/9002/src/runtime.cc#newcode824
src/runtime.cc:824: ASSERT(byte_length % elementSize == 0);
On 2013/04/16 13:47:13, Dmitry Lomov (chromium) wrote:
> On 2013/04/16 13:30:43, rossberg wrote:
> > I'm surprised that you dropped length instead of byte_length. Wouldn't it be
> > simpler the other way round? Then you don't need this extra invariant.
> 
> More rigid invariant is better, no? This helps catching bugs early
> 

No, the simpler the invariant/precondition of a function the better. What you
probably mean is that more rigid _checking_ of an invariant/precondition _that
you actually need_ is better. But if it's factored such that there is nothing
left to check in the first place, even better.

That is, I still think the right place to check the invariant between length and
byte_length is where you originally compute them. This function should not need
to be concerned with that.

But I don't care enough, so LGTM... :)

[Dmitry Lomov (no reviews), 2013-04-16 14:16:46]

Committed patchset #5 manually as r14285 (presubmit successful).