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

This reverts commit 8325f86d.

Reason for revert: Speculative revert for chromium:1336850.

Original change's description:
> [heap] Sweep code pages on the background thread
>
> We already make code pages writable & executable for concurrent
> Sparkplug. We can use the same mechanism for sweeping of code pages on
> the background thread, instead of scheduling incremental tasks on the
> main thread. This allows us to remove almost all special
> handling for code pages in the sweeper and allows us to off-load more
> work from the main thread.
>
> Bug: v8:12967
> Change-Id: Idb8e9f8e2eadbec26a386f2de683a80087f671f3
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3695557
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#81139}

Bug: v8:12967, chromium:1336850
Change-Id: I1fb775892c2679984221efa7ceb682800c88cb2f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3707274
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81213}

src/heap/code-object-registry.cc

@@ -20,19 +20,29 @@ void CodeObjectRegistry::RegisterNewlyAllocatedCodeObject(Address code) {
   code_object_registry_.push_back(code);
 }
 
-void CodeObjectRegistry::ReinitializeFrom(std::vector<Address>&& code_objects) {
+void CodeObjectRegistry::RegisterAlreadyExistingCodeObject(Address code) {
+  // 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_);
+  DCHECK(code_object_registry_.empty() || code_object_registry_.back() < code);
+  code_object_registry_.push_back(code);
+}
 
-#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::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;
-  code_object_registry_ = std::move(code_objects);
+}
+
+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();
 }
 
 bool CodeObjectRegistry::Contains(Address object) const {

src/heap/code-object-registry.h

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

src/heap/sweeper.cc

@@ -22,6 +22,7 @@ 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) {}
 
@@ -100,6 +101,8 @@ 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;
     }
@@ -108,6 +111,33 @@ 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();
 }
@@ -141,6 +171,7 @@ void Sweeper::StartSweeperTasks() {
     job_handle_ = V8::GetCurrentPlatform()->PostJob(
         TaskPriority::kUserVisible,
         std::make_unique<SweeperJob>(heap_->isolate(), this));
+    ScheduleIncrementalSweepingTask();
   }
 }
 
@@ -287,8 +318,6 @@ 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
@@ -296,7 +325,7 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
   p->ResetAllocationStatistics();
 
   CodeObjectRegistry* code_object_registry = p->GetCodeObjectRegistry();
-  std::vector<Address> code_objects;
+  if (code_object_registry) code_object_registry->Clear();
 
   base::Optional<ActiveSystemPages> active_system_pages_after_sweeping;
   if (should_reduce_memory_) {
@@ -346,7 +375,8 @@ 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_objects.push_back(object.address());
+    if (code_object_registry)
+      code_object_registry->RegisterAlreadyExistingCodeObject(object.address());
     DCHECK(marking_state_->IsBlack(object));
     Address free_end = object.address();
     if (free_end != free_start) {
@@ -400,8 +430,7 @@ int Sweeper::RawSweep(Page* p, FreeSpaceTreatmentMode free_space_treatment_mode,
                                          *active_system_pages_after_sweeping);
   }
 
-  if (code_object_registry)
-    code_object_registry->ReinitializeFrom(std::move(code_objects));
+  if (code_object_registry) code_object_registry->Finalize();
   p->set_concurrent_sweeping_state(Page::ConcurrentSweepingState::kDone);
 
   return static_cast<int>(
@@ -428,6 +457,18 @@ 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) {
@@ -519,6 +560,17 @@ 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,6 +89,8 @@ 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);
 
@@ -107,6 +109,7 @@ class Sweeper {
   Page* GetSweptPageSafe(PagedSpaceBase* space);
 
  private:
+  class IncrementalSweeperTask;
   class SweeperJob;
 
   static const int kNumberOfSweepingSpaces =
@@ -161,6 +164,10 @@ 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);
 
@@ -183,6 +190,7 @@ 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.RegisterNewlyAllocatedCodeObject(i * offset);
+    registry.RegisterAlreadyExistingCodeObject(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.RegisterNewlyAllocatedCodeObject(i * offset);
+    registry.RegisterAlreadyExistingCodeObject(i * offset);
   }
 
   for (int i = 1; i <= elements; i++) {
@@ -69,5 +69,25 @@ 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