[IdentityMap] Remove ability to delete entries during iteration.

Deletion can cause collisions to be moved in the map which breaks iteration.
For now just remove this support. Also add some additional collision tests
to the test.

BUG=v8:5203

Change-Id: I54a0a4af51da08b3f963dc1d7661dba291e4efea
Reviewed-on: https://chromium-review.googlesource.com/445900Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#43621}

src/identity-map.cc

@@ -41,11 +41,6 @@ void IdentityMapBase::EnableIteration() {
 void IdentityMapBase::DisableIteration() {
   CHECK(is_iterable());
   is_iterable_ = false;
-
-  // We might need to resize due to iterator deletion - do this now.
-  if (size_ * kResizeFactor < capacity_ / kResizeFactor) {
-    Resize(capacity_ / kResizeFactor);
-  }
 }
 
 int IdentityMapBase::ScanKeysFor(Object* address) const {
@@ -91,7 +86,7 @@ void* IdentityMapBase::DeleteIndex(int index) {
   size_--;
   DCHECK_GE(size_, 0);
 
-  if (!is_iterable() && (size_ * kResizeFactor < capacity_ / kResizeFactor)) {
+  if (size_ * kResizeFactor < capacity_ / kResizeFactor) {
     Resize(capacity_ / kResizeFactor);
     return ret_value;  // No need to fix collisions as resize reinserts keys.
   }
@@ -110,6 +105,7 @@ void* IdentityMapBase::DeleteIndex(int index) {
       DCHECK_GT(index, next_index);
       if (index < expected_index || expected_index <= next_index) continue;
     }
+
     DCHECK_EQ(not_mapped, keys_[index]);
     DCHECK_NULL(values_[index]);
     std::swap(keys_[index], keys_[next_index]);
@@ -212,7 +208,7 @@ int IdentityMapBase::NextIndex(int index) const {
   DCHECK_LE(index, capacity_);
   CHECK(is_iterable());  // Must be iterable to access by index;
   Object* not_mapped = heap_->not_mapped_symbol();
-  for (index++; index < capacity_; index++) {
+  for (++index; index < capacity_; ++index) {
     if (keys_[index] != not_mapped) {
       return index;
     }

src/identity-map.h

@@ -44,7 +44,6 @@ class IdentityMapBase {
   RawEntry GetEntry(Object* key);
   RawEntry FindEntry(Object* key) const;
   void* DeleteEntry(Object* key);
-  void* DeleteIndex(int index);
   void Clear();
 
   V8_EXPORT_PRIVATE RawEntry EntryAtIndex(int index) const;
@@ -62,6 +61,7 @@ class IdentityMapBase {
   int InsertKey(Object* address);
   int Lookup(Object* key) const;
   int LookupOrInsert(Object* key);
+  void* DeleteIndex(int index);
   void Rehash();
   void Resize(int new_capacity);
   int Hash(Object* address) const;
@@ -126,12 +126,6 @@ class IdentityMap : public IdentityMapBase {
       return *this;
     }
 
-    Iterator& DeleteAndIncrement() {
-      map_->DeleteIndex(index_);
-      index_ = map_->NextIndex(index_);
-      return *this;
-    }
-
     V* operator*() { return reinterpret_cast<V*>(map_->EntryAtIndex(index_)); }
     V* operator->() { return reinterpret_cast<V*>(map_->EntryAtIndex(index_)); }
     bool operator!=(const Iterator& other) { return index_ != other.index_; }

test/cctest/test-identity-map.cc

@@ -549,11 +549,12 @@ TEST(Iterator_smi_num) {
     t.map.Set(num_keys[i], reinterpret_cast<void*>(i + 5));
   }
 
-  // Check iterator sees all values.
+  // Check iterator sees all values once.
   std::set<intptr_t> seen;
   {
     IdentityMap<void*, ZoneAllocationPolicy>::IteratableScope it_scope(&t.map);
     for (auto it = it_scope.begin(); it != it_scope.end(); ++it) {
+      CHECK(seen.find(reinterpret_cast<intptr_t>(**it)) == seen.end());
       seen.insert(reinterpret_cast<intptr_t>(**it));
     }
   }
@@ -585,6 +586,7 @@ TEST(Iterator_smi_num_gc) {
   {
     IdentityMap<void*, ZoneAllocationPolicy>::IteratableScope it_scope(&t.map);
     for (auto it = it_scope.begin(); it != it_scope.end(); ++it) {
+      CHECK(seen.find(reinterpret_cast<intptr_t>(**it)) == seen.end());
       seen.insert(reinterpret_cast<intptr_t>(**it));
     }
   }
@@ -593,42 +595,49 @@ TEST(Iterator_smi_num_gc) {
   }
 }
 
-TEST(Iterator_smi_delete) {
-  IdentityMapTester t;
-  int smi_keys[] = {1, 2, 7, 15, 23};
-
-  // Initialize the map.
-  for (size_t i = 0; i < arraysize(smi_keys); i++) {
-    t.map.Set(t.smi(smi_keys[i]), reinterpret_cast<void*>(i));
-  }
+void IterateCollisionTest(int stride) {
+  for (int load = 15; load <= 120; load = load * 2) {
+    IdentityMapTester t;
 
-  // Iterate and delete half the elements.
-  std::set<intptr_t> deleted;
-  {
-    int i = 0;
-    IdentityMap<void*, ZoneAllocationPolicy>::IteratableScope it_scope(&t.map);
-    for (auto it = it_scope.begin(); it != it_scope.end();) {
-      if (i % 2) {
-        deleted.insert(reinterpret_cast<intptr_t>(**it));
-        it.DeleteAndIncrement();
-      } else {
-        ++it;
+    {  // Add entries to the map.
+      HandleScope scope(t.isolate());
+      int next = 1;
+      for (int i = 0; i < load; i++) {
+        t.map.Set(t.smi(next), reinterpret_cast<void*>(next));
+        t.CheckFind(t.smi(next), reinterpret_cast<void*>(next));
+        next = next + stride;
       }
     }
-  }
-
-  // Check values in map are correct.
-  for (intptr_t i = 0; i < 5; i++) {
-    void** entry = t.map.Find(t.smi(smi_keys[i]));
-    if (deleted.find(i) != deleted.end()) {
-      CHECK_NULL(entry);
-    } else {
-      CHECK_NOT_NULL(entry);
-      CHECK_EQ(reinterpret_cast<void*>(i), *entry);
+    // Iterate through the map and check we see all elements only once.
+    std::set<intptr_t> seen;
+    {
+      IdentityMap<void*, ZoneAllocationPolicy>::IteratableScope it_scope(
+          &t.map);
+      for (auto it = it_scope.begin(); it != it_scope.end(); ++it) {
+        CHECK(seen.find(reinterpret_cast<intptr_t>(**it)) == seen.end());
+        seen.insert(reinterpret_cast<intptr_t>(**it));
+      }
+    }
+    // Check get and find on map.
+    {
+      HandleScope scope(t.isolate());
+      int next = 1;
+      for (int i = 0; i < load; i++) {
+        CHECK(seen.find(next) != seen.end());
+        t.CheckFind(t.smi(next), reinterpret_cast<void*>(next));
+        t.CheckGet(t.smi(next), reinterpret_cast<void*>(next));
+        next = next + stride;
+      }
     }
   }
 }
 
+TEST(IterateCollisions_1) { IterateCollisionTest(1); }
+TEST(IterateCollisions_2) { IterateCollisionTest(2); }
+TEST(IterateCollisions_3) { IterateCollisionTest(3); }
+TEST(IterateCollisions_5) { IterateCollisionTest(5); }
+TEST(IterateCollisions_7) { IterateCollisionTest(7); }
+
 void CollisionTest(int stride, bool rehash = false, bool resize = false) {
   for (int load = 15; load <= 120; load = load * 2) {
     IdentityMapTester t;