[heap] Remove pre-freeing of SlotSet buckets

Now that sweeping uses its own RememberedSet, pre-freeing of empty
buckets is not necessary anymore. Mutator inserts into a different
remembered set, than the sweeper removes slots from.

Bug: v8:9454
Change-Id: I65d046926aa82aeb9eca7694e6a7eff1331d7e01
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1835547
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64114}

src/heap/heap.cc

@@ -4114,8 +4114,9 @@ void CollectSlots(MemoryChunk* chunk, Address start, Address end,
         }
         return KEEP_SLOT;
       },
-      SlotSet::PREFREE_EMPTY_BUCKETS);
+      SlotSet::FREE_EMPTY_BUCKETS);
   if (direction == OLD_TO_NEW) {
+    CHECK(chunk->SweepingDone());
     RememberedSetSweeping::Iterate(
         chunk,
         [start, end, untyped](MaybeObjectSlot slot) {
@@ -4124,7 +4125,7 @@ void CollectSlots(MemoryChunk* chunk, Address start, Address end,
           }
           return KEEP_SLOT;
         },
-        SlotSet::PREFREE_EMPTY_BUCKETS);
+        SlotSet::FREE_EMPTY_BUCKETS);
   }
   RememberedSet<direction>::IterateTyped(
       chunk, [=](SlotType type, Address slot) {

src/heap/mark-compact.cc

@@ -2083,10 +2083,10 @@ void MarkCompactCollector::FlushBytecodeFromSFI(
   DCHECK_NULL(chunk->sweeping_slot_set());
   RememberedSet<OLD_TO_NEW>::RemoveRange(
       chunk, compiled_data_start, compiled_data_start + compiled_data_size,
-      SlotSet::PREFREE_EMPTY_BUCKETS);
+      SlotSet::FREE_EMPTY_BUCKETS);
   RememberedSet<OLD_TO_OLD>::RemoveRange(
       chunk, compiled_data_start, compiled_data_start + compiled_data_size,
-      SlotSet::PREFREE_EMPTY_BUCKETS);
+      SlotSet::FREE_EMPTY_BUCKETS);
 
   // Swap the map, using set_map_after_allocation to avoid verify heap checks
   // which are not necessary since we are doing this during the GC atomic pause.
@@ -2237,9 +2237,9 @@ void MarkCompactCollector::RightTrimDescriptorArray(DescriptorArray array,
   MemoryChunk* chunk = MemoryChunk::FromHeapObject(array);
   DCHECK_NULL(chunk->sweeping_slot_set());
   RememberedSet<OLD_TO_NEW>::RemoveRange(chunk, start, end,
-                                         SlotSet::PREFREE_EMPTY_BUCKETS);
+                                         SlotSet::FREE_EMPTY_BUCKETS);
   RememberedSet<OLD_TO_OLD>::RemoveRange(chunk, start, end,
-                                         SlotSet::PREFREE_EMPTY_BUCKETS);
+                                         SlotSet::FREE_EMPTY_BUCKETS);
   heap()->CreateFillerObjectAt(start, static_cast<int>(end - start),
                                ClearRecordedSlots::kNo);
   array.set_number_of_all_descriptors(new_nof_all_descriptors);
@@ -3419,7 +3419,7 @@ class RememberedSetUpdatingItem : public UpdatingItem {
             if (!filter.IsValid(slot.address())) return REMOVE_SLOT;
             return CheckAndUpdateOldToNewSlot(slot);
           },
