Fast copy between TypedData and ExternalTypedData

Fast copy between TypedData and ExternalTypedData

Until now, copying data from TypedData to TypedData and from ExternalTypedData
to ExternalTypedData was fast. But copying between TypedData and
ExternalTypedData was not handled in a fast way.

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

[Vyacheslav Egorov (Google), 2013-04-16 14:22:23]

https://codereview.chromium.org/14296006/diff/1/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/1/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:82: #define COPY_DATA_AND_RETURN(dsttype, dst, srctype,
src)                       \
I'd prefer good old template to this huge macro.

[kustermann, 2013-04-16 15:48:51]

PTAL

[Vyacheslav Egorov (Google), 2013-04-16 16:23:06]

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:92: if (src_element_size_in_bytes !=
dst_element_size_in_bytes) {
You need to compare element types.

[siva, 2013-04-16 16:36:48]

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:94: }
I don't believe this check is sufficient, if dst is Uint8ClampedArray and src is
one of Int8Array or Uint8Array you would get all wrong results.

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:109: }
Why does this have to be explicitly listed here instead of calling the Copy
function that is provided in object.cc

We would like to minimize the spread of these NoGCScope regions all over the
code base as far as possible.

I would change this to call the appropriate Copy function ....

[kustermann, 2013-04-16 17:26:04]

Thanks for taking a look.

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:92: if (src_element_size_in_bytes !=
dst_element_size_in_bytes) {
On 2013/04/16 16:23:06, Vyacheslav Egorov (Google) wrote:
> You need to compare element types.

Done.

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:94: }
On 2013/04/16 16:36:48, siva wrote:
> I don't believe this check is sufficient, if dst is Uint8ClampedArray and src
is
> one of Int8Array or Uint8Array you would get all wrong results.

I added now an ElementType function to TypedData/ExternalTypedData (the same way
we have a ElementSizeInBytes function).

https://codereview.chromium.org/14296006/diff/3/runtime/lib/typeddata.cc#newc...
runtime/lib/typeddata.cc:109: }
On 2013/04/16 16:36:48, siva wrote:
> Why does this have to be explicitly listed here instead of calling the Copy
> function that is provided in object.cc
> 
> We would like to minimize the spread of these NoGCScope regions all over the
> code base as far as possible.
> 
> I would change this to call the appropriate Copy function ....

The reason for this is that the appropriate Copy functions don't accept
arguments of the other type. So 
ExternalTypedData::Copy(external_typed_array, .., typed_array, ...) doesn't
work:

void ExternalTypedData::Copy(const ExternalTypedData& dst, ...)
void TypedData::Copy(const TypedData& dst, ...)

But we could move the CopyData function to object.cc?

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5039: return ElementType(cid);
It is not the nicest thing that these functions return 'intptr_t'.

Alternatively, I could add a field to TypedData/ExternalTypedData which stores
it's "TypedDataType".

[Vyacheslav Egorov (Google), 2013-04-17 11:41:56]

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc#n...
runtime/lib/typeddata.cc:103: NoGCScope no_gc;
If TypedData and ExternalTypedData had a base class that class could have hosted
all this functionality.

I have no strong opinion on this though.

I can see that if we'll move this code object.cc that would lead to tons of
duplication.

Maybe Siva has an idea?

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5039: return ElementType(cid);
On 2013/04/16 17:26:04, kustermann wrote:
> It is not the nicest thing that these functions return 'intptr_t'.
> 
> Alternatively, I could add a field to TypedData/ExternalTypedData which stores
> it's "TypedDataType".

Why not introduce an enum:

enum TypeDataElementType {
#define V(name) k##name##Element,
}

and then use it here? (cast result to it).

[Søren Gjesse, 2013-04-17 12:39:45]

lgtm from me, but please get an lgtm from siva.

[Vyacheslav Egorov (Google), 2013-04-17 12:55:27]

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5039: return ElementType(cid);
I meant:

enum TypeDataElementType {
#define V(name) k##name##Element,
CLASS_LIST_TYPED_DATA(V)
#undef V
}

On 2013/04/17 11:41:56, Vyacheslav Egorov (Google) wrote:
> On 2013/04/16 17:26:04, kustermann wrote:
> > It is not the nicest thing that these functions return 'intptr_t'.
> > 
> > Alternatively, I could add a field to TypedData/ExternalTypedData which
stores
> > it's "TypedDataType".
> 
> Why not introduce an enum:
> 
> enum TypeDataElementType {
> #define V(name) k##name##Element,
> }
> 
> and then use it here? (cast result to it).

[Anders Johnsen, 2013-04-18 11:00:43]

