Revert of Replace PushIfAbsent by a Stack object and move StringBuilderJoin to JS (patchset v8#6 id:100001 of https://codereview.chromium.org/1775403008/ )

mi-ac · Commit bot · commit 0ca38a6df876 · 2016-03-11T08:41:34.000Z
Reason for revert: [Sheriff] This lets a gc stress test time out: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20gc%20stress/builds/2337/steps/Mjsunit%20%28flakes%29/logs/regress-542823 The test ran in <2sec before this. Please fix the test as well on a reland. Original issue's description: > Replace PushIfAbsent by a Stack object and move StringBuilderJoin to JS > > This significantly speeds up String(array). > BUG= > > Committed: https://crrev.com/c91faa0b39b62025460eb9f8b578e20d88f3549e > Cr-Commit-Position: refs/heads/master@{#34696} TBR=cbruni@chromium.org,adamk@chromium.org,bmeurer@chromium.org,verwaest@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review URL: https://codereview.chromium.org/1785003004 Cr-Commit-Position: refs/heads/master@{#34706}
diff --git a/src/js/array.js b/src/js/array.js
@@ -74,6 +74,11 @@ function DefineIndexedProperty(array, i, value) {
 }
 
 
+// Global list of arrays visited during toString, toLocaleString and
+// join invocations.
+var visited_arrays = new InternalArray();
+
+
 // Gets a sorted array of array keys.  Useful for operations on sparse
 // arrays.  Dupes have not been removed.
 function GetSortedArrayKeys(array, indices) {
@@ -145,17 +150,6 @@ function SparseJoin(array, len, convert) {
 }
 
 
-function StringBuilderJoin(elements, length, separator) {
-  length = MinSimple(elements.length, length);
-  if (length == 0) return "";
-  var result = elements[0];
-  for (var i = 1; i < length; i++) {
-    result = result + separator + elements[i];
-  }
-  return result;
-}
-
-
 function UseSparseVariant(array, length, is_array, touched) {
   // Only use the sparse variant on arrays that are likely to be sparse and the
   // number of elements touched in the operation is relatively small compared to
@@ -173,36 +167,6 @@ function UseSparseVariant(array, length, is_array, touched) {
     (touched > estimated_elements * 4);
 }
 
-function Stack() {
-  this.length = 0;
-  this.values = new InternalArray();
-}
-
-// Predeclare the instance variables on the prototype. Otherwise setting them in
-// the constructor will leak the instance through settings on Object.prototype.
-Stack.prototype.length = null;
-Stack.prototype.values = null;
-
-function StackPush(stack, value) {
-  stack.values[stack.length++] = value;
-}
-
-function StackPop(stack) {
-  stack.values[--stack.length] = null
-}
-
-function StackHas(stack, v) {
-  var length = stack.length;
-  var values = stack.values;
-  for (var i = 0; i < length; i++) {
-    if (values[i] === v) return true;
-  }
-  return false;
-}
-
-// Global list of arrays visited during toString, toLocaleString and
-// join invocations.
-var visited_arrays = new Stack();
 
 function Join(array, length, separator, convert) {
   if (length == 0) return '';
@@ -212,8 +176,7 @@ function Join(array, length, separator, convert) {
   if (is_array) {
     // If the array is cyclic, return the empty string for already
     // visited arrays.
-    if (StackHas(visited_arrays, array)) return '';
-    StackPush(visited_arrays, array);
+    if (!%PushIfAbsent(visited_arrays, array)) return '';
   }
 
   // Attempt to convert the elements.
@@ -272,11 +235,11 @@ function Join(array, length, separator, convert) {
         elements[i] = e;
       }
     }
-    return StringBuilderJoin(elements, length, separator);
+    return %StringBuilderJoin(elements, length, separator);
   } finally {
     // Make sure to remove the last element of the visited array no
     // matter what happens.
-    if (is_array) StackPop(visited_arrays);
+    if (is_array) visited_arrays.length = visited_arrays.length - 1;
   }
 }
 
@@ -1996,10 +1959,6 @@ utils.Export(function(to) {
   to.InnerArraySort = InnerArraySort;
   to.InnerArrayToLocaleString = InnerArrayToLocaleString;
   to.PackedArrayReverse = PackedArrayReverse;
-  to.Stack = Stack;
-  to.StackHas = StackHas;
-  to.StackPush = StackPush;
-  to.StackPop = StackPop;
 });
 
 %InstallToContext([
diff --git a/src/js/json.js b/src/js/json.js
@@ -19,21 +19,13 @@ var MakeTypeError;
 var MaxSimple;
 var MinSimple;
 var ObjectHasOwnProperty;
-var Stack;
-var StackHas;
-var StackPop;
-var StackPush;
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
 
 utils.Import(function(from) {
   MakeTypeError = from.MakeTypeError;
   MaxSimple = from.MaxSimple;
   MinSimple = from.MinSimple;
   ObjectHasOwnProperty = from.ObjectHasOwnProperty;
-  Stack = from.Stack;
-  StackHas = from.StackHas;
-  StackPop = from.StackPop;
-  StackPush = from.StackPush;
 });
 
 // -------------------------------------------------------------------
@@ -86,8 +78,7 @@ function JSONParse(text, reviver) {
 
 
 function SerializeArray(value, replacer, stack, indent, gap) {
-  if (StackHas(stack, value)) throw MakeTypeError(kCircularStructure);
-  StackPush(stack, value);
+  if (!%PushIfAbsent(stack, value)) throw MakeTypeError(kCircularStructure);
   var stepback = indent;
   indent += gap;
   var partial = new InternalArray();
@@ -110,14 +101,13 @@ function SerializeArray(value, replacer, stack, indent, gap) {
   } else {
     final = "[]";
   }
-  StackPop(stack);
+  stack.pop();
   return final;
 }
 
 
 function SerializeObject(value, replacer, stack, indent, gap) {
-  if (StackHas(stack, value)) throw MakeTypeError(kCircularStructure);
-  StackPush(stack, value);
+  if (!%PushIfAbsent(stack, value)) throw MakeTypeError(kCircularStructure);
   var stepback = indent;
   indent += gap;
   var partial = new InternalArray();
@@ -156,7 +146,7 @@ function SerializeObject(value, replacer, stack, indent, gap) {
   } else {
     final = "{}";
   }
-  StackPop(stack);
+  stack.pop();
   return final;
 }
 
@@ -251,7 +241,7 @@ function JSONStringify(value, replacer, space) {
   if (!IS_CALLABLE(replacer) && !property_list && !gap && !IS_PROXY(value)) {
     return %BasicJSONStringify(value);
   }
-  return JSONSerialize('', {'': value}, replacer, new Stack(), "", gap);
+  return JSONSerialize('', {'': value}, replacer, new InternalArray(), "", gap);
 }
 
 // -------------------------------------------------------------------
@@ -289,7 +279,7 @@ function JsonSerializeAdapter(key, object) {
   var holder = {};
   holder[key] = object;
   // No need to pass the actual holder since there is no replacer function.
-  return JSONSerialize(key, holder, UNDEFINED, new Stack(), "", "");
+  return JSONSerialize(key, holder, UNDEFINED, new InternalArray(), "", "");
 }
 
 %InstallToContext(["json_serialize_adapter", JsonSerializeAdapter]);
diff --git a/src/runtime/runtime-array.cc b/src/runtime/runtime-array.cc
@@ -88,6 +88,29 @@ RUNTIME_FUNCTION(Runtime_TransitionElementsKind) {
 }
 
 
+// Push an object unto an array of objects if it is not already in the
+// array.  Returns true if the element was pushed on the stack and
+// false otherwise.
+RUNTIME_FUNCTION(Runtime_PushIfAbsent) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 2);
+  CONVERT_ARG_HANDLE_CHECKED(JSArray, array, 0);
+  CONVERT_ARG_HANDLE_CHECKED(JSReceiver, element, 1);
+  RUNTIME_ASSERT(array->HasFastSmiOrObjectElements());
+  int length = Smi::cast(array->length())->value();
+  FixedArray* elements = FixedArray::cast(array->elements());
+  for (int i = 0; i < length; i++) {
+    if (elements->get(i) == *element) return isolate->heap()->false_value();
+  }
+
+  // Strict not needed. Used for cycle detection in Array join implementation.
+  RETURN_FAILURE_ON_EXCEPTION(
+      isolate, JSObject::AddDataElement(array, length, element, NONE));
+  JSObject::ValidateElements(array);
+  return isolate->heap()->true_value();
+}
+
+
 // Moves all own elements of an object, that are below a limit, to positions
 // starting at zero. All undefined values are placed after non-undefined values,
 // and are followed by non-existing element. Does not change the length
diff --git a/src/runtime/runtime-strings.cc b/src/runtime/runtime-strings.cc
@@ -497,6 +497,89 @@ RUNTIME_FUNCTION(Runtime_StringBuilderConcat) {
 }
 
 
+RUNTIME_FUNCTION(Runtime_StringBuilderJoin) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 3);
+  CONVERT_ARG_HANDLE_CHECKED(JSArray, array, 0);
+  int32_t array_length;
+  if (!args[1]->ToInt32(&array_length)) {
+    THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewInvalidStringLengthError());
+  }
+  CONVERT_ARG_HANDLE_CHECKED(String, separator, 2);
+  RUNTIME_ASSERT(array->HasFastObjectElements());
+  RUNTIME_ASSERT(array_length >= 0);
+
+  Handle<FixedArray> fixed_array(FixedArray::cast(array->elements()));
+  if (fixed_array->length() < array_length) {
+    array_length = fixed_array->length();
+  }
+
+  if (array_length == 0) {
+    return isolate->heap()->empty_string();
+  } else if (array_length == 1) {
+    Object* first = fixed_array->get(0);
+    RUNTIME_ASSERT(first->IsString());
+    return first;
+  }
+
+  int separator_length = separator->length();
+  RUNTIME_ASSERT(separator_length > 0);
+  int max_nof_separators =
+      (String::kMaxLength + separator_length - 1) / separator_length;
+  if (max_nof_separators < (array_length - 1)) {
+    THROW_NEW_ERROR_RETURN_FAILURE(isolate, NewInvalidStringLengthError());
+  }
+  int length = (array_length - 1) * separator_length;
+  for (int i = 0; i < array_length; i++) {
+    Object* element_obj = fixed_array->get(i);
+    RUNTIME_ASSERT(element_obj->IsString());
+    String* element = String::cast(element_obj);
+    int increment = element->length();
+    if (increment > String::kMaxLength - length) {
+      STATIC_ASSERT(String::kMaxLength < kMaxInt);
+      length = kMaxInt;  // Provoke exception;
+      break;
+    }
+    length += increment;
+  }
+
+  Handle<SeqTwoByteString> answer;
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, answer, isolate->factory()->NewRawTwoByteString(length));
+
+  DisallowHeapAllocation no_gc;
+
+  uc16* sink = answer->GetChars();
+#ifdef DEBUG
+  uc16* end = sink + length;
+#endif
+
+  RUNTIME_ASSERT(fixed_array->get(0)->IsString());
+  String* first = String::cast(fixed_array->get(0));
+  String* separator_raw = *separator;
+  int first_length = first->length();
+  String::WriteToFlat(first, sink, 0, first_length);
+  sink += first_length;
+
+  for (int i = 1; i < array_length; i++) {
+    DCHECK(sink + separator_length <= end);
+    String::WriteToFlat(separator_raw, sink, 0, separator_length);
+    sink += separator_length;
+
+    RUNTIME_ASSERT(fixed_array->get(i)->IsString());
+    String* element = String::cast(fixed_array->get(i));
+    int element_length = element->length();
+    DCHECK(sink + element_length <= end);
+    String::WriteToFlat(element, sink, 0, element_length);
+    sink += element_length;
+  }
+  DCHECK(sink == end);
+
+  // Use %_FastOneByteArrayJoin instead.
+  DCHECK(!answer->IsOneByteRepresentation());
+  return *answer;
+}
+
 template <typename Char>
 static void JoinSparseArrayWithSeparator(FixedArray* elements,
                                          int elements_length,
diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h
@@ -35,6 +35,7 @@ namespace internal {
   F(FinishArrayPrototypeSetup, 1, 1) \
   F(SpecialArrayFunctions, 0, 1)     \
   F(TransitionElementsKind, 2, 1)    \
+  F(PushIfAbsent, 2, 1)              \
   F(RemoveArrayHoles, 2, 1)          \
   F(MoveArrayContents, 2, 1)         \
   F(EstimateNumberOfElements, 1, 1)  \
@@ -850,6 +851,7 @@ namespace internal {
   F(StringCharCodeAtRT, 2, 1)             \
   F(StringCompare, 2, 1)                  \
   F(StringBuilderConcat, 3, 1)            \
+  F(StringBuilderJoin, 3, 1)              \
   F(SparseJoinWithSeparator, 3, 1)        \
   F(StringToArray, 2, 1)                  \
   F(StringToLowerCase, 1, 1)              \
diff --git a/test/mjsunit/json-stringify-stack.js b/test/mjsunit/json-stringify-stack.js