-          SlotSet::PREFREE_EMPTY_BUCKETS);
+          SlotSet::FREE_EMPTY_BUCKETS);
     }
 
     if (chunk_->sweeping_slot_set<AccessMode::NON_ATOMIC>()) {
@@ -3430,7 +3430,7 @@ class RememberedSetUpdatingItem : public UpdatingItem {
             if (!filter.IsValid(slot.address())) return REMOVE_SLOT;
             return CheckAndUpdateOldToNewSlot(slot);
           },
-          SlotSet::PREFREE_EMPTY_BUCKETS);
+          SlotSet::FREE_EMPTY_BUCKETS);
     }
 
     if (chunk_->invalidated_slots<OLD_TO_NEW>() != nullptr) {
@@ -3448,7 +3448,7 @@ class RememberedSetUpdatingItem : public UpdatingItem {
             if (!filter.IsValid(slot.address())) return REMOVE_SLOT;
             return UpdateSlot<AccessMode::NON_ATOMIC>(slot);
           },
-          SlotSet::PREFREE_EMPTY_BUCKETS);
+          SlotSet::FREE_EMPTY_BUCKETS);
       chunk_->ReleaseSlotSet<OLD_TO_OLD>();
     }
     if ((updating_mode_ == RememberedSetUpdatingMode::ALL) &&
@@ -3790,10 +3790,10 @@ void MarkCompactCollector::PostProcessEvacuationCandidates() {
     // Remove outdated slots.
     RememberedSetSweeping::RemoveRange(page, page->address(),
                                        failed_object.address(),
-                                       SlotSet::PREFREE_EMPTY_BUCKETS);
+                                       SlotSet::FREE_EMPTY_BUCKETS);
     RememberedSet<OLD_TO_NEW>::RemoveRange(page, page->address(),
                                            failed_object.address(),
-                                           SlotSet::PREFREE_EMPTY_BUCKETS);
+                                           SlotSet::FREE_EMPTY_BUCKETS);
     RememberedSet<OLD_TO_NEW>::RemoveRangeTyped(page, page->address(),
                                                 failed_object.address());
     // Recompute live bytes.
