Revert "[runtime] Improve for-in performance"

This reverts commit 8fa7f9ed93.

Reason for revert: Speculating that this breaks GC stress

Original change's description:
> [runtime] Improve for-in performance
> 
> - Add fast-path String conversion for Smi (which is the most common case)
>   This improves for-in by ~10% on non-initialized enum-caches
> - Don't use the NumberStringCache for large indices to not overflow the cache
>   during key collection. This improves worst-case performance by ~2.5x
> - Drop number_to_string_native and number_to_string_runtime counters
> 
> Bug: v8:7717
> Change-Id: Ic1ff385e3374e6a7e7e7bdb9ae75fb8c238105d1
> Reviewed-on: https://chromium-review.googlesource.com/1167049
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Commit-Queue: Camillo Bruni <cbruni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#55233}

TBR=ulan@chromium.org,cbruni@chromium.org,verwaest@chromium.org

Change-Id: I8d0332478afcd7c6a3f8fbf1f044b9aa870b6b13
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:7717
Reviewed-on: https://chromium-review.googlesource.com/1182676
Reviewed-by: Aseem Garg <aseemgarg@chromium.org>
Commit-Queue: Aseem Garg <aseemgarg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55241}

src/code-stub-assembler.cc

@@ -6549,6 +6549,7 @@ TNode<String> CodeStubAssembler::NumberToString(TNode<Number> input) {
   GotoIfNot(Word32Equal(high, high_compare), &runtime);
 
   // Heap number match, return value from cache entry.
+  IncrementCounter(isolate()->counters()->number_to_string_native(), 1);
   result = CAST(
       LoadFixedArrayElement(CAST(number_string_cache), index, kPointerSize));
   Goto(&done);
@@ -6571,6 +6572,7 @@ TNode<String> CodeStubAssembler::NumberToString(TNode<Number> input) {
     GotoIf(WordNotEqual(smi_key, input), &runtime);
 
     // Smi match, return value from cache entry.
+    IncrementCounter(isolate()->counters()->number_to_string_native(), 1);
     result = CAST(LoadFixedArrayElement(CAST(number_string_cache), smi_index,
                                         kPointerSize, SMI_PARAMETERS));
     Goto(&done);

src/counters.h

@@ -1377,6 +1377,8 @@ class RuntimeCallTimerScope {
   SC(sub_string_native, V8.SubStringNative)                                    \
   SC(regexp_entry_runtime, V8.RegExpEntryRuntime)                              \
   SC(regexp_entry_native, V8.RegExpEntryNative)                                \
+  SC(number_to_string_native, V8.NumberToStringNative)                         \
+  SC(number_to_string_runtime, V8.NumberToStringRuntime)                       \
   SC(math_exp_runtime, V8.MathExpRuntime)                                      \
   SC(math_log_runtime, V8.MathLogRuntime)                                      \
   SC(math_pow_runtime, V8.MathPowRuntime)                                      \

src/elements.cc

@@ -1173,15 +1173,11 @@ class ElementsAccessorBase : public InternalElementsAccessor {
       PropertyFilter filter, Handle<FixedArray> list, uint32_t* nof_indices,
       uint32_t insertion_index = 0) {
     uint32_t length = Subclass::GetMaxIndex(*object, *backing_store);
-    uint32_t const kMaxStringTableEntries =
-        isolate->heap()->MaxNumberToStringCacheSize();
     for (uint32_t i = 0; i < length; i++) {
       if (Subclass::HasElementImpl(isolate, *object, i, *backing_store,
                                    filter)) {
         if (convert == GetKeysConversion::kConvertToString) {
-          bool use_cache = i < kMaxStringTableEntries;
-          Handle<String> index_string =
-              isolate->factory()->Uint32ToString(i, use_cache);
+          Handle<String> index_string = isolate->factory()->Uint32ToString(i);
           list->set(insertion_index, *index_string);
         } else {
           list->set(insertion_index, Smi::FromInt(i), SKIP_WRITE_BARRIER);

src/heap/factory-inl.h

@@ -164,14 +164,8 @@ Handle<Object> Factory::NewURIError() {
                   MessageTemplate::kURIMalformed);
 }
 
-Handle<String> Factory::Uint32ToString(uint32_t value, bool check_cache) {
-  Handle<String> result;
-  int32_t int32v = static_cast<int32_t>(value);
-  if (int32v >= 0 && Smi::IsValid(int32v)) {
-    result = NumberToString(Smi::FromInt(int32v), check_cache);
-  } else {
-    result = NumberToString(NewNumberFromUint(value), check_cache);
-  }
+Handle<String> Factory::Uint32ToString(uint32_t value) {
+  Handle<String> result = NumberToString(NewNumberFromUint(value));
 
   if (result->length() <= String::kMaxArrayIndexSize &&
       result->hash_field() == String::kEmptyHashField) {

src/heap/factory.cc

@@ -3539,90 +3539,68 @@ Handle<SharedFunctionInfo> Factory::NewSharedFunctionInfo(
   return share;
 }
 
-namespace {
-inline int NumberToStringCacheHash(Handle<FixedArray> cache, Smi* number) {
+static inline int NumberCacheHash(Handle<FixedArray> cache,
+                                  Handle<Object> number) {
   int mask = (cache->length() >> 1) - 1;
-  return number->value() & mask;
-}
-inline int NumberToStringCacheHash(Handle<FixedArray> cache, double number) {
-  int mask = (cache->length() >> 1) - 1;
-  int64_t bits = bit_cast<int64_t>(number);
-  return (static_cast<int>(bits) ^ static_cast<int>(bits >> 32)) & mask;
-}
-}  // namespace
-
-Handle<String> Factory::NumberToStringCacheSet(Handle<Object> number, int hash,
-                                               const char* string,
-                                               bool check_cache) {
-  // We tenure the allocated string since it is referenced from the
-  // number-string cache which lives in the old space.
-  Handle<String> js_string =
-      NewStringFromAsciiChecked(string, check_cache ? TENURED : NOT_TENURED);
-  if (!check_cache) return js_string;
-
-  if (!number_string_cache()->get(hash * 2)->IsUndefined(isolate())) {
-    int full_size = isolate()->heap()->MaxNumberToStringCacheSize();
-    if (number_string_cache()->length() != full_size) {
-      Handle<FixedArray> new_cache = NewFixedArray(full_size, TENURED);
-      isolate()->heap()->set_number_string_cache(*new_cache);
-      return js_string;
-    }
+  if (number->IsSmi()) {
+    return Handle<Smi>::cast(number)->value() & mask;
+  } else {
+    int64_t bits = bit_cast<int64_t>(number->Number());
+    return (static_cast<int>(bits) ^ static_cast<int>(bits >> 32)) & mask;
   }
-  number_string_cache()->set(hash * 2, *number);
-  number_string_cache()->set(hash * 2 + 1, *js_string);
-  return js_string;
 }
 
-Handle<Object> Factory::NumberToStringCacheGet(Object* number, int hash) {
+Handle<Object> Factory::GetNumberStringCache(Handle<Object> number) {
   DisallowHeapAllocation no_gc;
+  int hash = NumberCacheHash(number_string_cache(), number);
   Object* key = number_string_cache()->get(hash * 2);
-  if (key == number || (key->IsHeapNumber() && number->IsHeapNumber() &&
-                        key->Number() == number->Number())) {
+  if (key == *number || (key->IsHeapNumber() && number->IsHeapNumber() &&
+                         key->Number() == number->Number())) {
     return Handle<String>(
         String::cast(number_string_cache()->get(hash * 2 + 1)), isolate());
   }
   return undefined_value();
 }
 
-Handle<String> Factory::NumberToString(Handle<Object> number,
-                                       bool check_cache) {
-  if (number->IsSmi()) return NumberToString(Smi::cast(*number), check_cache);
-
-  double double_value = Handle<HeapNumber>::cast(number)->value();
-  // Try to canonicalize doubles.
-  int smi_value;
-  if (DoubleToSmiInteger(double_value, &smi_value)) {
-    return NumberToString(Smi::FromInt(smi_value), check_cache);
+void Factory::SetNumberStringCache(Handle<Object> number,
+                                   Handle<String> string) {
+  int hash = NumberCacheHash(number_string_cache(), number);
+  if (number_string_cache()->get(hash * 2) != *undefined_value()) {
+    int full_size = isolate()->heap()->FullSizeNumberStringCacheLength();
+    if (number_string_cache()->length() != full_size) {
+      Handle<FixedArray> new_cache = NewFixedArray(full_size, TENURED);
+      isolate()->heap()->set_number_string_cache(*new_cache);
+      return;
+    }
   }
-
-  int hash = 0;
-  if (check_cache) {
-    int hash = NumberToStringCacheHash(number_string_cache(), double_value);
-    Handle<Object> cached = NumberToStringCacheGet(*number, hash);
-    if (!cached->IsUndefined(isolate())) return Handle<String>::cast(cached);
-  }
-
-  char arr[100];
-  Vector<char> buffer(arr, arraysize(arr));
-  const char* string = DoubleToCString(double_value, buffer);
-
-  return NumberToStringCacheSet(number, hash, string, check_cache);
+  number_string_cache()->set(hash * 2, *number);
+  number_string_cache()->set(hash * 2 + 1, *string);
 }
 
-Handle<String> Factory::NumberToString(Smi* number, bool check_cache) {
-  int hash = 0;
-  if (check_cache) {
-    hash = NumberToStringCacheHash(number_string_cache(), number);
-    Handle<Object> cached = NumberToStringCacheGet(number, hash);
+Handle<String> Factory::NumberToString(Handle<Object> number,
+                                       bool check_number_string_cache) {
+  isolate()->counters()->number_to_string_runtime()->Increment();
+  if (check_number_string_cache) {
+    Handle<Object> cached = GetNumberStringCache(number);
     if (!cached->IsUndefined(isolate())) return Handle<String>::cast(cached);
   }
 
   char arr[100];
   Vector<char> buffer(arr, arraysize(arr));
-  const char* string = IntToCString(number->value(), buffer);
+  const char* str;
+  if (number->IsSmi()) {
+    int num = Handle<Smi>::cast(number)->value();
+    str = IntToCString(num, buffer);
+  } else {
+    double num = Handle<HeapNumber>::cast(number)->value();
+    str = DoubleToCString(num, buffer);
+  }
 
-  return NumberToStringCacheSet(handle(number, isolate()), hash, string,
-                                check_cache);
+  // We tenure the allocated string since it is referenced from the
+  // number-string cache which lives in the old space.
+  Handle<String> js_string = NewStringFromAsciiChecked(str, TENURED);
+  SetNumberStringCache(number, js_string);
+  return js_string;
 }
 
 Handle<DebugInfo> Factory::NewDebugInfo(Handle<SharedFunctionInfo> shared) {

src/heap/factory.h

@@ -822,11 +822,10 @@ class V8_EXPORT_PRIVATE Factory {
   DECLARE_ERROR(WasmRuntimeError)
 #undef DECLARE_ERROR
 
-  Handle<String> NumberToString(Handle<Object> number, bool check_cache = true);
-  Handle<String> NumberToString(Smi* number, bool check_cache = true);
+  Handle<String> NumberToString(Handle<Object> number,
+                                bool check_number_string_cache = true);
 
-  inline Handle<String> Uint32ToString(uint32_t value,
-                                       bool check_cache = false);
+  inline Handle<String> Uint32ToString(uint32_t value);
 
 #define ROOT_ACCESSOR(type, name, camel_name) inline Handle<type> name();
   ROOT_LIST(ROOT_ACCESSOR)
@@ -1001,11 +1000,10 @@ class V8_EXPORT_PRIVATE Factory {
 
   // Attempt to find the number in a small cache.  If we finds it, return
   // the string representation of the number.  Otherwise return undefined.
-  Handle<Object> NumberToStringCacheGet(Object* number, int hash);
+  Handle<Object> GetNumberStringCache(Handle<Object> number);
 
   // Update the cache with a new number-string pair.
-  Handle<String> NumberToStringCacheSet(Handle<Object> number, int hash,
-                                        const char* string, bool check_cache);
+  void SetNumberStringCache(Handle<Object> number, Handle<String> string);
 
   // Create a JSArray with no elements and no length.
   Handle<JSArray> NewJSArray(ElementsKind elements_kind,

src/heap/heap-inl.h

@@ -569,18 +569,6 @@ int Heap::GetNextTemplateSerialNumber() {
   return next_serial_number;
 }
 
-int Heap::MaxNumberToStringCacheSize() const {
-  // Compute the size of the number string cache based on the max newspace size.
-  // The number string cache has a minimum size based on twice the initial cache
-  // size to ensure that it is bigger after being made 'full size'.
-  size_t number_string_cache_size = max_semi_space_size_ / 512;
-  number_string_cache_size =
-      Max(static_cast<size_t>(kInitialNumberStringCacheSize * 2),
-          Min<size_t>(0x4000u, number_string_cache_size));
-  // There is a string and a number per entry so the length is twice the number
-  // of entries.
-  return static_cast<int>(number_string_cache_size * 2);
-}
 AlwaysAllocateScope::AlwaysAllocateScope(Isolate* isolate)
     : heap_(isolate->heap()) {
   heap_->always_allocate_scope_count_++;

src/heap/heap.cc

@@ -2722,6 +2722,19 @@ bool Heap::RootCanBeTreatedAsConstant(RootListIndex root_index) {
   return can_be;
 }
 
+int Heap::FullSizeNumberStringCacheLength() {
+  // Compute the size of the number string cache based on the max newspace size.
+  // The number string cache has a minimum size based on twice the initial cache
+  // size to ensure that it is bigger after being made 'full size'.
+  size_t number_string_cache_size = max_semi_space_size_ / 512;
+  number_string_cache_size =
+      Max(static_cast<size_t>(kInitialNumberStringCacheSize * 2),
+          Min<size_t>(0x4000u, number_string_cache_size));
+  // There is a string and a number per entry so the length is twice the number
+  // of entries.
+  return static_cast<int>(number_string_cache_size * 2);
+}
+
 
 void Heap::FlushNumberStringCache() {
   // Flush the number to string cache.

src/heap/heap.h

@@ -1480,9 +1480,6 @@ class Heap {
   static const char* GarbageCollectionReasonToString(
       GarbageCollectionReason gc_reason);
 
-  // Calculates the nof entries for the full sized number to string cache.
-  inline int MaxNumberToStringCacheSize() const;
-
  private:
   class SkipStoreBufferScope;
 
@@ -1691,6 +1688,8 @@ class Heap {
   // Record statistics after garbage collection.
   void ReportStatisticsAfterGC();
 
+  // Creates and installs the full-sized number string cache.
+  int FullSizeNumberStringCacheLength();
   // Flush the number to string cache.
   void FlushNumberStringCache();
 

src/objects/ordered-hash-table.cc

@@ -126,17 +126,13 @@ Handle<FixedArray> OrderedHashSet::ConvertToKeysArray(
   Handle<FixedArray> result = Handle<FixedArray>::cast(table);
   // From this point on table is no longer a valid OrderedHashSet.
   result->set_map(ReadOnlyRoots(isolate).fixed_array_map());
-  int const kMaxStringTableEntries =
-      isolate->heap()->MaxNumberToStringCacheSize();
   for (int i = 0; i < length; i++) {
     int index = kHashTableStartIndex + nof_buckets + (i * kEntrySize);
     Object* key = table->get(index);
     if (convert == GetKeysConversion::kConvertToString) {
       uint32_t index_value;
       if (key->ToArrayIndex(&index_value)) {
-        // Avoid trashing the Number2String cache if indices get very large.
-        bool use_cache = i < kMaxStringTableEntries;
-        key = *isolate->factory()->Uint32ToString(index_value, use_cache);
+        key = *isolate->factory()->Uint32ToString(index_value);
       } else {
         CHECK(key->IsName());
       }