Improve performance of Dart_ListGetAsBytes.

Improve performance of Dart_ListGetAsBytes.

This fixes two performance issues with Dart_ListGetAsBytes:

1. Avoid excessive handle allocation when copying from a generic List.

2. Add a fast path when copying from byte-size typed data view objects. The
old version only supported typed data, but no views.

R=asiva@google.com

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

[siva, 2013-08-01 16:29:31]

lgtm

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

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2179: Smi::Value(TypedDataView::OffsetInBytes(view))
+ offset;
This data_offset computation can be moved inside the
if (array.ElementSizeInBytes() == 1) { ... } block.

or you could compute this value early and reuse the obj handle for
'data'.

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2183: return CopyBytes(array, data_offset,
native_array, length);
We end up doing the range check twice in this case, maybe we
could move the Range check out of CopyBytes and do the RangeCheck
outside for the TypedData case too.

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2225: HandleScope handle_scope(isolate);
Could you use HANDLESCOPE(isolate); here to be consistent with the Usage
pattern.

[ricow1, 2013-08-01 16:29:49]

DBC

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

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl_test...
runtime/vm/dart_api_impl_test.cc:840: "List main(int size) {"
add \n here and below

[Florian Schneider, 2013-08-02 11:07:33]

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

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2179: Smi::Value(TypedDataView::OffsetInBytes(view))
+ offset;
On 2013/08/01 16:29:31, siva wrote:
> This data_offset computation can be moved inside the
> if (array.ElementSizeInBytes() == 1) { ... } block.
> 
> or you could compute this value early and reuse the obj handle for
> 'data'.

Done.

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2183: return CopyBytes(array, data_offset,
native_array, length);
On 2013/08/01 16:29:31, siva wrote:
> We end up doing the range check twice in this case, maybe we
> could move the Range check out of CopyBytes and do the RangeCheck
> outside for the TypedData case too.

Agree.  The second range check is redundant because there is already one
performed on the length of the TypedDataView object and a view is always within
the bounds of the underlying buffer.

I left it here as a safe-guard, but instead I will turn it into an ASSERT.

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl.cc#n...
runtime/vm/dart_api_impl.cc:2225: HandleScope handle_scope(isolate);
On 2013/08/01 16:29:31, siva wrote:
> Could you use HANDLESCOPE(isolate); here to be consistent with the Usage
> pattern.

Done.

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

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl_test...
runtime/vm/dart_api_impl_test.cc:840: "List main(int size) {"
On 2013/08/01 16:29:50, ricow1 wrote:
> add \n here and below

Done.

https://codereview.chromium.org/21562002/diff/1/runtime/vm/dart_api_impl_test...
runtime/vm/dart_api_impl_test.cc:840: "List main(int size) {"
On 2013/08/01 16:29:50, ricow1 wrote:
> add \n here and below

Done.

[Florian Schneider, 2013-08-02 11:32:10]

Committed patchset #2 manually as r25731 (presubmit successful).

[Ivan Posva, 2013-08-05 03:45:00]

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

https://codereview.chromium.org/21562002/diff/9001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:2179: if (data.IsTypedData()) {
How can this ever not be a typed data? I would expect this to be converted to an
ASSERT, no?

[Florian Schneider, 2013-08-05 08:38:09]

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

https://codereview.chromium.org/21562002/diff/9001/runtime/vm/dart_api_impl.c...
runtime/vm/dart_api_impl.cc:2179: if (data.IsTypedData()) {
On 2013/08/05 03:45:00, Ivan Posva wrote:
> How can this ever not be a typed data? I would expect this to be converted to
an
> ASSERT, no?

Not sure: the view constructor takes just a ByteBuffer argument, so the view can
be on anything implementing that interface.