@@ -4368,11 +4368,7 @@ void MinorMarkCompactCollector::CollectGarbage() {
 
   RememberedSet<OLD_TO_NEW>::IterateMemoryChunks(
       heap(), [](MemoryChunk* chunk) {
-        if (chunk->SweepingDone()) {
-          RememberedSet<OLD_TO_NEW>::FreeEmptyBuckets(chunk);
-        } else {
-          RememberedSet<OLD_TO_NEW>::PreFreeEmptyBuckets(chunk);
-        }
+        RememberedSet<OLD_TO_NEW>::FreeEmptyBuckets(chunk);
       });
 
   heap()->account_external_memory_concurrently_freed();
@@ -4669,7 +4665,7 @@ class PageMarkingItem : public MarkingItem {
           if (!filter.IsValid(slot.address())) return REMOVE_SLOT;
           return CheckAndMarkObject(task, slot);
         },
-        SlotSet::PREFREE_EMPTY_BUCKETS);
+        SlotSet::FREE_EMPTY_BUCKETS);
     filter = InvalidatedSlotsFilter::OldToNew(chunk_);
     RememberedSetSweeping::Iterate(
         chunk_,
@@ -4677,7 +4673,7 @@ class PageMarkingItem : public MarkingItem {
           if (!filter.IsValid(slot.address())) return REMOVE_SLOT;
           return CheckAndMarkObject(task, slot);
         },
-        SlotSet::PREFREE_EMPTY_BUCKETS);
+        SlotSet::FREE_EMPTY_BUCKETS);
   }
 
   void MarkTypedPointers(YoungGenerationMarkingTask* task) {

src/heap/remembered-set.h

@@ -178,30 +178,6 @@ class RememberedSet : public AllStatic {
     RememberedSetOperations::Iterate(slots, chunk, callback, mode);
   }
 
-  static int NumberOfPreFreedEmptyBuckets(MemoryChunk* chunk) {
-    DCHECK(type == OLD_TO_NEW);
-    int result = 0;
-    SlotSet* slots = chunk->slot_set<type>();
-    if (slots != nullptr) {
-      size_t pages = (chunk->size() + Page::kPageSize - 1) / Page::kPageSize;
-      for (size_t page = 0; page < pages; page++) {
-        result += slots[page].NumberOfPreFreedEmptyBuckets();
-      }
-    }
-    return result;
-  }
-
-  static void PreFreeEmptyBuckets(MemoryChunk* chunk) {
-    DCHECK(type == OLD_TO_NEW);
-    SlotSet* slots = chunk->slot_set<type>();
-    if (slots != nullptr) {
-      size_t pages = (chunk->size() + Page::kPageSize - 1) / Page::kPageSize;
-      for (size_t page = 0; page < pages; page++) {
-        slots[page].PreFreeEmptyBuckets();
-      }
-    }
-  }
-
   static void FreeEmptyBuckets(MemoryChunk* chunk) {
     DCHECK(type == OLD_TO_NEW);
     SlotSet* slots = chunk->slot_set<type>();
@@ -209,7 +185,6 @@ class RememberedSet : public AllStatic {
       size_t pages = (chunk->size() + Page::kPageSize - 1) / Page::kPageSize;
       for (size_t page = 0; page < pages; page++) {
         slots[page].FreeEmptyBuckets();
-        slots[page].FreeToBeFreedBuckets();
       }
     }
   }

src/heap/scavenger.cc

@@ -346,11 +346,7 @@ void ScavengerCollector::CollectGarbage() {
   heap_->new_lo_space()->FreeDeadObjects([](HeapObject) { return true; });
 
   RememberedSet<OLD_TO_NEW>::IterateMemoryChunks(heap_, [](MemoryChunk* chunk) {
-    if (chunk->SweepingDone()) {
-      RememberedSet<OLD_TO_NEW>::FreeEmptyBuckets(chunk);
-    } else {
-      RememberedSet<OLD_TO_NEW>::PreFreeEmptyBuckets(chunk);
-    }
+    RememberedSet<OLD_TO_NEW>::FreeEmptyBuckets(chunk);
   });
 
   // Update how much has survived scavenge.

src/heap/slot-set.h

@@ -30,9 +30,6 @@ class SlotSet : public Malloced {
  public:
   enum EmptyBucketMode {
     FREE_EMPTY_BUCKETS,     // An empty bucket will be deallocated immediately.
-    PREFREE_EMPTY_BUCKETS,  // An empty bucket will be unlinked from the slot
-                            // set, but deallocated on demand by a sweeper
-                            // thread.
     KEEP_EMPTY_BUCKETS      // An empty bucket will be kept.
   };
 
@@ -46,7 +43,6 @@ class SlotSet : public Malloced {
     for (int i = 0; i < kBuckets; i++) {
       ReleaseBucket(i);
     }
-    FreeToBeFreedBuckets();
   }
 
   // The slot offset specifies a slot at address page_start_ + slot_offset.
@@ -136,9 +132,7 @@ class SlotSet : public Malloced {
     DCHECK(current_bucket == end_bucket ||
            (current_bucket < end_bucket && current_cell == 0));
     while (current_bucket < end_bucket) {
-      if (mode == PREFREE_EMPTY_BUCKETS) {
-        PreFreeEmptyBucket(current_bucket);
-      } else if (mode == FREE_EMPTY_BUCKETS) {
+      if (mode == FREE_EMPTY_BUCKETS) {
         ReleaseBucket(current_bucket);
       } else {
         DCHECK(mode == KEEP_EMPTY_BUCKETS);
@@ -150,11 +144,11 @@ class SlotSet : public Malloced {
       current_bucket++;
     }
     // All buckets between start_bucket and end_bucket are cleared.
+    DCHECK(current_bucket == end_bucket);
+    if (current_bucket == kBuckets) return;
     bucket = LoadBucket(&buckets_[current_bucket]);
-    DCHECK(current_bucket == end_bucket && current_cell <= end_cell);
-    if (current_bucket == kBuckets || bucket == nullptr) {
-      return;
-    }
+    DCHECK(current_cell <= end_cell);
+    if (bucket == nullptr) return;
     while (current_cell < end_cell) {
       StoreCell(&bucket[current_cell], 0);
       current_cell++;
@@ -216,31 +210,12 @@ class SlotSet : public Malloced {
             }
           }
         }
-        if (mode == PREFREE_EMPTY_BUCKETS && in_bucket_count == 0) {
-          PreFreeEmptyBucket(bucket_index);
-        }
         new_count += in_bucket_count;
       }
     }
     return new_count;
   }
 
-  int NumberOfPreFreedEmptyBuckets() {
-    base::MutexGuard guard(&to_be_freed_buckets_mutex_);
-    return static_cast<int>(to_be_freed_buckets_.size());
-  }
-
-  void PreFreeEmptyBuckets() {
-    for (int bucket_index = 0; bucket_index < kBuckets; bucket_index++) {
-      Bucket bucket = LoadBucket(&buckets_[bucket_index]);
-      if (bucket != nullptr) {
-        if (IsEmptyBucket(bucket)) {
-          PreFreeEmptyBucket(bucket_index);
-        }
-      }
-    }
-  }
-
   void FreeEmptyBuckets() {
     for (int bucket_index = 0; bucket_index < kBuckets; bucket_index++) {
       Bucket bucket = LoadBucket(&buckets_[bucket_index]);
@@ -252,16 +227,6 @@ class SlotSet : public Malloced {
     }
   }
 
-  void FreeToBeFreedBuckets() {
-    base::MutexGuard guard(&to_be_freed_buckets_mutex_);
-    while (!to_be_freed_buckets_.empty()) {
-      Bucket top = to_be_freed_buckets_.top();
-      to_be_freed_buckets_.pop();
-      DeleteArray<uint32_t>(top);
-    }
-    DCHECK_EQ(0u, to_be_freed_buckets_.size());
-  }
-
  private:
   using Bucket = uint32_t*;
   static const int kMaxSlots = (1 << kPageSizeBits) / kTaggedSize;
@@ -291,15 +256,6 @@ class SlotSet : public Malloced {
     }
   }
 
-  void PreFreeEmptyBucket(int bucket_index) {
-    Bucket bucket = LoadBucket(&buckets_[bucket_index]);
-    if (bucket != nullptr) {
-      base::MutexGuard guard(&to_be_freed_buckets_mutex_);
-      to_be_freed_buckets_.push(bucket);
-      StoreBucket(&buckets_[bucket_index], nullptr);
-    }
-  }
-
   void ReleaseBucket(int bucket_index) {
     Bucket bucket = LoadBucket(&buckets_[bucket_index]);
     StoreBucket(&buckets_[bucket_index], nullptr);
@@ -379,8 +335,6 @@ class SlotSet : public Malloced {
   }
 
   Bucket buckets_[kBuckets];
-  base::Mutex to_be_freed_buckets_mutex_;
-  std::stack<uint32_t*> to_be_freed_buckets_;
 };
 
 enum SlotType {

src/heap/sweeper.cc

@@ -467,10 +467,6 @@ int Sweeper::ParallelSweepPage(
     if (typed_slot_set) {
       typed_slot_set->FreeToBeFreedChunks();
     }
-    SlotSet* slot_set = page->slot_set<OLD_TO_NEW>();
-    if (slot_set) {
-      slot_set->FreeToBeFreedBuckets();
-    }
   }
 
   {

test/cctest/heap/test-heap.cc

@@ -6040,56 +6040,61 @@ TEST(RememberedSetRemoveRange) {
     RememberedSet<OLD_TO_NEW>::Insert<AccessMode::ATOMIC>(chunk, x.first);
   }
 
-  RememberedSet<OLD_TO_NEW>::Iterate(chunk,
-                                     [&slots](MaybeObjectSlot slot) {
-                                       CHECK(slots[slot.address()]);
-                                       return KEEP_SLOT;
-                                     },
-                                     SlotSet::PREFREE_EMPTY_BUCKETS);
+  RememberedSet<OLD_TO_NEW>::Iterate(
+      chunk,
+      [&slots](MaybeObjectSlot slot) {
+        CHECK(slots[slot.address()]);
+        return KEEP_SLOT;
+      },
+      SlotSet::FREE_EMPTY_BUCKETS);
 
   RememberedSet<OLD_TO_NEW>::RemoveRange(chunk, start, start + kTaggedSize,
                                          SlotSet::FREE_EMPTY_BUCKETS);
   slots[start] = false;
-  RememberedSet<OLD_TO_NEW>::Iterate(chunk,
-                                     [&slots](MaybeObjectSlot slot) {
-                                       CHECK(slots[slot.address()]);
-                                       return KEEP_SLOT;
-                                     },
-                                     SlotSet::PREFREE_EMPTY_BUCKETS);
+  RememberedSet<OLD_TO_NEW>::Iterate(
+      chunk,
+      [&slots](MaybeObjectSlot slot) {
+        CHECK(slots[slot.address()]);
+        return KEEP_SLOT;
+      },
+      SlotSet::FREE_EMPTY_BUCKETS);
 
   RememberedSet<OLD_TO_NEW>::RemoveRange(chunk, start + kTaggedSize,
                                          start + Page::kPageSize,
                                          SlotSet::FREE_EMPTY_BUCKETS);
   slots[start + kTaggedSize] = false;
   slots[start + Page::kPageSize - kTaggedSize] = false;
-  RememberedSet<OLD_TO_NEW>::Iterate(chunk,
-                                     [&slots](MaybeObjectSlot slot) {
-                                       CHECK(slots[slot.address()]);
-                                       return KEEP_SLOT;
-                                     },
-                                     SlotSet::PREFREE_EMPTY_BUCKETS);
+  RememberedSet<OLD_TO_NEW>::Iterate(
+      chunk,
+      [&slots](MaybeObjectSlot slot) {
+        CHECK(slots[slot.address()]);
+        return KEEP_SLOT;
+      },
+      SlotSet::FREE_EMPTY_BUCKETS);
 
   RememberedSet<OLD_TO_NEW>::RemoveRange(chunk, start,
                                          start + Page::kPageSize + kTaggedSize,
                                          SlotSet::FREE_EMPTY_BUCKETS);
   slots[start + Page::kPageSize] = false;
-  RememberedSet<OLD_TO_NEW>::Iterate(chunk,
-                                     [&slots](MaybeObjectSlot slot) {
-                                       CHECK(slots[slot.address()]);
-                                       return KEEP_SLOT;
-                                     },
-                                     SlotSet::PREFREE_EMPTY_BUCKETS);
+  RememberedSet<OLD_TO_NEW>::Iterate(
+      chunk,
+      [&slots](MaybeObjectSlot slot) {
+        CHECK(slots[slot.address()]);
+        return KEEP_SLOT;
+      },
+      SlotSet::FREE_EMPTY_BUCKETS);
 
   RememberedSet<OLD_TO_NEW>::RemoveRange(chunk, chunk->area_end() - kTaggedSize,
                                          chunk->area_end(),
                                          SlotSet::FREE_EMPTY_BUCKETS);
   slots[chunk->area_end() - kTaggedSize] = false;
-  RememberedSet<OLD_TO_NEW>::Iterate(chunk,
-                                     [&slots](MaybeObjectSlot slot) {
-                                       CHECK(slots[slot.address()]);
-                                       return KEEP_SLOT;
-                                     },
-                                     SlotSet::PREFREE_EMPTY_BUCKETS);
+  RememberedSet<OLD_TO_NEW>::Iterate(
+      chunk,
+      [&slots](MaybeObjectSlot slot) {
+        CHECK(slots[slot.address()]);
+        return KEEP_SLOT;
+      },
+      SlotSet::FREE_EMPTY_BUCKETS);
 }
 
 HEAP_TEST(Regress670675) {
@@ -6185,53 +6190,6 @@ HEAP_TEST(Regress5831) {
   CHECK(chunk->NeverEvacuate());
 }
 
-TEST(Regress6800) {
-  CcTest::InitializeVM();
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope handle_scope(isolate);
-
-  const int kRootLength = 1000;
-  Handle<FixedArray> root =
-      isolate->factory()->NewFixedArray(kRootLength, AllocationType::kOld);
-  {
-    HandleScope inner_scope(isolate);
-    Handle<FixedArray> new_space_array = isolate->factory()->NewFixedArray(1);
-    for (int i = 0; i < kRootLength; i++) {
-      root->set(i, *new_space_array);
-    }
-    for (int i = 0; i < kRootLength; i++) {
-      root->set(i, ReadOnlyRoots(CcTest::heap()).undefined_value());
-    }
-  }
-  CcTest::CollectGarbage(NEW_SPACE);
-  CHECK_EQ(0, RememberedSet<OLD_TO_NEW>::NumberOfPreFreedEmptyBuckets(
-                  MemoryChunk::FromHeapObject(*root)));
-}
-
-TEST(Regress6800LargeObject) {
-  CcTest::InitializeVM();
-  Isolate* isolate = CcTest::i_isolate();
-  HandleScope handle_scope(isolate);
-
-  const int kRootLength = i::kMaxRegularHeapObjectSize / kTaggedSize;
-  Handle<FixedArray> root =
-      isolate->factory()->NewFixedArray(kRootLength, AllocationType::kOld);
-  CcTest::heap()->lo_space()->Contains(*root);
-  {
-    HandleScope inner_scope(isolate);
-    Handle<FixedArray> new_space_array = isolate->factory()->NewFixedArray(1);
-    for (int i = 0; i < kRootLength; i++) {
-      root->set(i, *new_space_array);
-    }
-    for (int i = 0; i < kRootLength; i++) {
-      root->set(i, ReadOnlyRoots(CcTest::heap()).undefined_value());
-    }
-  }
-  CcTest::CollectGarbage(OLD_SPACE);
-  CHECK_EQ(0, RememberedSet<OLD_TO_NEW>::NumberOfPreFreedEmptyBuckets(
-                  MemoryChunk::FromHeapObject(*root)));
-}
-
 HEAP_TEST(RegressMissingWriteBarrierInAllocate) {
   if (!FLAG_incremental_marking) return;
   ManualGCScope manual_gc_scope;