Cleanup throwing of the RangeError in the runtime to remove duplicated code.

Cleanup throwing of the RangeError in the runtime to remove duplicated code.

Improve List._copyFromObjectArray native performance for large arrays by using PassiveObject instead of Object.

R=iposva@google.com
BUG=

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

[Ivan Posva, 2014-09-02 15:27:07]

https://codereview.chromium.org/533483003/diff/1/runtime/lib/array.cc
File runtime/lib/array.cc (left):

https://codereview.chromium.org/533483003/diff/1/runtime/lib/array.cc#oldcode81
runtime/lib/array.cc:81: Object& src_obj = Object::Handle();
PassiveObject& src_obj = ...

Since we do not need to setup the vtable in the handle at all.

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

https://codereview.chromium.org/533483003/diff/1/runtime/vm/exceptions.cc#new...
runtime/vm/exceptions.cc:673: Exceptions::ThrowByType(Exceptions::kRange, args);
It would be helpful to give the expected range here as we do in other places in
the libraries. I do understand that this was not the case with most/all of the
code being refactored here, but I think it would be a nice improvement.

That means that you need to add two different ThrowRangeError functions here,
similar to the constructors of RangeError and additionally a private factory in
RangeError to distinguish them.

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

https://codereview.chromium.org/533483003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:18178: void Array::CopyFrom(intptr_t dst_start,
As discussed offline we will need to keep this as a copy of elements in the
general case, so maybe move this back to where it came from.

For copies into a newly allocated array (e.g from sublist or toList) though we
could use a similar strategy, which might call memmove as a leaf function
straight from recognized/intrisified code. Different CL maybe?

[Vyacheslav Egorov (Google), 2014-09-02 16:02:53]

I will split error messages out into a separate CL, I still not sure what we
want to do to fix the problem, which was the main purpose of this CL.

I see a clear solution only for a part of the problem.

https://codereview.chromium.org/533483003/diff/1/runtime/lib/array.cc
File runtime/lib/array.cc (left):

https://codereview.chromium.org/533483003/diff/1/runtime/lib/array.cc#oldcode81
runtime/lib/array.cc:81: Object& src_obj = Object::Handle();
On 2014/09/02 15:27:07, Ivan Posva wrote:
> PassiveObject& src_obj = ...
> 
> Since we do not need to setup the vtable in the handle at all.

PassiveObject& brings around 1.5x-2x improvement. Nice!

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

https://codereview.chromium.org/533483003/diff/1/runtime/vm/exceptions.cc#new...
runtime/vm/exceptions.cc:673: Exceptions::ThrowByType(Exceptions::kRange, args);
Alternatively I can just format a string like we do in other places in the
libraries.

I will take a look at what can be done here.

On 2014/09/02 15:27:07, Ivan Posva wrote:
> It would be helpful to give the expected range here as we do in other places
in
> the libraries. I do understand that this was not the case with most/all of the
> code being refactored here, but I think it would be a nice improvement.
> 
> That means that you need to add two different ThrowRangeError functions here,
> similar to the constructors of RangeError and additionally a private factory
in
> RangeError to distinguish them.

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

https://codereview.chromium.org/533483003/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:18178: void Array::CopyFrom(intptr_t dst_start,
> For copies into a newly allocated array (e.g from sublist or toList) though we
> could use a similar strategy, which might call memmove as a leaf function
> straight from recognized/intrisified code.

It is not completely clear how to implement this. I would like to avoid
intensifying this if possible. 

I see two approaches:

- could merge allocation of the Array and it's filling into a single native to
guarantee that the array instance is *new*. This is not exactly straightforward
because there are type arguments involved in the business, but I can make it
work by making this an instance method on the array that we are cloning;
- could recognize pattern in the optimizer (though currently it's a bit hard as
we don't have infrastructure for recognizing points where GC could have
occurred).

Also here is an interesting thing to consider:

final dst = new List(100000);
var pos = 0;
for (var src in lists) {
  dst.setRange(pos, pos + src.length, src);
  pos += src.length;
}

This CL, in its current form on the current VM, also improves this case - out of
the box.

[Vyacheslav Egorov (Google), 2014-09-03 13:08:36]

Please take another look.

I reverted memmove based optimization, so this CL is 99% about range errors now.

[Vyacheslav Egorov (Google), 2014-09-04 11:56:50]

ping

[Ivan Posva, 2014-09-04 15:07:35]

LGTM

Thanks!
-Ivan

[Vyacheslav Egorov (Google), 2014-09-05 13:16:09]

Committed patchset #2 (id:20001) manually as 39911 (presubmit successful).