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

runtime/lib/string.cc

Index: runtime/lib/string.cc
===================================================================
--- runtime/lib/string.cc	(revision 28056)
+++ runtime/lib/string.cc	(working copy)
@@ -214,12 +214,44 @@
 }
 
 
-DEFINE_NATIVE_ENTRY(Strings_concatAll, 1) {
-  GET_NON_NULL_NATIVE_ARGUMENT(Array, strings, arguments->NativeArgAt(0));
-  ASSERT(!strings.IsNull());
+DEFINE_NATIVE_ENTRY(Strings_concatAll, 3) {
+  GET_NON_NULL_NATIVE_ARGUMENT(Instance, argument, arguments->NativeArgAt(0));
+  GET_NON_NULL_NATIVE_ARGUMENT(Smi, start, arguments->NativeArgAt(1));
+  GET_NON_NULL_NATIVE_ARGUMENT(Smi, end, arguments->NativeArgAt(2));
+  const intptr_t start_ix = start.Value();
+  const intptr_t end_ix = end.Value();
+  if (start_ix < 0) {
+    const Array& args = Array::Handle(Array::New(1));
+    args.SetAt(0, start);
+    Exceptions::ThrowByType(Exceptions::kArgument, args);
+  }
+  Array& strings = Array::Handle();
+  if (argument.IsArray()) {
+    strings ^= argument.raw();
+    if (end_ix > strings.Length()) {
+      const Array& args = Array::Handle(Array::New(1));
+      args.SetAt(0, end);
+      Exceptions::ThrowByType(Exceptions::kArgument, args);

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

This is such a common sequence I am surprised it is not a helper function or
macro.

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

Will do that in next CL.

+    }
+  } else if (argument.IsGrowableObjectArray()) {
+    // Allocate new array and copy data into it.

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

Comment is wrong - you don't copy the the array.
You do the right thing and use the underlying array.

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

Removed comment.

+    GrowableObjectArray& g = GrowableObjectArray::Handle();
+    g ^= argument.raw();

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

'g' seems like a cryptic name, why not

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

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

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

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

+    strings = g.data();
+    if (end_ix > g.Length()) {
+      const Array& args = Array::Handle(Array::New(1));
+      args.SetAt(0, end);
+      Exceptions::ThrowByType(Exceptions::kArgument, args);
+    }
+  } else {
+    const Array& args = Array::Handle(Array::New(1));
+    args.SetAt(0, argument);
+    Exceptions::ThrowByType(Exceptions::kArgument, args);
+  }

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

The
if (end_ix > length) {
  ....
  ....
}
blob can be moved down here from the two places above, if we use a length
variable.

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

Yes.

+#if defined(DEBUG)
   // Check that the array contains strings.
   Instance& elem = Instance::Handle();
-  for (intptr_t i = 0; i < strings.Length(); i++) {
+  for (intptr_t i = start_ix; i < end_ix; i++) {
     elem ^= strings.At(i);
     if (!elem.IsString()) {
       const Array& args = Array::Handle(Array::New(1));
@@ -227,7 +259,8 @@
       Exceptions::ThrowByType(Exceptions::kArgument, args);
     }
   }
-  return String::ConcatAll(strings);
+#endif

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

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());
}

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

Changing to ASSERT.

+  return String::ConcatAllRange(strings, start_ix, end_ix, Heap::kNew);
 }