v8 typed arrays: add DataView type

v8 typed arrays: add DataView type

[bnoordhuis, 2013-05-23 22:59:04]

https://codereview.chromium.org/15943002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/15943002/diff/1/include/v8.h#newcode2451
include/v8.h:2451: uint8_t GetUint8(size_t byte_offset) const;
I'd be okay with removing these. I added them for the sake of convenience.

If you keep them, it might be worth it to inline them in v8.h - though that
might make unwrapping the JSDataView kind of icky.

https://codereview.chromium.org/15943002/diff/1/src/dataview.js
File src/dataview.js (right):

https://codereview.chromium.org/15943002/diff/1/src/dataview.js#newcode87
src/dataview.js:87: return %DataViewGetInt8(this, offset, false);
I've been trying to figure out if it's possible to generate inline code for this
but so far I've drawn up blank.  If you can point me in the right direction,
I'll happily follow up with a patch.

https://codereview.chromium.org/15943002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15943002/diff/1/src/runtime.cc#newcode802
src/runtime.cc:802: #define DATA_VIEW_GETTER(getter, accessor)                  
                 \
These could be merged / DRY'd with the code in src/api.cc.  I haven't done that
yet because, like I said in another comment, I'd be okay with dropping the API
functions.

https://codereview.chromium.org/15943002/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/15943002/diff/1/test/cctest/test-api.cc#newco...
test/cctest/test-api.cc:2668: CHECK_EQ(123.456, data_view->GetFloat64(4));
But if you decide to keep the API functions, there should probably be some
discussion on how to handle NaNs and other edge cases.  The DataView spec leaves
that remarkably wide open, the only requirement is that it's valid IEEE 754:

" When the not-a-number (NaN) value is stored into a Float32Array or
Float64Array, or into a DataView using the setFloat32 or setFloat64 methods, the
bit pattern written into the underlying ArrayBuffer is not specified, but shall
be one of the IEEE 754 bit patterns that represent NaN [IEEE-754].

When a bit pattern representing an IEEE 754 NaN is loaded from a Float32Array or
Float64Array, or from a DataView using the getFloat32 or getFloat64 methods, the
language binding (for example, ECMAScript) may use an alternative bit pattern to
represent the NaN value."

[Sven Panne, 2013-05-24 06:58:24]

Quick drive-by-comments...

https://codereview.chromium.org/15943002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6161
src/api.cc:6161: #define DATA_VIEW_GETTER(FunctionName, TypeName)               
              \
Don't duplicate tons of code here, just make swizzling a byte a no-op and do
things consistently. Furthermore, can't we use templates somehow instead of this
macro-horror?

https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6197
src/api.cc:6197: #define DATA_VIEW_SETTER(FunctionName, TypeName)               
              \
Same here.

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

