Make VM TypedList not implement ByteBuffer.

Make VM TypedList not implement ByteBuffer.

BUG= http://dartbug.com/10136
R=asiva@google.com, sra@google.com

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

[Lasse Reichstein Nielsen, 2014-01-14 11:06:46]

https://codereview.chromium.org/138033002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/138033002/diff/1/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:2600: result = GetByteDataConstructor(isolate,
Symbols::ByteDataDot_view(), 3);
Would it be better to get the _ByteDataView constructor directly?

[Søren Gjesse, 2014-01-14 12:00:17]

https://codereview.chromium.org/138033002/diff/40001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/40001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:622: // have a buffer extracted.
Is this just to save an instance variable? You are probably right that most
types lists will never have .buffer called, but still...

https://codereview.chromium.org/138033002/diff/40001/tests/lib/typed_data/typ...
File tests/lib/typed_data/typed_list_buffer_test.dart (right):

https://codereview.chromium.org/138033002/diff/40001/tests/lib/typed_data/typ...
tests/lib/typed_data/typed_list_buffer_test.dart:38: (ByteBuffer b) => new
Uint32List.view(b)),
Maybe add a comment that this multi test is to separate out 64-bit integers
which are not supported by dart2js.

[Lasse Reichstein Nielsen, 2014-01-14 12:19:17]

https://codereview.chromium.org/138033002/diff/40001/runtime/lib/typed_data.dart
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/40001/runtime/lib/typed_data.d...
runtime/lib/typed_data.dart:622: // have a buffer extracted.
On 2014/01/14 12:00:17, Søren Gjesse wrote:
> Is this just to save an instance variable? 

Yes.

https://codereview.chromium.org/138033002/diff/40001/tests/lib/typed_data/typ...
File tests/lib/typed_data/typed_list_buffer_test.dart (right):

https://codereview.chromium.org/138033002/diff/40001/tests/lib/typed_data/typ...
tests/lib/typed_data/typed_list_buffer_test.dart:38: (ByteBuffer b) => new
Uint32List.view(b)),
On 2014/01/14 12:00:17, Søren Gjesse wrote:
> Maybe add a comment that this multi test is to separate out 64-bit integers
> which are not supported by dart2js.

Done.

[srdjan, 2014-01-14 16:16:52]

Are there any performance differences?

[Lasse Reichstein Nielsen, 2014-01-14 16:25:58]

On 2014/01/14 16:16:52, srdjan wrote:
> Are there any performance differences?

I haven't checked. Will upload on golem tomorrow.
It's likely to be a small overhead, due to the extra wrapper object for
ByteBuffer, but that's only for moving the buffer around, all data access is
still directly on the typed list.

[Cutch, 2014-01-14 21:44:28]

On 2014/01/14 16:25:58, Lasse Reichstein Nielsen wrote:
> On 2014/01/14 16:16:52, srdjan wrote:
> > Are there any performance differences?
> 
> I haven't checked. Will upload on golem tomorrow.
> It's likely to be a small overhead, due to the extra wrapper object for
> ByteBuffer, but that's only for moving the buffer around, all data access is
> still directly on the typed list.

Hi,

What is the rationale for this change? I don't find the referenced bug very
enlightening. It seems like we are trying to emulate some aspect of JavaScript
typed arrays?

FYI, I'm the maintainer of dart:typed_data, so, in the future, please put me as
the reviewer for any dart:typed_data changes.

Thanks,
John

[siva, 2014-01-14 22:02:16]

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:623: static var _buffers = new Expando();
Why does the wrapper buffer object have to be the same value each time?
The underlying typedData will be the same and your == operator in _ByteBuffer
would return true even with different objects right.

https://codereview.chromium.org/138033002/diff/110001/tests/lib/typed_data/ty...
File tests/lib/typed_data/typed_list_buffer_test.dart (right):

https://codereview.chromium.org/138033002/diff/110001/tests/lib/typed_data/ty...
tests/lib/typed_data/typed_list_buffer_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
2014

[sra1, 2014-01-14 22:17:38]

On 2014/01/14 21:44:28, Cutch wrote:
> On 2014/01/14 16:25:58, Lasse Reichstein Nielsen wrote:
> > On 2014/01/14 16:16:52, srdjan wrote:
> > > Are there any performance differences?
> > 
> > I haven't checked. Will upload on golem tomorrow.
> > It's likely to be a small overhead, due to the extra wrapper object for
> > ByteBuffer, but that's only for moving the buffer around, all data access is
> > still directly on the typed list.
> 
> Hi,
> 
> What is the rationale for this change? I don't find the referenced bug very
> enlightening. It seems like we are trying to emulate some aspect of JavaScript
> typed arrays?

Yes.  There is a history of code on written on Dartium that doesn't work when
compiled to JavaScript.
The inconsistency between the two libraries causes problems in many places that
typed arrays are used in the browser stack.  Folding two types with different
roles onto one, the intent becomes ambiguous.  postmessage and indexDB in
particular have problems because the consumer can be coded in JavaScript.

> 
> FYI, I'm the maintainer of dart:typed_data, so, in the future, please put me
as
> the reviewer for any dart:typed_data changes.

I should also be a reviewer for some changes.  There are special considerations
with the dart2js implementation  (the compiler does various optimizations which
are dependent on how the classes are written), and I want to make sure we can
continue support typed_data efficiently in both the VM and browser.

> 
> Thanks,
> John

[Lasse Reichstein Nielsen, 2014-01-15 09:47:58]

Yes, the rationale for the change is that it is an long-standing open bug that
keeps tripping up users. I would have preferred fixing it before 1.0, but that
didn't happen.

