Add ByteArray interface and provide internal and external implementations.

Add ByteArray interface and provide internal and external implementations.

The new ByteArray interface supersedes the ByteBuffer type which allowed
only external data storage.  This type has been removed and replaced by
the more flexible ByteArray with a default implementation that stores its
elements on the managed heap.

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

[siva, 2012-01-20 22:51:04]

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newcode1
runtime/lib/byte_array.cc:1: // Copyright (c) 2011, the Dart project authors. 
Please see the AUTHORS file
2012 here and other files.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newcode7
runtime/lib/byte_array.cc:7: #include "vm/assembler.h"
Why does assembler.h have to included here?

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:38: if (!byte_array.IsByteArray()) {
Checked handle will assert out if it is not a byte array and we will not be
getting an exception.

Maybe it should be:
const Instance& byte_array = Instance::CheckedHandle(arguments->At(0));


and you could return
return reinterpret_cast<RawT*>(byte_array.raw());

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:47: static Smi& GetIndex(NativeArguments* arguments) {
Our normal style has been to return raw objects from functions, so this would be
RawSmi* GetIndex(....) {

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:56: return index;
just return reinterpret_cast<RawSmi*>(index_instance.raw());
as we have already verified that it is a Smi

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:74: if (value_instance.IsSmi()) {
shouldn't this be
if (!value_instance.IsSmi()) {
  ....
  ....
}

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:88: if (value_instance.IsInteger()) {
if (!value_instance.IsInteger()) {
  ...
  ...
}

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:102: if (value_instance.IsDouble()) {
if (!value_instance.IsDouble()) {
  ...
  ...
}

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:115: const InternalByteArray& array =
GetArray<InternalByteArray>(arguments);
const ArrayType& array = GetArray<ArrayType>(arguments) ??

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:125: const InternalByteArray& array =
GetArray<InternalByteArray>(arguments);
Ditto here?

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:157: SetIndexed<InternalByteArray, uint8_t>(arguments,
GetSmiValue<uint8_t>);
The template instantiation here takes only two type parameters, how does this
work?

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:252: GetIndexed<ExternalByteArray, uint8_t,
Smi>(arguments);
I don't understand how these worked as GetIndexed was not using
ExternalByteArray type at all?

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
byteOffset is confusing? It is really byteOffset or index (appropriate scaling
done by the implementation))?

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:109: add(element);
why not throw here instead of calling 'add'.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:184: void setInt8(int byteOffset, int value) {
why do we need these dart wrappers here, why not just declare
setInt8 as a native method?

It might make sense if you did the range and type check etc. here and throw
exceptions instead of in the native implementation. You could just assert in the
native implementation.

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

http://codereview.chromium.org/9195031/diff/1/runtime/vm/object.cc#newcode7399
runtime/vm/object.cc:7399: return false;
So we can't check if an internal byte array is equal to an external byte array?

http://codereview.chromium.org/9195031/diff/1/runtime/vm/object.cc#newcode7475
runtime/vm/object.cc:7475: }
Ditto comment about comparing with an internal byte array.

[Søren Gjesse, 2012-01-23 10:38:51]

Drive by comments...

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
I think that the index/offset should be sepcified in byes and it should be up to
the user to do any scaling. If the implementation does scaling writing byte
arrays with a mixture of different types becomes more difficult.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
Should it be possible to write in both little and big endian? node.js have both
on their Buffer object:
http://nodejs.org/docs/latest/api/buffers.html#buffer.readUInt16LE

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:52: class ByteArrayBase {
Shouldn't there be an "implements Iteratable, List" here?

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:70: Collection map(f(int element)) {
Long line.

[cshapiro, 2012-01-24 02:14:23]

PTAL

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newcode7
runtime/lib/byte_array.cc:7: #include "vm/assembler.h"
True.  In fact, both assembler.h and bigint_operations.h are unneeded.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.cc#newco...
runtime/lib/byte_array.cc:38: if (!byte_array.IsByteArray()) {
Fixed, the hard way.

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
It is possible although there is no support from the VM.  We could add that
feature, if need be.  It is not yet clear to me whether that functionality
belongs in the ByteArray class or in one of its subclasses.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
It is a byte offset, not a scaled index.  The original implementation used a
scaled index.  That did not survive the code review and the byte offset approach
prevailed.

As an aside, the resolution was to put fast, pre-scaled get and set methods in
the TypedArray-like subclasses, such as Int16Array and Float64Array.  ByteArray
would contain byte oriented access methods.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
For the record, we decided to provide an interface which is something similar to
DataView

https://developer.mozilla.org/en/JavaScript_typed_arrays/DataView

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:52: class ByteArrayBase {
Effectively, yes.  The version of this file that was uploaded was behind a
revision.  What I should have shown has the following signature

  interface ByteArray extends List default InternalByteArray {

I think Iterable and Collection are implicitly inherited by way of List.  So, we
should be okay.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:70: Collection map(f(int element)) {
Fixed.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:109: add(element);
Good point.  I like the idea of throwing rather than calling add.  The error
message will be more descriptive.  I will go ahead and make the change.

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:184: void setInt8(int byteOffset, int value) {
This was Ivan's suggestion.  I may have misunderstood him, but my recollection
was to keep the implementation methods separate from the public interface
methods.

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

http://codereview.chromium.org/9195031/diff/1/runtime/vm/object.cc#newcode7399
runtime/vm/object.cc:7399: return false;
I have removed the equality checks.  They do not make sense for mutable types.

http://codereview.chromium.org/9195031/diff/1/runtime/vm/object.cc#newcode7475
runtime/vm/object.cc:7475: }
Same here.  Gone.

[siva, 2012-01-25 17:22:47]

lgtm

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

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:22: int getUint16(int byteOffset);
Ok got it. 

On 2012/01/24 02:14:23, cshapiro wrote:
> For the record, we decided to provide an interface which is something similar
to
> DataView
> 
> https://developer.mozilla.org/en/JavaScript_typed_arrays/DataView

http://codereview.chromium.org/9195031/diff/1/runtime/lib/byte_array.dart#new...
runtime/lib/byte_array.dart:184: void setInt8(int byteOffset, int value) {
I agree with Ivan's premise that native methods should not be public we should
make them all private. I was just pointing out that we have a public dart method
and we could do all the type checks and range checks etc. here in dart code and
throw exceptions instead of doing the checks in the native implementation. The
native implementations could just assert for these.

On 2012/01/24 02:14:23, cshapiro wrote:
> This was Ivan's suggestion.  I may have misunderstood him, but my recollection
> was to keep the implementation methods separate from the public interface
> methods.

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

http://codereview.chromium.org/9195031/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:76: value ^= value_instance.raw();                    
                   \
Why do you need the value_instance stuff in the GETTER macro? Seems like a cut
and paste error.

[cshapiro, 2012-01-25 23:48:46]

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

http://codereview.chromium.org/9195031/diff/9001/runtime/lib/byte_array.cc#ne...
runtime/lib/byte_array.cc:76: value ^= value_instance.raw();                    
                   \
How embarrassing.  You are correct.

[Ivan Posva, 2012-01-25 23:49:57]

LGTM with comments.

-Ivan

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:34: const Instance& index_instance =
Instance::CheckedHandle(arguments->At(1));
GET_NATIVE_ARGUMENT(Smi, index, arguments->At(1));

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:57: const Instance& array_instance =                  
                   \
A whole chunk here can be replaced with:
GET_NATIVE_ARGUMENT(ArrayT, array, arguments->At(0));
GET_NATIVE_ARGUMENT(Smi, index, arguments->At(1));

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:82: const Instance& array_instance =                  
                   \
ditto + GET_NATIVE_ARGUMENT(ObjectT, value, arguments->At(2));

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:52: class ByteArrayBase {
Should ByteArrayBase a private class? How about all the other classes in this
file?

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:71: return Collections.map(this, new
GrowableObjectArray.withCapacity(length), f);
long line

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/vm/object.cc...
runtime/vm/object.cc:7479: result.SetLength(len);
Not calling bzero here will cause you to leak the byte values of the null
object.

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/vm/object.h#...
runtime/vm/object.h:3308: static RawInternalByteArray* New(const uint8_t*
characters,
characters?

[cshapiro, 2012-01-26 02:25:42]

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:34: const Instance& index_instance =
Instance::CheckedHandle(arguments->At(1));
Removed entirely.

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:57: const Instance& array_instance =                  
                   \
Yes, that looks much better.  Done.

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.cc:82: const Instance& array_instance =                  
                   \
Yup.  Done.

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:52: class ByteArrayBase {
Done.

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/lib/byte_arr...
runtime/lib/byte_array.dart:71: return Collections.map(this, new
GrowableObjectArray.withCapacity(length), f);
Fixed

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/vm/object.cc...
runtime/vm/object.cc:7479: result.SetLength(len);
Right.  I assumed we zero initialized.  Fixed.

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

https://chromiumcodereview.appspot.com/9195031/diff/9001/runtime/vm/object.h#...
runtime/vm/object.h:3308: static RawInternalByteArray* New(const uint8_t*
characters,
Well, they are characters in the C sense.  That is probably a bit confusing.  I
have renamed this data, like we have in the definition of the implementation.