Implement {Int,Uint}{8,16,32,64} and Float{32,64} typed arrays.

Implement {Int,Uint}{8,16,32,64} and Float{32,64} typed arrays.

Internal and external representations of each array are provided.  A
single interface type implementing the ByteArray and List interfaces
is shared among the internal and external implementation type.

Committed: https://code.google.com/p/dart/source/detail?r=7311

[Ivan Posva, 2012-05-02 08:19:17]

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
File runtime/lib/byte_array.cc (right):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:36: GET_NATIVE_ARGUMENT(ObjectT, integer_value,
arguments->At(2));
I find the name integer_value a bit confusing here. Aren't float values also
kept in here?

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
File runtime/lib/byte_array.dart (right):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:5: interface ByteArray default _Uint8Array {  //
TODO
Please document what is there TODO?

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:54: interface ByteArrayViewable {
I am not sure I understand this interface and its purpose.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:126: throw const
UnsupportedOperationException("Cannot add to a non-extendable array");
Long line and many below.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:129: void addLast(int value) {
The type here should be parameterized with E, where E is the element type of the
 array. Otherwise you will get unexpected exceptions in checked mode when you do
this:

var arr = new Float64Array(len);
arr.addLast(1.0);

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:171: _ByteArray from, int startFromInBytes) native
"ByteArray_setRange";
Long line.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:1524: throw const
UnsupportedOperationException("Cannot add to a non-extendable array");
Why not add a base class _ByteArrayView similar to _ByteArray to avoid the
duplication of these methods?

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/dart_api_...
File runtime/vm/dart_api_impl.cc (left):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:1656: if (obj.IsByteArray()) {
You should preserve support for fast handling of byte arrays. This is essential
for dart:io.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:1830: if (obj.IsByteArray()) {
ditto.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/dart_api_...
runtime/vm/dart_api_impl.cc:1914: if (obj.IsByteArray()) {
ditto.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/intrinsif...
File runtime/vm/intrinsifier.h (left):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/intrinsif...
runtime/vm/intrinsifier.h:65: V(_ByteArrayBase, get:length,
ByteArrayBase_getLength)                       \
I hope there is a plan to intrinsify the byte arrays needed for dart:io.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/object.cc...
runtime/vm/object.cc:595: public_class_name = String::New("_Int8Array");
How about adding a RegisterPrivateClass similar to RegisterClass? Then all of
this extra code could be abstracted out.

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/object.h
File runtime/vm/object.h (right):

https://chromiumcodereview.appspot.com/9958046/diff/9002/runtime/vm/object.h#...
runtime/vm/object.h:4174: return Length() * sizeof(int8_t);
Internal arrays have a kBytesPerElement, why not here?

[cshapiro, 2012-05-03 04:01:49]

PTAL

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.cc
File runtime/lib/byte_array.cc (right):

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:36: GET_NATIVE_ARGUMENT(ObjectT, integer_value,
arguments->At(2));
True.  I have used the symbol "value_object" instead.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:5: interface ByteArray default _Uint8Array {  //
TODO
Right.  These TODO statements mark changes that should be removed after the
library is updated.  I will add that to the TODO.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:54: interface ByteArrayViewable {
This interface contains methods for viewing a typed array as a byte array (a
data array like object).

There was disagreement over having the array types share a common superclass
with all of the low-level access methods.  This interface provides the work
around.  This interface to type methods that write bytes to sockets, streams, or
other such interfaces.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:126: throw const
UnsupportedOperationException("Cannot add to a non-extendable array");
Wrapped.  Done.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:129: void addLast(int value) {
Hmm...

The _Float64Array class extends _ByteArray so if _ByteArray were made generic,
we would need a new member in RawFloat64Array to hold the type of E, as is done
for RawArray.  That seems like a high cost to pay for a checked mode warning on
methods that throw an error. 

Do you know of a way around that? If there is, I am happy to implement it.

In the mean time, I have worked around this by removing the type information
from value.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:171: _ByteArray from, int startFromInBytes) native
"ByteArray_setRange";
All long lines have now been wrapped.

As an aside, I am not sure there is an agreement in the sources as to what a
long line is.  I am happy to do 80 columns, though.

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:1524: throw const
UnsupportedOperationException("Cannot add to a non-extendable array");
Sounds good.  Done.

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (left):

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1656: if (obj.IsByteArray()) {
Done.  Restored.  I think this case is easy to keep.  The ones below are not as
straight forward.

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1830: if (obj.IsByteArray()) {
I will make this work for the Uint8Array types.  I am not sure any other types
make sense.

The semantics of this function and its setter variant are not obvious.  While
they were the only way to transmit data back when, now we have something better.
 Once this change is complete, we ought to replace these (rather slow) methods
with something specific to byte arrays.

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1914: if (obj.IsByteArray()) {
Sure, with the same limitations as above.

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/object.cc#newcode595
runtime/vm/object.cc:595: public_class_name = String::New("_Int8Array");
Sounds good.  Done.

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/object.h
File runtime/vm/object.h (right):

http://codereview.chromium.org/9958046/diff/9002/runtime/vm/object.h#newcode4174
runtime/vm/object.h:4174: return Length() * sizeof(int8_t);
Good question.  I define kBytesPerElement for all external types except the
ExternalInt8Array and ExternalUint8Array.  I have made those two classes like
all the others.

[Ivan Posva, 2012-05-03 05:53:19]

LGTM with comments.

-Ivan

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9958046/diff/9002/runtime/lib/byte_array.dart#...
runtime/lib/byte_array.dart:129: void addLast(int value) {
On 2012/05/03 04:01:50, cshapiro wrote:
> Hmm...
> 
> The _Float64Array class extends _ByteArray so if _ByteArray were made generic,
> we would need a new member in RawFloat64Array to hold the type of E, as is
done
> for RawArray.  That seems like a high cost to pay for a checked mode warning
on
> methods that throw an error. 
> 
> Do you know of a way around that? If there is, I am happy to implement it.
> 
> In the mean time, I have worked around this by removing the type information
> from value.  

I think that is fine for now. Another option would be to duplicate this class
for the double typed arrays once we see that the missing checked mode failure
becomes an issue.

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1388: assert(array is _ByteArrayBase);
Isn't this assert redundant? This test will already be executed as part of the
parameter checking when running in checked mode.

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1502: class _ByteArrayViewBase<E> {
The use of E in this case is inconsistent to the _ByteArrayBase above. Also you
are not passing a type parameter in any of the subclasses. Please either remove
the E and or make it consistent.

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1513: void addAll(Collection<int> value) {
If you keep E then please change this to "Collection<E>".

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/intrinsifier.h
File runtime/vm/intrinsifier.h (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/intrinsifier.h#n...
runtime/vm/intrinsifier.h:64: V(GrowableObjectArray, set:data,
GrowableArray_setData)                      \
I am still concerned about the performance implications of removing these
intrinsics.

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:627: // String& public_class_name = String::Handle();
Comments?

[cshapiro, 2012-05-04 04:42:04]

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart
File runtime/lib/byte_array.dart (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1388: assert(array is _ByteArrayBase);
Removed.

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1502: class _ByteArrayViewBase<E> {
Removed.

http://codereview.chromium.org/9958046/diff/16002/runtime/lib/byte_array.dart...
runtime/lib/byte_array.dart:1513: void addAll(Collection<int> value) {
Removed.

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/intrinsifier.h
File runtime/vm/intrinsifier.h (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/intrinsifier.h#n...
runtime/vm/intrinsifier.h:64: V(GrowableObjectArray, set:data,
GrowableArray_setData)                      \
I spoke with Srdjan about getting some help writing new intrinsic
implementations of the access methods.  We will work on that together
post-submit.

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/9958046/diff/16002/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:627: // String& public_class_name = String::Handle();
Sorry, you weren't supposed to see that.  Fixed!