The reason for fixing it now is that I realized that there is a simple solution
which won't require a deep rewrite of the actual typed array classes, and that
*should* be almost performance neutral (if you don't use .buffer, it costs
nothing, and if you do, it is only overhead while you carry around the buffer,
the actual data access is still the same).

If we can allow the buffers to not be identical, we can make it even cheaper. I
don't know if that would trip up users that expect the JS behavior.

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:623: static var _buffers = new Expando();
There are co19 tests that assume the buffers to be identical.

If we don't want that, I can easily change it back (I found those tests because
I initially returned a new ByteBuffer object each time).

https://codereview.chromium.org/138033002/diff/110001/tests/lib/typed_data/ty...
File tests/lib/typed_data/typed_list_buffer_test.dart (right):

https://codereview.chromium.org/138033002/diff/110001/tests/lib/typed_data/ty...
tests/lib/typed_data/typed_list_buffer_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
That'll be a safe comment on all my new files for another couple of months :)

[Lasse Reichstein Nielsen, 2014-01-15 10:39:29]

Add more review volunteers.

[siva, 2014-01-15 18:29:07]

Once the use of expando to mandate that the buffers are identical is removed
your cl LGTM.

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/110001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:623: static var _buffers = new Expando();
I don't think anywhere in the spec we state that the buffers have to be
identical, I don't think the co19 tests should be construed as a standard. I
would file a bug stating those tests are invalid and needs to be fixed.

On 2014/01/15 09:47:58, Lasse Reichstein Nielsen wrote:
> There are co19 tests that assume the buffers to be identical.
> 
> If we don't want that, I can easily change it back (I found those tests
because
> I initially returned a new ByteBuffer object each time).

[sra1, 2014-01-15 19:09:46]

lgtm for impact on dart2js

https://codereview.chromium.org/138033002/diff/220001/tests/lib/typed_data/ty...
File tests/lib/typed_data/typed_list_buffer_test.dart (right):

https://codereview.chromium.org/138033002/diff/220001/tests/lib/typed_data/ty...
tests/lib/typed_data/typed_list_buffer_test.dart:17: List constructors = [
Please add ByteData to the tested TypedData types.

[Lasse Reichstein Nielsen, 2014-01-16 10:19:20]

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

[Ivan Posva, 2014-01-16 16:07:31]

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:2348: : _typedData = (_buffer as
_ByteBuffer)._typedData,
Please remove this unnecessary type test.

[Lasse Reichstein Nielsen, 2014-01-16 18:33:30]

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:2348: : _typedData = (_buffer as
_ByteBuffer)._typedData,
It's not unnecessary (ByteBuffer does not have a _typedData getter), but I can
change the parameter type to _ByteBuffer.

[Lasse Reichstein Nielsen, 2014-01-16 18:34:19]

And reverted because html/transferable_test fails.

[srdjan, 2014-01-16 21:22:58]

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:2348: : _typedData = (_buffer as
_ByteBuffer)._typedData,
On 2014/01/16 18:33:30, Lasse Reichstein Nielsen wrote:
> It's not unnecessary (ByteBuffer does not have a _typedData getter), but I can
> change the parameter type to _ByteBuffer.

Change the parameter type please.

[Lasse Reichstein Nielsen, 2014-01-17 10:40:00]

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
File runtime/lib/typed_data.dart (right):

https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
runtime/lib/typed_data.dart:2348: : _typedData = (_buffer as
_ByteBuffer)._typedData,
On 2014/01/16 21:22:58, srdjan wrote:
> On 2014/01/16 18:33:30, Lasse Reichstein Nielsen wrote:
> > It's not unnecessary (ByteBuffer does not have a _typedData getter), but I
can
> > change the parameter type to _ByteBuffer.
> 
> Change the parameter type please.

Done.

[Cutch, 2014-02-06 17:27:29]

On 2014/01/17 10:40:00, Lasse Reichstein Nielsen wrote:
>
https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
> File runtime/lib/typed_data.dart (right):
> 
>
https://codereview.chromium.org/138033002/diff/290001/runtime/lib/typed_data....
> runtime/lib/typed_data.dart:2348: : _typedData = (_buffer as
> _ByteBuffer)._typedData,
> On 2014/01/16 21:22:58, srdjan wrote:
> > On 2014/01/16 18:33:30, Lasse Reichstein Nielsen wrote:
> > > It's not unnecessary (ByteBuffer does not have a _typedData getter), but I
> can
> > > change the parameter type to _ByteBuffer.
> > 
> > Change the parameter type please.
> 
> Done.

Is this CL abandoned ?

[Lasse Reichstein Nielsen, 2014-02-10 10:06:12]

Not abandoned. I did hit a problem and got sidetracked doing other stuff, but I
expect to get back to it.

[Cutch, 2014-06-11 20:15:24]

On 2014/02/10 10:06:12, Lasse Reichstein Nielsen wrote:
> Not abandoned. I did hit a problem and got sidetracked doing other stuff, but
I
> expect to get back to it.

Lasse, TypedList implementing ByteBuffer has been causing some confusion- any
plan to finish this?

[Lasse Reichstein Nielsen, 2014-06-12 05:56:40]

On 2014/06/11 20:15:24, Cutch wrote:

> Lasse, TypedList implementing ByteBuffer has been causing some confusion- any
> plan to finish this?

I've been prioritizing Isolates for a while. I should be able to look at this
again soon.

[Cutch, 2014-06-17 14:56:19]

On 2014/06/12 05:56:40, Lasse Reichstein Nielsen wrote:
> On 2014/06/11 20:15:24, Cutch wrote:
> 
> > Lasse, TypedList implementing ByteBuffer has been causing some confusion-
any
> > plan to finish this?
> 
> I've been prioritizing Isolates for a while. I should be able to look at this
> again soon.

Landed something very similar https://codereview.chromium.org/339763002.