Reland "[heap] Sweep code pages on the background thread"

This reverts commit 6ddf042f.

Revert did not fix the crasher.

Bug: v8:12967, chromium:1336850
Change-Id: I6d474644e3d94c14df17af6efa70747bae6ad652
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3716487Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81290}

src/heap/code-object-registry.cc

@@ -20,29 +20,19 @@ void CodeObjectRegistry::RegisterNewlyAllocatedCodeObject(Address code) {
   code_object_registry_.push_back(code);
 }
 
-void CodeObjectRegistry::RegisterAlreadyExistingCodeObject(Address code) {
-  // Only called by the sweeper. Still uses a mutex to protect against accesses
-  // from the sampling profiler.
+void CodeObjectRegistry::ReinitializeFrom(std::vector<Address>&& code_objects) {
   base::RecursiveMutexGuard guard(&code_object_registry_mutex_);
-  DCHECK(is_sorted_);
-  DCHECK(code_object_registry_.empty() || code_object_registry_.back() < code);
-  code_object_registry_.push_back(code);
-}
 
-void CodeObjectRegistry::Clear() {
-  // Only called by the sweeper. Still uses a mutex to protect against accesses
-  // from the sampling profiler.
-  base::RecursiveMutexGuard guard(&code_object_registry_mutex_);
-  code_object_registry_.clear();
-  is_sorted_ = true;
-}
+#if DEBUG
+  Address last_start = kNullAddress;
+  for (Address object_start : code_objects) {
+    DCHECK_LT(last_start, object_start);
+    last_start = object_start;
+  }
+#endif  // DEBUG
 
-void CodeObjectRegistry::Finalize() {
-  // Only called by the sweeper. Still uses a mutex to protect against accesses
-  // from the sampling profiler.
-  base::RecursiveMutexGuard guard(&code_object_registry_mutex_);
-  DCHECK(is_sorted_);
-  code_object_registry_.shrink_to_fit();
+  is_sorted_ = true;
+  code_object_registry_ = std::move(code_objects);
 }
 
 bool CodeObjectRegistry::Contains(Address object) const {

src/heap/code-object-registry.h

@@ -21,9 +21,7 @@ namespace internal {
 class V8_EXPORT_PRIVATE CodeObjectRegistry {
  public:
   void RegisterNewlyAllocatedCodeObject(Address code);
-  void RegisterAlreadyExistingCodeObject(Address code);
-  void Clear();
-  void Finalize();
+  void ReinitializeFrom(std::vector<Address>&& code_objects);
   bool Contains(Address code) const;
   Address GetCodeObjectStartFromInnerAddress(Address address) const;
 

src/heap/sweeper.cc

@@ -22,7 +22,6 @@ namespace internal {
 Sweeper::Sweeper(Heap* heap, MajorNonAtomicMarkingState* marking_state)
     : heap_(heap),
       marking_state_(marking_state),
-      incremental_sweeper_pending_(false),
       sweeping_in_progress_(false),
       should_reduce_memory_(false) {}
 
@@ -101,8 +100,6 @@ class Sweeper::SweeperJob final : public JobTask {
       const AllocationSpace space_id = static_cast<AllocationSpace>(
           FIRST_GROWABLE_PAGED_SPACE +
           ((i + offset) % kNumberOfSweepingSpaces));
-      // Do not sweep code space concurrently.
-      if (space_id == CODE_SPACE) continue;
       DCHECK(IsValidSweepingSpace(space_id));
       if (!sweeper_->ConcurrentSweepSpace(space_id, delegate)) return;
     }
@@ -111,33 +108,6 @@ class Sweeper::SweeperJob final : public JobTask {
   GCTracer* const tracer_;
 };
 
-class Sweeper::IncrementalSweeperTask final : public CancelableTask {
- public:
-  IncrementalSweeperTask(Isolate* isolate, Sweeper* sweeper)
-      : CancelableTask(isolate), isolate_(isolate), sweeper_(sweeper) {}
-
-  ~IncrementalSweeperTask() override = default;
-
-  IncrementalSweeperTask(const IncrementalSweeperTask&) = delete;
-  IncrementalSweeperTask& operator=(const IncrementalSweeperTask&) = delete;
-
- private:
-  void RunInternal() final {
-    VMState<GC> state(isolate_);
-    TRACE_EVENT_CALL_STATS_SCOPED(isolate_, "v8", "V8.Task");
-    sweeper_->incremental_sweeper_pending_ = false;
-
-    if (sweeper_->sweeping_in_progress()) {
-      if (!sweeper_->IncrementalSweepSpace(CODE_SPACE)) {
-        sweeper_->ScheduleIncrementalSweepingTask();
-      }
-    }
-  }
-
-  Isolate* const isolate_;
-  Sweeper* const sweeper_;
-};
-
 void Sweeper::TearDown() {
   if (job_handle_ && job_handle_->IsValid()) job_handle_->Cancel();
 }
@@ -171,7 +141,6 @@ void Sweeper::StartSweeperTasks() {
     job_handle_ = V8::GetCurrentPlatform()->PostJob(
         TaskPriority::kUserVisible,
         std::make_unique<SweeperJob>(heap_->isolate(), this));
-    ScheduleIncrementalSweepingTask();
   }
 }
 
@@ -318,6 +287,8 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
   DCHECK(!p->IsEvacuationCandidate() && !p->SweepingDone());
 
   // Phase 1: Prepare the page for sweeping.
+  base::Optional<CodePageMemoryModificationScope> write_scope;
+  if (space->identity() == CODE_SPACE) write_scope.emplace(p);
 
   // Set the allocated_bytes_ counter to area_size and clear the wasted_memory_
   // counter. The free operations below will decrease allocated_bytes_ to actual
@@ -325,7 +296,7 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
   p->ResetAllocationStatistics();
 
   CodeObjectRegistry* code_object_registry = p->GetCodeObjectRegistry();
-  if (code_object_registry) code_object_registry->Clear();
+  std::vector<Address> code_objects;
 
   base::Optional<ActiveSystemPages> active_system_pages_after_sweeping;
   if (should_reduce_memory_) {
@@ -375,8 +346,7 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
   for (auto object_and_size :
        LiveObjectRange<kBlackObjects>(p, marking_state_->bitmap(p))) {
     HeapObject const object = object_and_size.first;
-    if (code_object_registry)
-      code_object_registry->RegisterAlreadyExistingCodeObject(object.address());
+    if (code_object_registry) code_objects.push_back(object.address());
     DCHECK(marking_state_->IsBlack(object));
     Address free_end = object.address();
     if (free_end != free_start) {
@@ -430,7 +400,8 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
                                          *active_system_pages_after_sweeping);
   }
 
-  if (code_object_registry) code_object_registry->Finalize();
+  if (code_object_registry)
+    code_object_registry->ReinitializeFrom(std::move(code_objects));
   p->set_concurrent_sweeping_state(Page::ConcurrentSweepingState::kDone);
 
   return static_cast<int>(
@@ -457,18 +428,6 @@ bool Sweeper::ConcurrentSweepSpace(AllocationSpace identity,
   return false;
 }
 
-bool Sweeper::IncrementalSweepSpace(AllocationSpace identity) {
-  TRACE_GC_EPOCH(heap_->tracer(), GCTracer::Scope::MC_INCREMENTAL_SWEEPING,
-                 ThreadKind::kMain);
-  const double start = heap_->MonotonicallyIncreasingTimeInMs();
-  if (Page* page = GetSweepingPageSafe(identity)) {
-    ParallelSweepPage(page, identity, SweepingMode::kLazyOrConcurrent);
-  }
-  const double duration = heap_->MonotonicallyIncreasingTimeInMs() - start;
-  heap_->tracer()->AddIncrementalSweepingStep(duration);
-  return sweeping_list_[GetSweepSpaceIndex(identity)].empty();
-}
-
 int Sweeper::ParallelSweepSpace(AllocationSpace identity,
                                 SweepingMode sweeping_mode,
                                 int required_freed_bytes, int max_pages) {
@@ -560,17 +519,6 @@ bool Sweeper::TryRemoveSweepingPageSafe(AllocationSpace space, Page* page) {
   return true;
 }
 
-void Sweeper::ScheduleIncrementalSweepingTask() {
-  if (!incremental_sweeper_pending_) {
-    incremental_sweeper_pending_ = true;
-    v8::Isolate* isolate = reinterpret_cast<v8::Isolate*>(heap_->isolate());
-    auto taskrunner =
-        V8::GetCurrentPlatform()->GetForegroundTaskRunner(isolate);
-    taskrunner->PostTask(
-        std::make_unique<IncrementalSweeperTask>(heap_->isolate(), this));
-  }
-}
-
 void Sweeper::AddPage(AllocationSpace space, Page* page,
                       Sweeper::AddPageMode mode) {
   base::MutexGuard guard(&mutex_);

src/heap/sweeper.h

@@ -89,8 +89,6 @@ class Sweeper {
 
   void EnsurePageIsSwept(Page* page);
 
-  void ScheduleIncrementalSweepingTask();
-
   int RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
                SweepingMode sweeping_mode, const base::MutexGuard& page_guard);
 
@@ -109,7 +107,6 @@ class Sweeper {
   Page* GetSweptPageSafe(PagedSpaceBase* space);
 
  private:
-  class IncrementalSweeperTask;
   class SweeperJob;
 
   static const int kNumberOfSweepingSpaces =
@@ -164,10 +161,6 @@ class Sweeper {
   // are no more pages to sweep in the given space.
   bool ConcurrentSweepSpace(AllocationSpace identity, JobDelegate* delegate);
 
-  // Sweeps incrementally one page from the given space. Returns true if
-  // there are no more pages to sweep in the given space.
-  bool IncrementalSweepSpace(AllocationSpace identity);
-
   Page* GetSweepingPageSafe(AllocationSpace space);
   bool TryRemoveSweepingPageSafe(AllocationSpace space, Page* page);
 
@@ -190,7 +183,6 @@ class Sweeper {
   base::ConditionVariable cv_page_swept_;
   SweptList swept_list_[kNumberOfSweepingSpaces];
   SweepingList sweeping_list_[kNumberOfSweepingSpaces];
-  bool incremental_sweeper_pending_;
   // Main thread can finalize sweeping, while background threads allocation slow
   // path checks this flag to see whether it could support concurrent sweeping.
   std::atomic<bool> sweeping_in_progress_;

test/unittests/heap/code-object-registry-unittest.cc

@@ -14,7 +14,7 @@ TEST(CodeObjectRegistry, RegisterAlreadyExistingObjectsAndContains) {
   const int elements = 10;
   const int offset = 100;
   for (int i = 0; i < elements; i++) {
-    registry.RegisterAlreadyExistingCodeObject(i * offset);
+    registry.RegisterNewlyAllocatedCodeObject(i * offset);
   }
 
   for (int i = 0; i < elements; i++) {
@@ -41,7 +41,7 @@ TEST(CodeObjectRegistry, FindAlreadyExistingObjects) {
   const int offset = 100;
   const int inner = 2;
   for (int i = 1; i <= elements; i++) {
-    registry.RegisterAlreadyExistingCodeObject(i * offset);
+    registry.RegisterNewlyAllocatedCodeObject(i * offset);
   }
 
   for (int i = 1; i <= elements; i++) {
@@ -69,25 +69,5 @@ TEST(CodeObjectRegistry, FindNewlyAllocatedObjects) {
   }
 }
 
-TEST(CodeObjectRegistry, FindAlternatingObjects) {
-  CodeObjectRegistry registry;
-  const int elements = 10;
-  const int offset = 100;
-  const int inner = 2;
-  for (int i = 1; i <= elements; i++) {
-    if (i % 2 == 0) {
-      registry.RegisterAlreadyExistingCodeObject(i * offset);
-    } else {
-      registry.RegisterNewlyAllocatedCodeObject(i * offset);
-    }
-  }
-
-  for (int i = 1; i <= elements; i++) {
-    for (int j = 0; j < inner; j++) {
-      CHECK_EQ(registry.GetCodeObjectStartFromInnerAddress(i * offset + j),
-               i * offset);
-    }
-  }
-}
 }  // namespace internal
 }  // namespace v8