From: danno@chromium.org <danno@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Date: Thu, 26 Jun 2014 00:40:45 +0000 (+0000)
Subject: Optimize Map/Set.prototype.forEach
X-Git-Tag: upstream/4.7.83~8527
X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=8313c523b357ab6b7caacc1db12b3f266c31ae07;p=platform%2Fupstream%2Fv8.git

Optimize Map/Set.prototype.forEach

Instead of using an iterator result object and an entries array
(for Map) we introduce a new runtime function that uses an array
as an out param.

On the Map ForEach perf test this leads to a 2.5x performance
improvement. On the overall Map and Set tests this leads to a 18%
and 13% improvement respectively.

BUG=None
LOG=Y
R=danno@chromium.org

Review URL: https://codereview.chromium.org/355663002

Patch from Erik Arvidsson <arv@chromium.org>.

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@22027 ce2b1a6d-e550-0410-aec6-3dcde31c8c00
---

diff --git a/src/collection-iterator.js b/src/collection-iterator.js
index 2436a93..545a345 100644
--- a/src/collection-iterator.js
+++ b/src/collection-iterator.js
@@ -21,7 +21,23 @@ function SetIteratorNextJS() {
     throw MakeTypeError('incompatible_method_receiver',
                         ['Set Iterator.prototype.next', this]);
   }
-  return %SetIteratorNext(this);
+
+  var value_array = [UNDEFINED, UNDEFINED];
+  var entry = {value: value_array, done: false};
+  switch (%SetIteratorNext(this, value_array)) {
+    case 0:
+      entry.value = UNDEFINED;
+      entry.done = true;
+      break;
+    case ITERATOR_KIND_VALUES:
+      entry.value = value_array[0];
+      break;
+    case ITERATOR_KIND_ENTRIES:
+      value_array[1] = value_array[0];
+      break;
+  }
+
+  return entry;
 }
 
 
@@ -97,7 +113,24 @@ function MapIteratorNextJS() {
     throw MakeTypeError('incompatible_method_receiver',
                         ['Map Iterator.prototype.next', this]);
   }
-  return %MapIteratorNext(this);
+
+  var value_array = [UNDEFINED, UNDEFINED];
+  var entry = {value: value_array, done: false};
+  switch (%MapIteratorNext(this, value_array)) {
+    case 0:
+      entry.value = UNDEFINED;
+      entry.done = true;
+      break;
+    case ITERATOR_KIND_KEYS:
+      entry.value = value_array[0];
+      break;
+    case ITERATOR_KIND_VALUES:
+      entry.value = value_array[1];
+      break;
+    // ITERATOR_KIND_ENTRIES does not need any processing.
+  }
+
+  return entry;
 }
 
 
diff --git a/src/collection.js b/src/collection.js
index 8ddf8e1..01e7c53 100644
--- a/src/collection.js
+++ b/src/collection.js
@@ -129,11 +129,13 @@ function SetForEach(f, receiver) {
   }
 
   var iterator = new SetIterator(this, ITERATOR_KIND_VALUES);
-  var entry;
+  var key;
   var stepping = DEBUG_IS_ACTIVE && %DebugCallbackSupportsStepping(f);
-  while (!(entry = %SetIteratorNext(iterator)).done) {
+  var value_array = [UNDEFINED];
+  while (%SetIteratorNext(iterator, value_array)) {
     if (stepping) %DebugPrepareStepInIfStepping(f);
-    %_CallFunction(receiver, entry.value, entry.value, this, f);
+    key = value_array[0];
+    %_CallFunction(receiver, key, key, this, f);
   }
 }
 
@@ -264,11 +266,11 @@ function MapForEach(f, receiver) {
   }
 
   var iterator = new MapIterator(this, ITERATOR_KIND_ENTRIES);
-  var entry;
   var stepping = DEBUG_IS_ACTIVE && %DebugCallbackSupportsStepping(f);
