Improve performance of string buffer by modifying concatAll native to allow growable array and an i…

Improve performance of string buffer by modifying concatAll native to allow growable array and an interval. Eliminate unnecessary check for String elements in release mode.

R=asiva@google.com

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

[kasperl, 2013-09-30 16:37:26]

StringBuffer changes STV!

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:103: if (_partsCodeUnits == 0) return "";
Maybe use 'return ?:' now that the code is a lot simpler?

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:160: );
Not sure how you usually indent such a hanging ). This looks a little bit off.

[srdjan, 2013-09-30 16:46:27]

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:103: if (_partsCodeUnits == 0) return "";
On 2013/09/30 16:37:27, kasperl wrote:
> Maybe use 'return ?:' now that the code is a lot simpler?

Done.

https://codereview.chromium.org/25087006/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:160: );
On 2013/09/30 16:37:27, kasperl wrote:
> Not sure how you usually indent such a hanging ). This looks a little bit off.

Done.

[sra1, 2013-09-30 17:54:43]

DBC

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:234: Exceptions::ThrowByType(Exceptions::kArgument, args);
This is such a common sequence I am surprised it is not a helper function or
macro.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:237: // Allocate new array and copy data into it.
Comment is wrong - you don't copy the the array.
You do the right thing and use the underlying array.

[siva, 2013-09-30 18:27:21]

lgtm

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:239: g ^= argument.raw();
'g' seems like a cryptic name, why not

strings = GrowaleObjectArray::Handle(argument.raw()).data();

Also the comment is incorrect, no new array is allocated.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:250: }
The
if (end_ix > length) {
  ....
  ....
}
blob can be moved down here from the two places above, if we use a length
variable.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:262: #endif
Why does this check have to be done in DEBUG mode? We would get
errors in checked mode right because you have declared the list
as List<String> now.

It seems odd that one could get Exceptions in debug mode but not
in release mode. Maybe if you really want it change the for loop
to just ASSERT, i.e:
for (...) {
  ....
  ASSERT(elem.IsString());
}

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string_patch....
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string_patch....
runtime/lib/string_patch.dart:500: // Call this method if not all list elements
are known to be OneByteString-s.
should this be OneByteString(s).

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

https://codereview.chromium.org/25087006/diff/22001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:13288: 
extra blank line.

https://codereview.chromium.org/25087006/diff/22001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:13983: intptr_t str_len = str.Length();
const intrptr_t str_len

[srdjan, 2013-10-01 14:57:44]

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:234: Exceptions::ThrowByType(Exceptions::kArgument, args);
On 2013/09/30 17:54:43, sra1 wrote:
> This is such a common sequence I am surprised it is not a helper function or
> macro.

Will do that in next CL.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:237: // Allocate new array and copy data into it.
On 2013/09/30 17:54:43, sra1 wrote:
> Comment is wrong - you don't copy the the array.
> You do the right thing and use the underlying array.

Removed comment.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:239: g ^= argument.raw();
On 2013/09/30 18:27:21, siva wrote:
> 'g' seems like a cryptic name, why not
> 
> strings = GrowaleObjectArray::Handle(argument.raw()).data();
> 
> Also the comment is incorrect, no new array is allocated.

strings is the backing store array whose length is the capacity not the length.
Changing:
const GrowableObjectArray& g_array = GrowableObjectArray::Cast(argument);

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:250: }
On 2013/09/30 18:27:21, siva wrote:
> The
> if (end_ix > length) {
>   ....
>   ....
> }
> blob can be moved down here from the two places above, if we use a length
> variable.

Yes.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string.cc#new...
runtime/lib/string.cc:262: #endif
On 2013/09/30 18:27:21, siva wrote:
> Why does this check have to be done in DEBUG mode? We would get
> errors in checked mode right because you have declared the list
> as List<String> now.
> 
> It seems odd that one could get Exceptions in debug mode but not
> in release mode. Maybe if you really want it change the for loop
> to just ASSERT, i.e:
> for (...) {
>   ....
>   ASSERT(elem.IsString());
> }

Changing to ASSERT.

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string_patch....
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/25087006/diff/22001/runtime/lib/string_patch....
runtime/lib/string_patch.dart:500: // Call this method if not all list elements
are known to be OneByteString-s.
On 2013/09/30 18:27:21, siva wrote:
> should this be OneByteString(s).

Done.

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

https://codereview.chromium.org/25087006/diff/22001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:13288: 
On 2013/09/30 18:27:21, siva wrote:
> extra blank line.

Done.

https://codereview.chromium.org/25087006/diff/22001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:13983: intptr_t str_len = str.Length();
On 2013/09/30 18:27:21, siva wrote:
> const intrptr_t str_len

Done.

[srdjan, 2013-10-01 16:49:02]

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