cppgc: Fix PrepareForSweepVisitor

The visitor was removing pages while at the same time iterating them on
NormalPagedSpace.

Removing all pages at once is safe and should also be faster.

Bug: chromium:1056170
Change-Id: I56eedf6f09498f126cb09238e01962b48e75b657
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2190427
Reviewed-by: Omer Katz <[email protected]>
Reviewed-by: Anton Bikineev <[email protected]>
Commit-Queue: Michael Lippautz <[email protected]>
Cr-Commit-Position: refs/heads/master@{#67687}

Author: mlippautz

src/heap/cppgc/heap-space.cc

@@ -26,6 +26,12 @@ void BaseSpace::RemovePage(BasePage* page) {
   pages_.erase(it);
 }
 
+BaseSpace::Pages BaseSpace::RemoveAllPages() {
+  Pages pages = std::move(pages_);
+  pages_.clear();
+  return pages;
+}
+
 NormalPageSpace::NormalPageSpace(RawHeap* heap, size_t index)
     : BaseSpace(heap, index, PageType::kNormal) {}
 

src/heap/cppgc/heap-space.h

@@ -44,6 +44,7 @@ class V8_EXPORT_PRIVATE BaseSpace {
   // Page manipulation functions.
   void AddPage(BasePage*);
   void RemovePage(BasePage*);
+  Pages RemoveAllPages();
 
  protected:
   enum class PageType { kNormal, kLarge };

src/heap/cppgc/object-allocator-inl.h

@@ -37,7 +37,7 @@ void* ObjectAllocator::AllocateObject(size_t size, GCInfoIndex gcinfo,
 }
 
 // static
-inline RawHeap::RegularSpaceType ObjectAllocator::GetInitialSpaceIndexForSize(
+RawHeap::RegularSpaceType ObjectAllocator::GetInitialSpaceIndexForSize(
     size_t size) {
   if (size < 64) {
     if (size < 32) return RawHeap::RegularSpaceType::kNormal1;

src/heap/cppgc/sweeper.cc

@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "src/heap/cppgc/free-list.h"
+#include "src/heap/cppgc/globals.h"
 #include "src/heap/cppgc/heap-object-header-inl.h"
 #include "src/heap/cppgc/heap-object-header.h"
 #include "src/heap/cppgc/heap-page.h"
@@ -80,26 +81,17 @@ class PrepareForSweepVisitor final
   bool VisitNormalPageSpace(NormalPageSpace* space) {
     space->ResetLinearAllocationBuffer();
     space->free_list().Clear();
-    return false;
-  }
-
-  bool VisitNormalPage(NormalPage* page) {
-    MovePageToSweeper(page);
+    (*states_)[space->index()].unswept_pages = space->RemoveAllPages();
     return true;
   }
 
-  bool VisitLargePage(LargePage* page) {
-    MovePageToSweeper(page);
+  bool VisitLargePageSpace(LargePageSpace* space) {
+    (*states_)[space->index()].unswept_pages = space->RemoveAllPages();
+
     return true;
   }
 
  private:
-  void MovePageToSweeper(BasePage* page) {
-    BaseSpace* space = page->space();
-    space->RemovePage(page);
-    (*states_)[space->index()].unswept_pages.push_back(page);
-  }
-
   SpaceStates* states_;
 };
 

test/unittests/heap/cppgc/sweeper-unittest.cc

@@ -156,6 +156,18 @@ TEST_F(SweeperTest, SweepObjectsOnAllArenas) {
   EXPECT_EQ(5u, g_destructor_callcount);
 }
 
+TEST_F(SweeperTest, SweepMultiplePagesInSingleSpace) {
+  MakeGarbageCollected<GCed<2 * kLargeObjectSizeThreshold>>(GetHeap());
+  MakeGarbageCollected<GCed<2 * kLargeObjectSizeThreshold>>(GetHeap());
+  MakeGarbageCollected<GCed<2 * kLargeObjectSizeThreshold>>(GetHeap());
+
+  EXPECT_EQ(0u, g_destructor_callcount);
+
+  Sweep();
+
+  EXPECT_EQ(3u, g_destructor_callcount);
+}
+
 TEST_F(SweeperTest, CoalesceFreeListEntries) {
   constexpr size_t kObjectSize = 32;
   using Type = GCed<kObjectSize>;