STV! :)

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc#n...
runtime/lib/typeddata.cc:91: if (dst_array.ElementType() ==
src_array.ElementType()) {
DBC: !=

[kustermann, 2013-04-18 13:57:57]

It would be nice if you could take another look.

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc#n...
runtime/lib/typeddata.cc:91: if (dst_array.ElementType() ==
src_array.ElementType()) {
On 2013/04/18 11:00:43, Anders Johnsen wrote:
> DBC: !=
That was really stupid of me.

https://codereview.chromium.org/14296006/diff/7001/runtime/lib/typeddata.cc#n...
runtime/lib/typeddata.cc:103: NoGCScope no_gc;
On 2013/04/17 11:41:56, Vyacheslav Egorov (Google) wrote:
> If TypedData and ExternalTypedData had a base class that class could have
hosted
> all this functionality.
> 
> I have no strong opinion on this though.
> 
> I can see that if we'll move this code object.cc that would lead to tons of
> duplication.
> 
> Maybe Siva has an idea?

I agree that a base class would solve the code duplication issue. But since I'm
not familiar with the VM code, I'd rather keep my change small.

I moved now this piece of code to another function template in object.h.

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/7001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5039: return ElementType(cid);
On 2013/04/17 12:55:27, Vyacheslav Egorov (Google) wrote:
> I meant:
> 
> enum TypeDataElementType {
> #define V(name) k##name##Element,
> CLASS_LIST_TYPED_DATA(V)
> #undef V
> }
> 
> On 2013/04/17 11:41:56, Vyacheslav Egorov (Google) wrote:
> > On 2013/04/16 17:26:04, kustermann wrote:
> > > It is not the nicest thing that these functions return 'intptr_t'.
> > > 
> > > Alternatively, I could add a field to TypedData/ExternalTypedData which
> stores
> > > it's "TypedDataType".
> > 
> > Why not introduce an enum:
> > 
> > enum TypeDataElementType {
> > #define V(name) k##name##Element,
> > }
> > 
> > and then use it here? (cast result to it). 
> 

Done.

[siva, 2013-04-19 17:38:40]

LGTM with some comments.

https://codereview.chromium.org/14296006/diff/17003/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/lib/typeddata.cc#...
runtime/lib/typeddata.cc:104: length_in_bytes);
See comments in object.h about templatizing TypedData::Copy

Then over here you would just call:

TypedData::Copy(dst_array, dst_offset_in_bytes, src_array, src_offset_in_bytes,
length_in_bytes);

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:12614: dst, dst_offset_in_bytes, src, src_offset_in_bytes,
length_in_bytes);
This code can be removed as it is already set up in object.h

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:12662: }
Get rid of this Copy function.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5092: return element_size[class_id -
kTypedDataInt8ArrayCid];
you could use your new ElementType() here instead of
class_id - kTypedDataInt8ArrayCid

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5204: return TypedData::element_size[class_id -
kExternalTypedDataInt8ArrayCid];
Ditto comment about using the new ElementType

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5273: }
Why not make this the static function of class TypedData.
i.e:

class TypedData {
  .....
  .....
  template<typename DstType, typename SrcType>
  static void Copy(const DstType& dst, intptr_t dst_offset_in_bytes,
                   const SrcType& src, intptr_t src_offset_in_bytes,
                   intptr_t length_in_bytes) {
    add the code you have in TypedArrayCopyData here....
  }
  ....
  ....
};

Get rid of the Copy function in ExternalTypedData, everybody would use the
templatized copy function you define above.

[kustermann, 2013-04-19 19:21:54]

Thanks for taking another look. 

I'll commit this CL on Monday.

https://codereview.chromium.org/14296006/diff/17003/runtime/lib/typeddata.cc
File runtime/lib/typeddata.cc (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/lib/typeddata.cc#...
runtime/lib/typeddata.cc:104: length_in_bytes);
On 2013/04/19 17:38:40, siva wrote:
> See comments in object.h about templatizing TypedData::Copy
> 
> Then over here you would just call:
> 
> TypedData::Copy(dst_array, dst_offset_in_bytes, src_array,
src_offset_in_bytes,
> length_in_bytes);

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:12614: dst, dst_offset_in_bytes, src, src_offset_in_bytes,
length_in_bytes);
On 2013/04/19 17:38:40, siva wrote:
> This code can be removed as it is already set up in object.h

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.cc#newc...
runtime/vm/object.cc:12662: }
On 2013/04/19 17:38:40, siva wrote:
> Get rid of this Copy function.

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5092: return element_size[class_id -
kTypedDataInt8ArrayCid];
On 2013/04/19 17:38:40, siva wrote:
> you could use your new ElementType() here instead of
> class_id - kTypedDataInt8ArrayCid

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5092: return element_size[class_id -
kTypedDataInt8ArrayCid];
On 2013/04/19 17:38:40, siva wrote:
> you could use your new ElementType() here instead of
> class_id - kTypedDataInt8ArrayCid

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5204: return TypedData::element_size[class_id -
kExternalTypedDataInt8ArrayCid];
On 2013/04/19 17:38:40, siva wrote:
> Ditto comment about using the new ElementType

Done.

https://codereview.chromium.org/14296006/diff/17003/runtime/vm/object.h#newco...
runtime/vm/object.h:5273: }
On 2013/04/19 17:38:40, siva wrote:
> Why not make this the static function of class TypedData.
> i.e:
> 
> class TypedData {
>   .....
>   .....
>   template<typename DstType, typename SrcType>
>   static void Copy(const DstType& dst, intptr_t dst_offset_in_bytes,
>                    const SrcType& src, intptr_t src_offset_in_bytes,
>                    intptr_t length_in_bytes) {
>     add the code you have in TypedArrayCopyData here....
>   }
>   ....
>   ....
> };
> 
> Get rid of the Copy function in ExternalTypedData, everybody would use the
> templatized copy function you define above.

Done.

[kustermann, 2013-04-22 08:27:25]

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