ES6 Map/Set iterators/forEach improvements

This changes how Map/Set interacts with its iterators. When the
underlying table is rehashed or cleared, we create a new table (like
before) but we add a reference from the old table to the new table. We
also add an array describing how to transition the iterator from the
old table to the new table.

When Next is called on the iterator it checks if there is a newer table
that it should transition to. If there is, it updates the index based
on the previously recorded changes and finally changes itself to point
at the new table.

With these changes Map/Set no longer keeps the iterators alive. Also,
as before, the iterators keep the underlying table(s) alive but not the
actual Map/Set.

BUG=v8:1793
LOG=Y
R=mstarzinger@chromium.org, rossberg@chromium.org

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

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

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

src/collection.js

@@ -106,12 +106,8 @@ function SetForEach(f, receiver) {
 
   var iterator = %SetCreateIterator(this, ITERATOR_KIND_VALUES);
   var entry;
-  try {
-    while (!(entry = %SetIteratorNext(iterator)).done) {
-      %_CallFunction(receiver, entry.value, entry.value, this, f);
-    }
-  } finally {
-    %SetIteratorClose(iterator);
+  while (!(entry = %SetIteratorNext(iterator)).done) {
+    %_CallFunction(receiver, entry.value, entry.value, this, f);
   }
 }
 
@@ -219,12 +215,8 @@ function MapForEach(f, receiver) {
 
   var iterator = %MapCreateIterator(this, ITERATOR_KIND_ENTRIES);
   var entry;
-  try {
-    while (!(entry = %MapIteratorNext(iterator)).done) {
-      %_CallFunction(receiver, entry.value[1], entry.value[0], this, f);
-    }
-  } finally {
-    %MapIteratorClose(iterator);
+  while (!(entry = %MapIteratorNext(iterator)).done) {
+    %_CallFunction(receiver, entry.value[1], entry.value[0], this, f);
   }
 }
 

src/objects-debug.cc

@@ -702,13 +702,7 @@ void JSSetIterator::JSSetIteratorVerify() {
   VerifyHeapPointer(table());
   CHECK(table()->IsOrderedHashTable() || table()->IsUndefined());
   CHECK(index()->IsSmi());
-  CHECK(count()->IsSmi());
   CHECK(kind()->IsSmi());
-  VerifyHeapPointer(next_iterator());
-  CHECK(next_iterator()->IsJSSetIterator() || next_iterator()->IsUndefined());
-  VerifyHeapPointer(table());
-  CHECK(previous_iterator()->IsJSSetIterator()
-        || previous_iterator()->IsUndefined());
 }
 
 
@@ -718,13 +712,7 @@ void JSMapIterator::JSMapIteratorVerify() {
   VerifyHeapPointer(table());
   CHECK(table()->IsOrderedHashTable() || table()->IsUndefined());
   CHECK(index()->IsSmi());
-  CHECK(count()->IsSmi());
   CHECK(kind()->IsSmi());
-  VerifyHeapPointer(next_iterator());
-  CHECK(next_iterator()->IsJSMapIterator() || next_iterator()->IsUndefined());
-  VerifyHeapPointer(table());
-  CHECK(previous_iterator()->IsJSMapIterator()
-        || previous_iterator()->IsUndefined());
 }
 
 

src/objects-inl.h

@@ -5683,12 +5683,7 @@ ACCESSORS(JSMap, table, Object, kTableOffset)
 
 ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(table, Object, kTableOffset)
 ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(index, Smi, kIndexOffset)
-ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(count, Smi, kCountOffset)
 ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(kind, Smi, kKindOffset)
-ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(next_iterator, Object,
-                                      kNextIteratorOffset)
-ORDERED_HASH_TABLE_ITERATOR_ACCESSORS(previous_iterator, Object,
-                                      kPreviousIteratorOffset)
 
 #undef ORDERED_HASH_TABLE_ITERATOR_ACCESSORS
 

src/objects-printer.cc

@@ -743,14 +743,8 @@ void OrderedHashTableIterator<Derived, TableType>::
   table()->ShortPrint(out);
   PrintF(out, "\n - index = ");
   index()->ShortPrint(out);
-  PrintF(out, "\n - count = ");
-  count()->ShortPrint(out);
   PrintF(out, "\n - kind = ");
   kind()->ShortPrint(out);
-  PrintF(out, "\n - next_iterator = ");
-  next_iterator()->ShortPrint(out);
-  PrintF(out, "\n - previous_iterator = ");
-  previous_iterator()->ShortPrint(out);
   PrintF(out, "\n");
 }
 

src/objects.h

@@ -4153,16 +4153,27 @@ class ObjectHashTable: public HashTable<ObjectHashTable,
 //   [0]: bucket count
 //   [1]: element count
 //   [2]: deleted element count
-//   [3]: live iterators (doubly-linked list)
-//   [4..(NumberOfBuckets() - 1)]: "hash table", where each item is an offset
-//                                 into the data table (see below) where the
-//                                 first item in this bucket is stored.
-//   [4 + NumberOfBuckets()..length]: "data table", an array of length
+//   [3..(3 + NumberOfBuckets() - 1)]: "hash table", where each item is an
+//                            offset into the data table (see below) where the
+//                            first item in this bucket is stored.
+//   [3 + NumberOfBuckets()..length]: "data table", an array of length
 //                            Capacity() * kEntrySize, where the first entrysize
 //                            items are handled by the derived class and the
 //                            item at kChainOffset is another entry into the
 //                            data table indicating the next entry in this hash
 //                            bucket.
+//
+// When we transition the table to a new version we obsolete it and reuse parts
+// of the memory to store information how to transition an iterator to the new
+// table:
+//
+// Memory layout for obsolete table:
+//   [0]: bucket count
+//   [1]: Next newer table
+//   [2]: Number of removed holes or -1 when the table was cleared.
+//   [3..(3 + NumberOfRemovedHoles() - 1)]: The indexes of the removed holes.
+//   [3 + NumberOfRemovedHoles()..length]: Not used
+//
 template<class Derived, class Iterator, int entrysize>
 class OrderedHashTable: public FixedArray {
  public:
@@ -4199,10 +4210,6 @@ class OrderedHashTable: public FixedArray {
     return Smi::cast(get(kNumberOfBucketsIndex))->value();
   }
 
-  Object* iterators() { return get(kIteratorsIndex); }
-
-  void set_iterators(Object* value) { set(kIteratorsIndex, value); }
-
   // Returns the index into the data table where the new entry
   // should be placed. The table is assumed to have enough space
   // for a new entry.
@@ -4219,6 +4226,20 @@ class OrderedHashTable: public FixedArray {
 
   Object* KeyAt(int entry) { return get(EntryToIndex(entry)); }
 
+  bool IsObsolete() {
+    return !get(kNextTableIndex)->IsSmi();
+  }
+
+  // The next newer table. This is only valid if the table is obsolete.
+  Derived* NextTable() {
+    return Derived::cast(get(kNextTableIndex));
+  }
+
+  // When the table is obsolete we store the indexes of the removed holes.
+  int RemovedIndexAt(int index) {
+    return Smi::cast(get(kRemovedHolesIndex + index))->value();
+  }
+
   static const int kNotFound = -1;
   static const int kMinCapacity = 4;
 
@@ -4255,11 +4276,21 @@ class OrderedHashTable: public FixedArray {
     return Smi::cast(get(kHashTableStartIndex + bucket))->value();
   }
 
+  void SetNextTable(Derived* next_table) {
+    set(kNextTableIndex, next_table);
+  }
+
+  void SetRemovedIndexAt(int index, int removed_index) {
+    return set(kRemovedHolesIndex + index, Smi::FromInt(removed_index));
+  }
+
   static const int kNumberOfBucketsIndex = 0;
   static const int kNumberOfElementsIndex = kNumberOfBucketsIndex + 1;
   static const int kNumberOfDeletedElementsIndex = kNumberOfElementsIndex + 1;
-  static const int kIteratorsIndex = kNumberOfDeletedElementsIndex + 1;
-  static const int kHashTableStartIndex = kIteratorsIndex + 1;
+  static const int kHashTableStartIndex = kNumberOfDeletedElementsIndex + 1;
+
+  static const int kNextTableIndex = kNumberOfElementsIndex;
+  static const int kRemovedHolesIndex = kHashTableStartIndex;
 
   static const int kEntrySize = entrysize + 1;
   static const int kChainOffset = entrysize;
@@ -9956,17 +9987,15 @@ class JSMap: public JSObject {
 // OrderedHashTableIterator is an iterator that iterates over the keys and
 // values of an OrderedHashTable.
 //
-// The hash table has a reference to the iterator and the iterators themselves
-// have references to the [next_iterator] and [previous_iterator], thus creating
-// a double linked list.
+// The iterator has a reference to the underlying OrderedHashTable data,
+// [table], as well as the current [index] the iterator is at.
 //
-// When the hash table changes the iterators are called to update their [index]
-// and [count]. The hash table calls [EntryRemoved], [TableCompacted] as well
-// as [TableCleared].
+// When the OrderedHashTable is rehashed it adds a reference from the old table
+// to the new table as well as storing enough data about the changes so that the
+// iterator [index] can be adjusted accordingly.
 //
-// When an iterator is done it closes itself. It removes itself from the double
-// linked list and it sets its [table] to undefined, no longer keeping the
-// [table] alive.
+// When the [Next] result from the iterator is requested, the iterator checks if
+// there is a newer table that it needs to transition to.
 template<class Derived, class TableType>
 class OrderedHashTableIterator: public JSObject {
  public:
@@ -9976,29 +10005,17 @@ class OrderedHashTableIterator: public JSObject {
   // [index]: The index into the data table.
   DECL_ACCESSORS(index, Smi)
 
-  // [count]: The logical index into the data table, ignoring the holes.
-  DECL_ACCESSORS(count, Smi)
-
   // [kind]: The kind of iteration this is. One of the [Kind] enum values.
   DECL_ACCESSORS(kind, Smi)
 
-  // [next_iterator]: Used as a double linked list for the live iterators.
-  DECL_ACCESSORS(next_iterator, Object)
-
-  // [previous_iterator]: Used as a double linked list for the live iterators.
-  DECL_ACCESSORS(previous_iterator, Object)
-
 #ifdef OBJECT_PRINT
   void OrderedHashTableIteratorPrint(FILE* out);
 #endif
 
   static const int kTableOffset = JSObject::kHeaderSize;
   static const int kIndexOffset = kTableOffset + kPointerSize;
-  static const int kCountOffset = kIndexOffset + kPointerSize;
-  static const int kKindOffset = kCountOffset + kPointerSize;
-  static const int kNextIteratorOffset = kKindOffset + kPointerSize;
-  static const int kPreviousIteratorOffset = kNextIteratorOffset + kPointerSize;
-  static const int kSize = kPreviousIteratorOffset + kPointerSize;
+  static const int kKindOffset = kIndexOffset + kPointerSize;
+  static const int kSize = kKindOffset + kPointerSize;
 
   enum Kind {
     kKindKeys = 1,
@@ -10006,25 +10023,6 @@ class OrderedHashTableIterator: public JSObject {
     kKindEntries = 3
   };
 
-  // Called by the underlying [table] when an entry is removed.
-  void EntryRemoved(int index);
-
-  // Called by the underlying [table] when it is compacted/rehashed.
-  void TableCompacted() {
-    // All holes have been removed so index is now same as count.
-    set_index(count());
-  }
-
-  // Called by the underlying [table] when it is cleared.
-  void TableCleared() {
-    set_index(Smi::FromInt(0));
-    set_count(Smi::FromInt(0));
-  }
-
-  // Removes the iterator from the double linked list and removes its reference
-  // back to the [table].
-  void Close();
-
   // 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.
@@ -10035,16 +10033,9 @@ class OrderedHashTableIterator: public JSObject {
       Handle<Map> map, Handle<TableType> table, int kind);
 
  private:
-  // Ensures [index] is not pointing to a hole.
-  void Seek();
-
-  // Moves [index] to next valid entry. Closes the iterator if moving past the
-  // end.
-  void MoveNext();
-
-  bool Closed() {
-    return table()->IsUndefined();
-  }
+  // Transitions the iterator to the non obsolote backing store. This is a NOP
+  // if the [table] is not obsolete.
+  void Transition();
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(OrderedHashTableIterator);
 };
@@ -10066,7 +10057,8 @@ class JSSetIterator: public OrderedHashTableIterator<JSSetIterator,
   static inline JSSetIterator* cast(Object* obj);
 
   static Handle<Object> ValueForKind(
-      Handle<JSSetIterator> iterator, int entry_index);
+      Handle<JSSetIterator> iterator,
+      int entry_index);
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSSetIterator);
@@ -10089,7 +10081,8 @@ class JSMapIterator: public OrderedHashTableIterator<JSMapIterator,
   static inline JSMapIterator* cast(Object* obj);
 
   static Handle<Object> ValueForKind(
-      Handle<JSMapIterator> iterator, int entry_index);
+      Handle<JSMapIterator> iterator,
+      int entry_index);
 
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(JSMapIterator);

src/runtime.cc

@@ -1600,15 +1600,6 @@ RUNTIME_FUNCTION(Runtime_SetIteratorNext) {
 }
 
 
-RUNTIME_FUNCTION(Runtime_SetIteratorClose) {
-  HandleScope scope(isolate);
-  ASSERT(args.length() == 1);
-  CONVERT_ARG_HANDLE_CHECKED(JSSetIterator, holder, 0);
-  holder->Close();
-  return isolate->heap()->undefined_value();
-}
-
-
 RUNTIME_FUNCTION(Runtime_MapInitialize) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
@@ -1709,15 +1700,6 @@ RUNTIME_FUNCTION(Runtime_MapIteratorNext) {
 }
 
 
-RUNTIME_FUNCTION(Runtime_MapIteratorClose) {
-  HandleScope scope(isolate);
-  ASSERT(args.length() == 1);
-  CONVERT_ARG_HANDLE_CHECKED(JSMapIterator, holder, 0);
-  holder->Close();
-  return isolate->heap()->undefined_value();
-}
-
-
 static Handle<JSWeakCollection> WeakCollectionInitialize(
     Isolate* isolate,
     Handle<JSWeakCollection> weak_collection) {

src/runtime.h

@@ -276,7 +276,6 @@ namespace internal {
   F(SetCreateIterator, 2, 1) \
   \
   F(SetIteratorNext, 1, 1) \
-  F(SetIteratorClose, 1, 1) \
   \
   /* Harmony maps */ \
   F(MapInitialize, 1, 1) \
@@ -289,7 +288,6 @@ namespace internal {
   F(MapCreateIterator, 2, 1) \
   \
   F(MapIteratorNext, 1, 1) \
-  F(MapIteratorClose, 1, 1) \
   \
   /* Harmony weak maps and sets */ \
   F(WeakCollectionInitialize, 1, 1) \

test/mjsunit/harmony/collections.js

@@ -852,3 +852,138 @@ for (var i = 9; i >= 0; i--) {
   });
   assertEquals(4950, accumulated);
 })();
+
+
+(function TestMapForEachAllRemovedTransition() {
+  var map = new Map;
+  map.set(0, 0);
+
+  var buffer = [];
+  map.forEach(function(v) {
+    buffer.push(v);
+    if (v === 0) {
+      for (var i = 1; i < 4; i++) {
+        map.set(i, i);
+      }
+    }
+
+    if (v === 3) {
+      for (var i = 0; i < 4; i++) {
+        map.delete(i);
+      }
+      for (var i = 4; i < 8; i++) {
+        map.set(i, i);
+      }
+    }
+  });
+
+  assertArrayEquals([0, 1, 2, 3, 4, 5, 6, 7], buffer);
+})();
+
+
+(function TestMapForEachClearTransition() {
+  var map = new Map;
+  map.set(0, 0);
+
+  var i = 0;
+  var buffer = [];
+  map.forEach(function(v) {
+    buffer.push(v);
+    if (++i < 5) {
+      for (var j = 0; j < 5; j++) {
+        map.clear();
+        map.set(i, i);
+      }
+    }
+  });
+
+  assertArrayEquals([0, 1, 2, 3, 4], buffer);
+})();
+
+
+(function TestMapForEachNestedNonTrivialTransition() {
+  var map = new Map;
+  map.set(0, 0);
+  map.set(1, 1);
+  map.set(2, 2);
+  map.set(3, 3);
+  map.delete(0);
+
+  var i = 0;
+  var buffer = [];
+  map.forEach(function(v) {
+    if (++i > 10) return;
+
+    buffer.push(v);
+
+    if (v == 3) {
+      map.delete(1);
+      for (var j = 4; j < 10; j++) {
+        map.set(j, j);
+      }
+      for (var j = 4; j < 10; j += 2) {
+        map.delete(j);
+      }
+      map.delete(2);
+
+      for (var j = 10; j < 20; j++) {
+        map.set(j, j);
+      }
+      for (var j = 10; j < 20; j += 2) {
+        map.delete(j);
+      }
+
+      map.delete(3);
+    }
+  });
+
+  assertArrayEquals([1, 2, 3, 5, 7, 9, 11, 13, 15, 17], buffer);
+})();
+
+
+(function TestMapForEachAllRemovedTransitionNoClear() {
+  var map = new Map;
+  map.set(0, 0);
+
+  var buffer = [];
+  map.forEach(function(v) {
+    buffer.push(v);
+    if (v === 0) {
+      for (var i = 1; i < 8; i++) {
+        map.set(i, i);
+      }
+    }
+
+    if (v === 4) {
+      for (var i = 0; i < 8; i++) {
+        map.delete(i);
+      }
+    }
+  });
+
+  assertArrayEquals([0, 1, 2, 3, 4], buffer);
+})();
+
+
+(function TestMapForEachNoMoreElementsAfterTransition() {
+  var map = new Map;
+  map.set(0, 0);
+
+  var buffer = [];
+  map.forEach(function(v) {
+    buffer.push(v);
+    if (v === 0) {
+      for (var i = 1; i < 16; i++) {
+        map.set(i, i);
+      }
+    }
+
+    if (v === 4) {
+      for (var i = 5; i < 16; i++) {
+        map.delete(i);
+      }
+    }
+  });
+
+  assertArrayEquals([0, 1, 2, 3, 4], buffer);
+})();

test/mjsunit/runtime-gen/mapiteratorclose.js

-// Copyright 2014 the V8 project authors. All rights reserved.
-// AUTO-GENERATED BY tools/generate-runtime-tests.py, DO NOT MODIFY
-// Flags: --allow-natives-syntax --harmony
-var _holder = %MapCreateIterator(new Map(), 3);
-%MapIteratorClose(_holder);

test/mjsunit/runtime-gen/setiteratorclose.js

-// Copyright 2014 the V8 project authors. All rights reserved.
-// AUTO-GENERATED BY tools/generate-runtime-tests.py, DO NOT MODIFY
-// Flags: --allow-natives-syntax --harmony
-var _holder = %SetCreateIterator(new Set(), 2);
-%SetIteratorClose(_holder);

tools/generate-runtime-tests.py

@@ -47,8 +47,8 @@ EXPAND_MACROS = [
 # that the parser doesn't bit-rot. Change the values as needed when you add,
 # remove or change runtime functions, but make sure we don't lose our ability
 # to parse them!
-EXPECTED_FUNCTION_COUNT = 361
-EXPECTED_FUZZABLE_COUNT = 328
+EXPECTED_FUNCTION_COUNT = 359
+EXPECTED_FUZZABLE_COUNT = 326
 EXPECTED_CCTEST_COUNT = 6
 EXPECTED_UNKNOWN_COUNT = 5
 EXPECTED_BUILTINS_COUNT = 824