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

runtime/lib/string.cc

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/bootstrap_natives.h"
 
 #include "vm/exceptions.h"
 #include "vm/native_entry.h"
 #include "vm/object.h"
 #include "vm/symbols.h"
@@ skipping 196 matching lines @@
 }
 
 
 DEFINE_NATIVE_ENTRY(String_toUpperCase, 1) {
   const String& receiver = String::CheckedHandle(arguments->NativeArgAt(0));
   ASSERT(!receiver.IsNull());
   return String::ToUpperCase(receiver);
 }
 
 
-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));
       args.SetAt(0, elem);
       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);
 }
 
 
 DEFINE_NATIVE_ENTRY(StringBuffer_createStringFromUint16Array, 3) {
   GET_NON_NULL_NATIVE_ARGUMENT(TypedData, codeUnits, arguments->NativeArgAt(0));
   GET_NON_NULL_NATIVE_ARGUMENT(Smi, length, arguments->NativeArgAt(1));
   GET_NON_NULL_NATIVE_ARGUMENT(Bool, isLatin1, arguments->NativeArgAt(2));
   intptr_t array_length = codeUnits.Length();
   intptr_t length_value = length.Value();
   if (length_value < 0 || length_value > array_length) {
     const Array& args = Array::Handle(Array::New(1));
     args.SetAt(0, length);
     Exceptions::ThrowByType(Exceptions::kRange, args);
   }
   const String& result = isLatin1.value()
       ? String::Handle(OneByteString::New(length_value, Heap::kNew))
       : String::Handle(TwoByteString::New(length_value, Heap::kNew));
   NoGCScope no_gc;
 
   uint16_t* data_position = reinterpret_cast<uint16_t*>(codeUnits.DataAddr(0));
   String::Copy(result, 0, data_position, length_value);
   return result.raw();
 }
 
 }  // namespace dart