https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc#newcode765
src/objects-debug.cc:765: CHECK(byte_offset()->IsSmi() ||
byte_offset()->IsHeapNumber()
Use IsNumber() here and below.

https://codereview.chromium.org/15943002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15943002/diff/1/src/runtime.cc#newcode802
src/runtime.cc:802: #define DATA_VIEW_GETTER(getter, accessor)                  
                 \
Whatever you do, we shouldn't land this with pages of duplicated code.

[bnoordhuis, 2013-05-24 11:49:03]

https://codereview.chromium.org/15943002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6161
src/api.cc:6161: #define DATA_VIEW_GETTER(FunctionName, TypeName)               
              \
On 2013/05/24 06:58:25, Sven Panne wrote:
> Don't duplicate tons of code here, just make swizzling a byte a no-op and do
> things consistently.

That means adding a `bool little_endian` to the GetInt8() and GetUint8() API
functions which seems kind of weird.  Your call.

> Furthermore, can't we use templates somehow instead of this macro-horror?

I don't see how unless it's moved to v8.h - but that means Swizzle() and
IsLittleEndian() have to move as well.

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

https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc#newcode765
src/objects-debug.cc:765: CHECK(byte_offset()->IsSmi() ||
byte_offset()->IsHeapNumber()
On 2013/05/24 06:58:25, Sven Panne wrote:
> Use IsNumber() here and below.

Why is that? I ask because JSArrayBufferVerify() uses the same check.

[bnoordhuis, 2013-05-24 11:53:52]

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

https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc#newcode765
src/objects-debug.cc:765: CHECK(byte_offset()->IsSmi() ||
byte_offset()->IsHeapNumber()
On 2013/05/24 11:49:03, ben2 wrote:
> On 2013/05/24 06:58:25, Sven Panne wrote:
> > Use IsNumber() here and below.
> 
> Why is that? I ask because JSArrayBufferVerify() uses the same check.

Never mind, I see that IsNumber() is just IsSmi() || IsHeapNumber().  Want me to
update JSArrayBufferVerify() as well?

[Sven Panne, 2013-05-24 12:19:20]

https://codereview.chromium.org/15943002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6214
src/api.cc:6214: #define DATA_VIEW_SETTER_SWIZZLE(FunctionName, TypeName)       
              \
Can't we make this macro a template and simply delegate to it? No duplication,
no macros, no need to have a swizzle flag externally visible. Same for getters,
of course.

https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6233
src/api.cc:6233: DATA_VIEW_GETTER(GetInt8, int8_t)
These and the following lines would simply delegate without any macro trickery,
as mentioned above. Templates *are* macros, after all. :-)

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

https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc#newcode765
src/objects-debug.cc:765: CHECK(byte_offset()->IsSmi() ||
byte_offset()->IsHeapNumber()
> Never mind, I see that IsNumber() is just IsSmi() || IsHeapNumber().  Want me
to
> update JSArrayBufferVerify() as well?

This would be nice.

[bnoordhuis, 2013-05-30 23:54:58]

On 2013/05/24 12:19:20, Sven Panne wrote:
> https://codereview.chromium.org/15943002/diff/1/src/api.cc
> File src/api.cc (right):
> 
> https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6214
> src/api.cc:6214: #define DATA_VIEW_SETTER_SWIZZLE(FunctionName, TypeName)     
 
>               \
> Can't we make this macro a template and simply delegate to it? No duplication,
> no macros, no need to have a swizzle flag externally visible. Same for
getters,
> of course.
> 
> https://codereview.chromium.org/15943002/diff/1/src/api.cc#newcode6233
> src/api.cc:6233: DATA_VIEW_GETTER(GetInt8, int8_t)
> These and the following lines would simply delegate without any macro
trickery,
> as mentioned above. Templates *are* macros, after all. :-)
> 
> https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc
> File src/objects-debug.cc (right):
> 
>
https://codereview.chromium.org/15943002/diff/1/src/objects-debug.cc#newcode765
> src/objects-debug.cc:765: CHECK(byte_offset()->IsSmi() ||
> byte_offset()->IsHeapNumber()
> > Never mind, I see that IsNumber() is just IsSmi() || IsHeapNumber().  Want
me
> to
> > update JSArrayBufferVerify() as well?
> 
> This would be nice.

Done. PTAL.

[Dmitry Lomov (no reviews), 2013-06-03 13:09:04]

Ben,
I appreciate the work you put into this patch.

However, I think this implementation is a little premature: 1. DataView is not
yet very well specified in ES6 draft
2. much of the implementation of typed arrays/array buffers etc., especially on
API side, might change significantly while we are integrating this in Blink. On
implementation side, one thing that is not finished at all yet is ArrayBuffer
neutering, and that will affect many implementation details here.

If you noticed, I have been implementing ArrayBuffers and friends in V8, and I
postponed tackling DataView for precisely these reasons.

If you feel like continuing working on this patch, here are a few bigger things
that would be needed:

1. Please split this patch into two parts: one concerning JavaScript side of
things (implementation of constructor and methods), and another concerning V8
API (include/v8.h, api.h etc).
2. Generally there should be a base class for JSTypedArray an JSDataView, such
as JSArrayBufferView, that shares byte_offset, byte_length and buffer.
2. Please add way more tests. I put some suggestions below, also you might want
to take a look at WebKit layout tests. 
Also, my understanding is that your tests will not work on big-endian machine.

I put together some of the more important comments for your code below; I mostly
looked at JS part, not at the API part.

https://codereview.chromium.org/15943002/diff/17001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15943002/diff/17001/src/api.cc#newcode6099
src/api.cc:6099: i::Handle<i::ExternalArray> elements =
DataView is not a external array, so everything here is not needed.

https://codereview.chromium.org/15943002/diff/17001/src/dataview.js
File src/dataview.js (right):

https://codereview.chromium.org/15943002/diff/17001/src/dataview.js#newcode82
src/dataview.js:82: var offset = TO_INT32(byteOffset);
Should be TO_POSITIVE_INTEGER. Byte offsets may be longer than int32. Also in
all getters below.

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode...
src/objects-inl.h:5335: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
Use NumberToSize

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode...
src/objects-inl.h:5353: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
Use NumberToSize.

https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc#newcode795
src/runtime.cc:795: holder->set_map(*map);
This is completely unneeded. DataViews are not external arrays. They do not have
a redefined notion of indexing operation:
var dv = new DataView(new ArrayBuffer(100));
dv[0] = "a";
dv[0] // "a"

https://codereview.chromium.org/15943002/diff/17001/src/runtime.cc#newcode828
src/runtime.cc:828: CONVERT_SMI_ARG_CHECKED(byte_offset, 1);
This is wrong. Even if byte_offset is a result of TO_INT32, it won't fit  into
SMI. The proper implementation should handle byte offest bigger than int 32.
Same applies throughout.

https://codereview.chromium.org/15943002/diff/17001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/utils.h#newcode267
src/utils.h:267: inline enum Endianness GetEndianness() {
This belongs to cpu.h. Should be a #define with possibly STATIC_ASSERT, not a
runtime function

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
File test/mjsunit/harmony/dataview.js (right):

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview.js:1: // Copyright 2013 the V8 project authors.
All rights reserved.
This needs way more tests, including calling constructor with "new", out of
range arguments for the constructor, behavior of DataView as a regular JS object
(adding properties, indexed access etc)

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview.js:30: assertThrows("DataView()", TypeError);
Please do not use string version of assertThrows, use the version that takes a a
function:

assertThrows(function() { DataView(); }, TypeError);

[bnoordhuis, 2013-06-03 13:47:32]

Replying to things that are unclear. No comment means "I agree" or "will do."

On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> Ben,
> I appreciate the work you put into this patch.
> 
> However, I think this implementation is a little premature: 1. DataView is not
> yet very well specified in ES6 draft

I don't exactly disagree but it's already out there. SM implements it and people
use it. Node.js too, for that matter.

> 2. much of the implementation of typed arrays/array buffers etc., especially
on
> API side, might change significantly while we are integrating this in Blink.
On
> implementation side, one thing that is not finished at all yet is ArrayBuffer
> neutering, and that will affect many implementation details here.

What implementation details are we talking here? Neutering is a fairly
straightforward operation, isn't it, both conceptually and implementation-wise?

> If you noticed, I have been implementing ArrayBuffers and friends in V8, and I
> postponed tackling DataView for precisely these reasons.
> 
> If you feel like continuing working on this patch, here are a few bigger
things
> that would be needed:
> 
> 1. Please split this patch into two parts: one concerning JavaScript side of
> things (implementation of constructor and methods), and another concerning V8
> API (include/v8.h, api.h etc).
> 2. Generally there should be a base class for JSTypedArray an JSDataView, such
> as JSArrayBufferView, that shares byte_offset, byte_length and buffer.
> 2. Please add way more tests. I put some suggestions below, also you might
want
> to take a look at WebKit layout tests. 
> Also, my understanding is that your tests will not work on big-endian machine.

Why is that?

[bnoordhuis, 2013-06-03 13:48:12]

https://codereview.chromium.org/15943002/diff/17001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15943002/diff/17001/src/api.cc#newcode6099
src/api.cc:6099: i::Handle<i::ExternalArray> elements =
On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> DataView is not a external array, so everything here is not needed.

Sorry, that's a left-over from my initial attempt.  The DataView we have in
node.js is indexable for convenience reasons. I thought I'd removed it from the
V8 patch but I probably botched a rebase.

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode...
src/objects-inl.h:5335: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> Use NumberToSize

I copied that from ArrayBuffer actually, to keep friction as low as possible.
:-)

Want me to update that as well?

https://codereview.chromium.org/15943002/diff/17001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/utils.h#newcode267
src/utils.h:267: inline enum Endianness GetEndianness() {
On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> This belongs to cpu.h. Should be a #define with possibly STATIC_ASSERT, not a
> runtime function

I'll move it but is turning it into a macro necessary? It's effectively a
compile-time constant and it avoids a lot of compiler-specific #ifdefs.

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
File test/mjsunit/harmony/dataview.js (right):

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview.js:30: assertThrows("DataView()", TypeError);
On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> Please do not use string version of assertThrows, use the version that takes a
a
> function:
> 
> assertThrows(function() { DataView(); }, TypeError);

Will do. Out of curiosity, is it a style thing or are there tangible benefits?

[Dmitry Lomov (no reviews), 2013-06-03 14:25:34]

On 2013/06/03 13:47:32, ben2 wrote:
> Replying to things that are unclear. No comment means "I agree" or "will do."
> 
> On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> > Ben,
> > I appreciate the work you put into this patch.
> > 
> > However, I think this implementation is a little premature: 1. DataView is
not
> > yet very well specified in ES6 draft
> 
> I don't exactly disagree but it's already out there. SM implements it and
people
> use it. Node.js too, for that matter.

Sure, but I am unconvinced as yet that this is the right thing to put into V8
_yet_. 

> 
> > 2. much of the implementation of typed arrays/array buffers etc., especially
> on
> > API side, might change significantly while we are integrating this in Blink.
> On
> > implementation side, one thing that is not finished at all yet is
ArrayBuffer
> > neutering, and that will affect many implementation details here.
> 
> What implementation details are we talking here? Neutering is a fairly
> straightforward operation, isn't it, both conceptually and
implementation-wise?

Well, you need to maintain weak lists of views of array buffers (because once a
buffer is neutered all its views need to be neutered too).
Here is some ongoing work: https://codereview.chromium.org/15562008/.
Frankly I am not too thrilled at the perspective of merging that CL with your
stuff :)


> 
> > If you noticed, I have been implementing ArrayBuffers and friends in V8, and
I
> > postponed tackling DataView for precisely these reasons.
> > 
> > If you feel like continuing working on this patch, here are a few bigger
> things
> > that would be needed:
> > 
> > 1. Please split this patch into two parts: one concerning JavaScript side of
> > things (implementation of constructor and methods), and another concerning
V8
> > API (include/v8.h, api.h etc).
> > 2. Generally there should be a base class for JSTypedArray an JSDataView,
such
> > as JSArrayBufferView, that shares byte_offset, byte_length and buffer.
> > 2. Please add way more tests. I put some suggestions below, also you might
> want
> > to take a look at WebKit layout tests. 
> > Also, my understanding is that your tests will not work on big-endian
machine.
> 
> Why is that?

Ah, ok I see. I though you have that test, but you do not ;)
Well, you need to test that values that you set through DataView functions end
up correctly in the underlying ArrayBuffer. Something like:
var dv = new DataView(ab)
var u = new Uint8Array(ab)
dv.setUint16(0xAAFF);
if (%IsLittleEndian()) {
  assert(0xFF, u[0]);
  assert(0xAA, u[1]);
} else { /* vice versa */ }

[Dmitry Lomov (no reviews), 2013-06-03 14:42:49]

Some replies.

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/objects-inl.h#newcode...
src/objects-inl.h:5335: size_t byte_length =
static_cast<size_t>(this->byte_length()->Number());
On 2013/06/03 13:48:12, ben2 wrote:
> On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> > Use NumberToSize
> 
> I copied that from ArrayBuffer actually, to keep friction as low as possible.
> :-)
> 
> Want me to update that as well? 
Use NumberToSize here, do not update usages in code you have not touched (to
minimize churn)

https://codereview.chromium.org/15943002/diff/17001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/15943002/diff/17001/src/utils.h#newcode267
src/utils.h:267: inline enum Endianness GetEndianness() {
On 2013/06/03 13:48:12, ben2 wrote:
> On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> > This belongs to cpu.h. Should be a #define with possibly STATIC_ASSERT, not
a
> > runtime function
> 
> I'll move it but is turning it into a macro necessary? It's effectively a
> compile-time constant and it avoids a lot of compiler-specific #ifdefs.

Explicitly define expected endianness using V8_TARGET_ARCH_ macros (see global.h
for definitions) and use STATIC_ASSERT and this set of functions to assert that
you have defined it correctly for all architectures.

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
File test/mjsunit/harmony/dataview.js (right):

https://codereview.chromium.org/15943002/diff/17001/test/mjsunit/harmony/data...
test/mjsunit/harmony/dataview.js:30: assertThrows("DataView()", TypeError);
This is stylistic. Friends do not allow friends use eval.

On 2013/06/03 13:48:12, ben2 wrote:
> On 2013/06/03 13:09:04, Dmitry Lomov (chromium) wrote:
> > Please do not use string version of assertThrows, use the version that takes
a
> a
> > function:
> > 
> > assertThrows(function() { DataView(); }, TypeError);
> 
> Will do. Out of curiosity, is it a style thing or are there tangible benefits?

[Dmitry Lomov (no reviews), 2013-07-03 19:38:55]

Closing this since we now ship an implementation of DataView