Optimize _GrowableArray._join and _StringBase._interpolate.

runtime/lib/string_patch.dart

Index: runtime/lib/string_patch.dart
diff --git a/runtime/lib/string_patch.dart b/runtime/lib/string_patch.dart
index 3ac78a8fd9404faa5724cd771666eb61053110d5..98d2cfce803a8cf7daaf485153eaaaa042bf473d 100644
--- a/runtime/lib/string_patch.dart
+++ b/runtime/lib/string_patch.dart
@@ -518,32 +518,59 @@ class _StringBase {
     return buffer.toString();
   }
 
-
   /**
    * Convert all objects in [values] to strings and concat them
    * into a result string.
+   * Modifies the input list if it contains non-`String` values.
    */
-  static String _interpolate(List<String> values) {
+  static String _interpolate(final List values) {
     final numValues = values.length;
-    _List stringList = new List<String>(numValues);
-    bool isOneByteString = true;
+    if (numValues <= 2) {
+      // If possible, do this optimization at compiler level and call a
+      // specialized _interpolateSingle when there is only one argument,
+      // and _interpolateTwo with two.
+      // Call here only with three or more arguments.
+      // That would avoid allocating a list for the values if there are
+      // only one or two.
+      if (numValues == 0) return "";
+      final first = values[0];
+      final firstString = first.toString();
+      if (firstString is! String) {
+        throw new ArgumentError(first);
+      }
+      if (numValues == 1) return firstString;
+      final second = values[1];
+      final secondString = second.toString();
+      if (secondString is! String) {
+        throw new ArgumentError(second);
+      }
+      return firstString + secondString;
+    }
     int totalLength = 0;
     for (int i = 0; i < numValues; i++) {
-      var s = values[i].toString();
-      if (isOneByteString && (ClassID.getID(s) == ClassID.cidOneByteString)) {
+      final e = values[i];
+      final s = e.toString();
+      values[i] = s;
+      if (ClassID.getID(s) == ClassID.cidOneByteString) {
         totalLength += s.length;
-      } else {
-        isOneByteString = false;
+        continue;
+      }
+      // Loops while the values convert to one-byte strings.
+      if (s is! String) {
+        throw new ArgumentError(s);
+      }
+      i++;
+      for (;i < numValues; i++) {
+        final e = values[i];
+        final s = e.toString();
+        values[i] = s;
         if (s is! String) {
           throw new ArgumentError(s);
         }
       }
-      stringList[i] = s;
+      return _concatRangeNative(values, 0, numValues);
     }
-    if (isOneByteString) {
-      return _OneByteString._concatAll(stringList, totalLength);
-    }
-    return _concatRangeNative(stringList, 0, stringList.length);
+    return _OneByteString._concatAll(values, totalLength);
   }
 
   Iterable<Match> allMatches(String string, [int start = 0]) {
@@ -661,6 +688,29 @@ class _OneByteString extends _StringBase implements String {
     return super == other;
   }
 
+  String operator +(String other) {

[Florian Schneider, 2014/09/11 13:27:36]

I'm not convinced about this optimization: Maybe the native operator+ defined in
StringBase should be optimized instead by "copying" it down to all sub-classes
of StringBase and optimized for the argument type by the compiler.

[Lasse Reichstein Nielsen, 2014/09/11 14:15:05]

If you can do compiler magic to do better, by all means do :)

+    if (ClassID.getID(other) != ClassID.cidExternalOneByteString) {

[Florian Schneider, 2014/09/11 13:27:36]

Why cidExternalOneByteString? Shouldn't it be cidOneByteString?

[Lasse Reichstein Nielsen, 2014/09/11 14:15:05]

Ack. I had both at some time, and apparently deleted the wrong one.
That also explains why the errors below wasn't caught in tests - no external
strings in the tests.

This also improves performance, now that the code actually runs.

+      return super + other;
+    }
+    final firstLength = this.length;
+    final secondLength = other.length;
+    final length = firstLength + secondLength;
+    if (length > 128) {
+      final list = new _List(2);
+      list[0] = this;
+      list[1] = other;
+      return _concatAll(list, 0, 2);
+    }
+    final _OneByteString result = _allocate(length);
+    for (int i = 0; i < firstLength; i++) {
+      _setAt(result, i, this.codeUnitAt(i));
+    }
+    for (int i = 0; i < secondLength; i++) {

[Florian Schneider, 2014/09/11 13:27:36]

I'd rename secondLength to otherLength. s/firstLength/thisLength, too.

[Lasse Reichstein Nielsen, 2014/09/11 14:15:05]

Done.

+      _setAt(result, firstLength + i, this.codeUnitAt(i));

[Florian Schneider, 2014/09/11 13:27:36]

s/this/other/

[Lasse Reichstein Nielsen, 2014/09/11 14:15:05]

Done.

+    }
+    return result;
+  }
+
   String _substringUncheckedNative(int startIndex, int endIndex)
       native "OneByteString_substringUnchecked";
 
@@ -682,11 +732,11 @@ class _OneByteString extends _StringBase implements String {
       // Native is quicker.
       return _StringBase._concatRangeNative(strings, 0, strings.length);
     }
-    var res = _OneByteString._allocate(totalLength);
+    final res = _OneByteString._allocate(totalLength);
     final stringsLength = strings.length;
     int rIx = 0;
     for (int i = 0; i < stringsLength; i++) {
-      _OneByteString e = strings[i];
+      final _OneByteString e = strings[i];
       final eLength = e.length;
       for (int s = 0; s < eLength; s++) {
         res._setAt(rIx++, e.codeUnitAt(s));