-  while (!(entry = %MapIteratorNext(iterator)).done) {
+  var value_array = [UNDEFINED, UNDEFINED];
+  while (%MapIteratorNext(iterator, value_array)) {
     if (stepping) %DebugPrepareStepInIfStepping(f);
-    %_CallFunction(receiver, entry.value[1], entry.value[0], this, f);
+    %_CallFunction(receiver, value_array[1], value_array[0], this, f);
   }
 }
 
diff --git a/src/factory.cc b/src/factory.cc
index 6618357..29e396b 100644
--- a/src/factory.cc
+++ b/src/factory.cc
@@ -1394,18 +1394,6 @@ Handle<JSFunction> Factory::NewFunctionFromSharedFunctionInfo(
 }
 
 
-Handle<JSObject> Factory::NewIteratorResultObject(Handle<Object> value,
-                                                     bool done) {
-  Handle<Map> map(isolate()->native_context()->iterator_result_map());
-  Handle<JSObject> result = NewJSObjectFromMap(map, NOT_TENURED, false);
-  result->InObjectPropertyAtPut(
-      JSGeneratorObject::kResultValuePropertyIndex, *value);
-  result->InObjectPropertyAtPut(
-      JSGeneratorObject::kResultDonePropertyIndex, *ToBoolean(done));
-  return result;
-}
-
-
 Handle<ScopeInfo> Factory::NewScopeInfo(int length) {
   Handle<FixedArray> array = NewFixedArray(length, TENURED);
   array->set_map_no_write_barrier(*scope_info_map());
diff --git a/src/factory.h b/src/factory.h
index e22ea8d..cad25b5 100644
--- a/src/factory.h
+++ b/src/factory.h
@@ -551,8 +551,6 @@ class Factory V8_FINAL {
   Handle<Object> NewEvalError(const char* message,
                               Vector< Handle<Object> > args);
 
-  Handle<JSObject> NewIteratorResultObject(Handle<Object> value, bool done);
-
   Handle<String> NumberToString(Handle<Object> number,
                                 bool check_number_string_cache = true);
 
diff --git a/src/objects-inl.h b/src/objects-inl.h
index eeec572..812bc18 100644
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -6964,6 +6964,36 @@ void FlexibleBodyDescriptor<start_offset>::IterateBody(HeapObject* obj,
 }
 
 
+template<class Derived, class TableType>
+Object* OrderedHashTableIterator<Derived, TableType>::CurrentKey() {
+  TableType* table(TableType::cast(this->table()));
+  int index = Smi::cast(this->index())->value();
+  Object* key = table->KeyAt(index);
+  ASSERT(!key->IsTheHole());
+  return key;
+}
+
+
+void JSSetIterator::PopulateValueArray(FixedArray* array) {
+  array->set(0, CurrentKey());
+}
+
+
+void JSMapIterator::PopulateValueArray(FixedArray* array) {
+  array->set(0, CurrentKey());
+  array->set(1, CurrentValue());
+}
+
+
+Object* JSMapIterator::CurrentValue() {
+  OrderedHashMap* table(OrderedHashMap::cast(this->table()));
+  int index = Smi::cast(this->index())->value();
+  Object* value = table->ValueAt(index);
+  ASSERT(!value->IsTheHole());
+  return value;
+}
+
+
 #undef TYPE_CHECKER
 #undef CAST_ACCESSOR
 #undef INT_ACCESSORS
diff --git a/src/objects.cc b/src/objects.cc
index 08a192f..6a437d6 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -16248,47 +16248,14 @@ Handle<OrderedHashMap> OrderedHashMap::Put(Handle<OrderedHashMap> table,
 
 
 template<class Derived, class TableType>
-Handle<JSObject> OrderedHashTableIterator<Derived, TableType>::Next(
-    Handle<Derived> iterator) {
-  Isolate* isolate = iterator->GetIsolate();
-  Factory* factory = isolate->factory();
-
-  Handle<Object> maybe_table(iterator->table(), isolate);
-  if (!maybe_table->IsUndefined()) {
-    iterator->Transition();
-
-    Handle<TableType> table(TableType::cast(iterator->table()), isolate);
-    int index = Smi::cast(iterator->index())->value();
-    int used_capacity = table->UsedCapacity();
-
-    while (index < used_capacity && table->KeyAt(index)->IsTheHole()) {
-      index++;
-    }
-
-    if (index < used_capacity) {
-      int entry_index = table->EntryToIndex(index);
-      Handle<Object> value =
-          Derived::ValueForKind(iterator, entry_index);
-      iterator->set_index(Smi::FromInt(index + 1));
-      return factory->NewIteratorResultObject(value, false);
-    }
-
-    iterator->set_table(iterator->GetHeap()->undefined_value());
-  }
-
-  return factory->NewIteratorResultObject(factory->undefined_value(), true);
-}
-
-
-template<class Derived, class TableType>
 void OrderedHashTableIterator<Derived, TableType>::Transition() {
-  Isolate* isolate = GetIsolate();
-  Handle<TableType> table(TableType::cast(this->table()), isolate);
+  DisallowHeapAllocation no_allocation;
+  TableType* table = TableType::cast(this->table());
   if (!table->IsObsolete()) return;
 
   int index = Smi::cast(this->index())->value();
   while (table->IsObsolete()) {
-    Handle<TableType> next_table(table->NextTable(), isolate);
+    TableType* next_table = table->NextTable();
 
     if (index > 0) {
       int nod = table->NumberOfDeletedElements();
@@ -16309,82 +16276,80 @@ void OrderedHashTableIterator<Derived, TableType>::Transition() {
     table = next_table;
   }
 
-  set_table(*table);
+  set_table(table);
   set_index(Smi::FromInt(index));
 }
 
 
-template Handle<JSObject>
-OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::Next(
-    Handle<JSSetIterator> iterator);
-
-template void
-OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::Transition();
+template<class Derived, class TableType>
+bool OrderedHashTableIterator<Derived, TableType>::HasMore() {
+  DisallowHeapAllocation no_allocation;
+  if (this->table()->IsUndefined()) return false;
 
+  Transition();
 
-template Handle<JSObject>
-OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::Next(
-    Handle<JSMapIterator> iterator);
+  TableType* table = TableType::cast(this->table());
+  int index = Smi::cast(this->index())->value();
+  int used_capacity = table->UsedCapacity();
 
-template void
-OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::Transition();
+  while (index < used_capacity && table->KeyAt(index)->IsTheHole()) {
+    index++;
+  }
 
+  set_index(Smi::FromInt(index));
 
-Handle<Object> JSSetIterator::ValueForKind(
-    Handle<JSSetIterator> iterator, int entry_index) {
-  int kind = iterator->kind()->value();
-  // Set.prototype only has values and entries.
-  ASSERT(kind == kKindValues || kind == kKindEntries);
+  if (index < used_capacity) return true;
 
-  Isolate* isolate = iterator->GetIsolate();
-  Factory* factory = isolate->factory();
+  set_table(GetHeap()->undefined_value());
+  return false;
+}
 
-  Handle<OrderedHashSet> table(
-      OrderedHashSet::cast(iterator->table()), isolate);
-  Handle<Object> value = Handle<Object>(table->get(entry_index), isolate);
 
-  if (kind == kKindEntries) {
-    Handle<FixedArray> array = factory->NewFixedArray(2);
-    array->set(0, *value);
-    array->set(1, *value);
-    return factory->NewJSArrayWithElements(array);
+template<class Derived, class TableType>
+Smi* OrderedHashTableIterator<Derived, TableType>::Next(JSArray* value_array) {
+  DisallowHeapAllocation no_allocation;
+  if (HasMore()) {
+    FixedArray* array = FixedArray::cast(value_array->elements());
+    static_cast<Derived*>(this)->PopulateValueArray(array);
+    MoveNext();
+    return kind();
   }
-
-  return value;
+  return Smi::FromInt(0);
 }
 
 
-Handle<Object> JSMapIterator::ValueForKind(
-    Handle<JSMapIterator> iterator, int entry_index) {
-  int kind = iterator->kind()->value();
-  ASSERT(kind == kKindKeys || kind == kKindValues || kind == kKindEntries);
+template Smi*
+OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::Next(
+    JSArray* value_array);
 
-  Isolate* isolate = iterator->GetIsolate();
-  Factory* factory = isolate->factory();
+template bool
+OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::HasMore();
 
-  Handle<OrderedHashMap> table(
-      OrderedHashMap::cast(iterator->table()), isolate);
+template void
+OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::MoveNext();
 
-  switch (kind) {
-    case kKindKeys:
-      return Handle<Object>(table->get(entry_index), isolate);
+template Object*
+OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::CurrentKey();
 
-    case kKindValues:
-      return Handle<Object>(table->get(entry_index + 1), isolate);
+template void
+OrderedHashTableIterator<JSSetIterator, OrderedHashSet>::Transition();
 
-    case kKindEntries: {
-      Handle<Object> key(table->get(entry_index), isolate);
-      Handle<Object> value(table->get(entry_index + 1), isolate);
-      Handle<FixedArray> array = factory->NewFixedArray(2);
-      array->set(0, *key);
-      array->set(1, *value);
-      return factory->NewJSArrayWithElements(array);
-    }
-  }
 
-  UNREACHABLE();
-  return factory->undefined_value();
-}
+template Smi*
+OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::Next(
+    JSArray* value_array);
+
+template bool
+OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::HasMore();
+
+template void
+OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::MoveNext();
+
+template Object*
+OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::CurrentKey();
+
+template void
+OrderedHashTableIterator<JSMapIterator, OrderedHashMap>::Transition();
 
 
 DeclaredAccessorDescriptorIterator::DeclaredAccessorDescriptorIterator(
diff --git a/src/objects.h b/src/objects.h
index cf38eab..a0cee46 100644
--- a/src/objects.h
+++ b/src/objects.h
@@ -4463,11 +4463,11 @@ class OrderedHashMap:public OrderedHashTable<
       Handle<Object> key,
       Handle<Object> value);
 
- private:
   Object* ValueAt(int entry) {
     return get(EntryToIndex(entry) + kValueOffset);
   }
 
+ private:
   static const int kValueOffset = 1;
 };
 
@@ -10104,13 +10104,26 @@ class OrderedHashTableIterator: public JSObject {
     kKindEntries = 3
   };
 
-  // Returns an iterator result object: {value: any, done: boolean} and moves
-  // the index to the next valid entry. Closes the iterator if moving past the
-  // end.
-  static Handle<JSObject> Next(Handle<Derived> iterator);
+  // Whether the iterator has more elements. This needs to be called before
+  // calling |CurrentKey| and/or |CurrentValue|.
+  bool HasMore();
+
+  // Move the index forward one.
+  void MoveNext() {
+    set_index(Smi::FromInt(Smi::cast(index())->value() + 1));
+  }
+
+  // Populates the array with the next key and value and then moves the iterator
+  // forward.
+  // This returns the |kind| or 0 if the iterator is already at the end.
+  Smi* Next(JSArray* value_array);
+
+  // Returns the current key of the iterator. This should only be called when
+  // |HasMore| returns true.
+  inline Object* CurrentKey();
 
  private:
-  // Transitions the iterator to the non obsolote backing store. This is a NOP
+  // Transitions the iterator to the non obsolete backing store. This is a NOP
   // if the [table] is not obsolete.
   void Transition();
 
@@ -10127,9 +10140,9 @@ class JSSetIterator: public OrderedHashTableIterator<JSSetIterator,
 
   DECLARE_CAST(JSSetIterator)
 
-  static Handle<Object> ValueForKind(
-      Handle<JSSetIterator> iterator,
-      int entry_index);
+  // Called by |Next| to populate the array. This allows the subclasses to
+  // populate the array differently.
+  inline void PopulateValueArray(FixedArray* array);
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSSetIterator);
@@ -10145,11 +10158,15 @@ class JSMapIterator: public OrderedHashTableIterator<JSMapIterator,
 
   DECLARE_CAST(JSMapIterator)
 
-  static Handle<Object> ValueForKind(
-      Handle<JSMapIterator> iterator,
-      int entry_index);
+  // Called by |Next| to populate the array. This allows the subclasses to
+  // populate the array differently.
+  inline void PopulateValueArray(FixedArray* array);
 
  private:
+  // Returns the current value of the iterator. This should only be called when
+  // |HasMore| returns true.
+  inline Object* CurrentValue();
+
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSMapIterator);
 };
 
diff --git a/src/runtime.cc b/src/runtime.cc
index e154047..831f76d 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -1604,10 +1604,11 @@ RUNTIME_FUNCTION(Runtime_SetIteratorInitialize) {
 
 
 RUNTIME_FUNCTION(Runtime_SetIteratorNext) {
-  HandleScope scope(isolate);
-  ASSERT(args.length() == 1);
-  CONVERT_ARG_HANDLE_CHECKED(JSSetIterator, holder, 0);
-  return *JSSetIterator::Next(holder);
+  SealHandleScope shs(isolate);
+  ASSERT(args.length() == 2);
+  CONVERT_ARG_CHECKED(JSSetIterator, holder, 0);
+  CONVERT_ARG_CHECKED(JSArray, value_array, 1);
+  return holder->Next(value_array);
 }
 
 
@@ -1708,10 +1709,11 @@ RUNTIME_FUNCTION(Runtime_MapIteratorInitialize) {
 
 
 RUNTIME_FUNCTION(Runtime_MapIteratorNext) {
-  HandleScope scope(isolate);
-  ASSERT(args.length() == 1);
-  CONVERT_ARG_HANDLE_CHECKED(JSMapIterator, holder, 0);
-  return *JSMapIterator::Next(holder);
+  SealHandleScope shs(isolate);
+  ASSERT(args.length() == 2);
+  CONVERT_ARG_CHECKED(JSMapIterator, holder, 0);
+  CONVERT_ARG_CHECKED(JSArray, value_array, 1);
+  return holder->Next(value_array);
 }
 
 
diff --git a/src/runtime.h b/src/runtime.h
index 6820bf9..43a362b 100644
--- a/src/runtime.h
+++ b/src/runtime.h
@@ -274,7 +274,7 @@ namespace internal {
   F(SetGetSize, 1, 1) \
   \
   F(SetIteratorInitialize, 3, 1) \
-  F(SetIteratorNext, 1, 1) \
+  F(SetIteratorNext, 2, 1) \
   \
   /* Harmony maps */ \
   F(MapInitialize, 1, 1) \
@@ -286,7 +286,7 @@ namespace internal {
   F(MapGetSize, 1, 1) \
   \
   F(MapIteratorInitialize, 3, 1) \
-  F(MapIteratorNext, 1, 1) \
+  F(MapIteratorNext, 2, 1) \
   \
   /* Harmony weak maps and sets */ \
   F(WeakCollectionInitialize, 1, 1) \
diff --git a/test/mjsunit/runtime-gen/mapiteratornext.js b/test/mjsunit/runtime-gen/mapiteratornext.js
index cbd7e5b..b46796e 100644
--- a/test/mjsunit/runtime-gen/mapiteratornext.js
+++ b/test/mjsunit/runtime-gen/mapiteratornext.js
@@ -2,4 +2,5 @@
 // AUTO-GENERATED BY tools/generate-runtime-tests.py, DO NOT MODIFY
 // Flags: --allow-natives-syntax --harmony
 var _holder = new Map().entries();
-%MapIteratorNext(_holder);
+var _value_array = new Array();
+%MapIteratorNext(_holder, _value_array);
diff --git a/test/mjsunit/runtime-gen/setiteratornext.js b/test/mjsunit/runtime-gen/setiteratornext.js
index 7cf88cf..a169c46 100644
--- a/test/mjsunit/runtime-gen/setiteratornext.js
+++ b/test/mjsunit/runtime-gen/setiteratornext.js
@@ -2,4 +2,5 @@
 // AUTO-GENERATED BY tools/generate-runtime-tests.py, DO NOT MODIFY
 // Flags: --allow-natives-syntax --harmony
 var _holder = new Set().values();
-%SetIteratorNext(_holder);
+var _value_array = new Array();
+%SetIteratorNext(_holder, _value_array);