Improve performance of addAll for internallt known lists.

Improve performance of addAll for internallt known lists.

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

[srdjan, 2015-01-23 23:33:00]

srdjan@google.com changed reviewers:
+ lrn@google.com

[srdjan, 2015-01-26 18:39:54]

srdjan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2015-01-26 20:56:38]

LGTMwS

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:156: final isList =
maybe rename isBuiltinList, isVMList?

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:171: if (isList) {
Should this block (171-180) be moved down to 182?

[srdjan, 2015-01-26 21:34:09]

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:156: final isList =
On 2015/01/26 20:56:38, Cutch wrote:
> maybe rename isBuiltinList, isVMList?

isVMList.

https://codereview.chromium.org/873863002/diff/20001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:171: if (isList) {
On 2015/01/26 20:56:38, Cutch wrote:
> Should this block (171-180) be moved down to 182?

This code can be executed only if isVMList is true (if (isVMList || iterable is
EfficientLength). If I move it out I would need to recompute 'newLen' and
'iterLen',  and would not change the behavior.

[srdjan, 2015-01-26 21:50:45]

Committed patchset #3 (id:40001) manually as r43154 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-01-27 15:13:57]

LGTM too. Sorry for being slow.

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:159: (cid == ClassID.cidImmutableArray);
Would it be worth it to recognize growable list as well in setRange?

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:173: throw new
ConcurrentModificationError(this);
This does change behavior (it now throws before adding the first element), but
nobody should be depending on that, so that's fine with me.

Of the internal lists, only GrowableObjectArray will reach here, the other ones
throw immediately when calling addAll, but the boolean is already computed, so
it's probably just as good as comparing cid to ClassID.cidGrowable....
Might be worth a comment though, so the _setLength below isn't confusing anyone
into thinking you change the length of an Array.

[srdjan, 2015-01-29 17:42:19]

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
File runtime/lib/growable_array.dart (right):

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:159: (cid == ClassID.cidImmutableArray);
On 2015/01/27 15:13:57, Lasse Reichstein Nielsen wrote:
> Would it be worth it to recognize growable list as well in setRange?

I think so, but we would need to move it here since cid-s are VM specific stuff.
Comparing cids is faster than instanceof test.

https://codereview.chromium.org/873863002/diff/40001/runtime/lib/growable_arr...
runtime/lib/growable_array.dart:173: throw new
ConcurrentModificationError(this);
On 2015/01/27 15:13:57, Lasse Reichstein Nielsen wrote:
> This does change behavior (it now throws before adding the first element), but
> nobody should be depending on that, so that's fine with me.
> 
> Of the internal lists, only GrowableObjectArray will reach here, the other
ones
> throw immediately when calling addAll, but the boolean is already computed, so
> it's probably just as good as comparing cid to ClassID.cidGrowable....
> Might be worth a comment though, so the _setLength below isn't confusing
anyone
> into thinking you change the length of an Array.

I do not understand. 'this' is always GrowableObjectArray.