[heap] Move completion of sweeping before actual GC

This CL completes sweeping in Heap::PerformGarbageCollection before
invoking the actual collection. Collection code can now assume that
sweeping was already finished.

This helps with emitting the right epoch for sweeping and avoids a
data-race when updating the epoch while sweeping tasks are still running.

Bug: chromium:1154636
Change-Id: Ic9c4ac49568199d0ea48f17eea132079defe74a5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2573478
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71794}

src/heap/array-buffer-sweeper.cc

@@ -154,7 +154,7 @@ void ArrayBufferSweeper::RequestSweep(SweepingScope scope) {
           scope == SweepingScope::kYoung
               ? GCTracer::Scope::BACKGROUND_YOUNG_ARRAY_BUFFER_SWEEP
               : GCTracer::Scope::BACKGROUND_FULL_ARRAY_BUFFER_SWEEP;
-      TRACE_GC1(heap_->tracer(), scope_id, ThreadKind::kBackground);
+      TRACE_GC_EPOCH(heap_->tracer(), scope_id, ThreadKind::kBackground);
       base::MutexGuard guard(&sweeping_mutex_);
       job_->Sweep();
       job_finished_.NotifyAll();

src/heap/gc-tracer.cc

@@ -768,7 +768,6 @@ void GCTracer::PrintNVP() const {
           "incremental.finalize.external.prologue=%.1f "
           "incremental.finalize.external.epilogue=%.1f "
           "incremental.layout_change=%.1f "
-          "incremental.start=%.1f "
           "incremental.sweep_array_buffers=%.1f "
           "incremental.sweeping=%.1f "
           "incremental.embedder_prologue=%.1f "
@@ -860,7 +859,6 @@ void GCTracer::PrintNVP() const {
           current_.scopes[Scope::MC_INCREMENTAL_EXTERNAL_EPILOGUE],
           current_.scopes[Scope::MC_INCREMENTAL_LAYOUT_CHANGE],
           current_.scopes[Scope::MC_INCREMENTAL_START],
-          current_.scopes[Scope::MC_INCREMENTAL_SWEEP_ARRAY_BUFFERS],
           current_.scopes[Scope::MC_INCREMENTAL_SWEEPING],
           current_.scopes[Scope::MC_INCREMENTAL_EMBEDDER_PROLOGUE],
           current_.scopes[Scope::MC_INCREMENTAL_EMBEDDER_TRACING],

src/heap/heap.cc

@@ -1742,15 +1742,38 @@ void Heap::StartIncrementalMarking(int gc_flags,
                                    GCCallbackFlags gc_callback_flags) {
   DCHECK(incremental_marking()->IsStopped());
 
-  // The next GC cycle begins here.
-  UpdateEpochFull();
+  // Sweeping needs to be completed such that markbits are all cleared before
+  // starting marking again.
+  CompleteSweepingFull();
 
   SafepointScope safepoint(this);
+
+#ifdef DEBUG
+  VerifyCountersAfterSweeping();
+#endif
+
+  // Now that sweeping is completed, we can update the current epoch for the new
+  // full collection.
+  UpdateEpochFull();
+
   set_current_gc_flags(gc_flags);
   current_gc_callback_flags_ = gc_callback_flags;
   incremental_marking()->Start(gc_reason);
 }
 
+void Heap::CompleteSweepingFull() {
+  TRACE_GC_EPOCH(tracer(), GCTracer::Scope::MC_COMPLETE_SWEEPING,
+                 ThreadKind::kMain);
+
+  {
+    TRACE_GC(tracer(), GCTracer::Scope::MC_COMPLETE_SWEEP_ARRAY_BUFFERS);
+    array_buffer_sweeper()->EnsureFinished();
+  }
+
+  mark_compact_collector()->EnsureSweepingCompleted();
+  DCHECK(!mark_compact_collector()->sweeping_in_progress());
+}
+
 void Heap::StartIncrementalMarkingIfAllocationLimitIsReached(
     int gc_flags, const GCCallbackFlags gc_callback_flags) {
   if (incremental_marking()->IsStopped()) {
@@ -1977,6 +2000,15 @@ size_t Heap::PerformGarbageCollection(
   DisallowJavascriptExecution no_js(isolate());
   base::Optional<SafepointScope> optional_safepoint_scope;
 
+  if (IsYoungGenerationCollector(collector)) {
+    CompleteSweepingYoung(collector);
+  } else {
+    DCHECK_EQ(GarbageCollector::MARK_COMPACTOR, collector);
+    CompleteSweepingFull();
+  }
+
+  // The last GC cycle is done after completing sweeping. Start the next GC
+  // cycle.
   UpdateCurrentEpoch(collector);
 
   // Stop time-to-collection timer before safepoint - we do not want to measure
@@ -2074,6 +2106,24 @@ size_t Heap::PerformGarbageCollection(
   return freed_global_handles;
 }
 
+void Heap::CompleteSweepingYoung(GarbageCollector collector) {
+  GCTracer::Scope::ScopeId scope_id;
+
+  switch (collector) {
+    case GarbageCollector::MINOR_MARK_COMPACTOR:
+      scope_id = GCTracer::Scope::MINOR_MC_COMPLETE_SWEEP_ARRAY_BUFFERS;
+      break;
+    case GarbageCollector::SCAVENGER:
+      scope_id = GCTracer::Scope::SCAVENGER_COMPLETE_SWEEP_ARRAY_BUFFERS;
+      break;
+    default:
+      UNREACHABLE();
+  }
+
+  TRACE_GC_EPOCH(tracer(), scope_id, ThreadKind::kMain);
+  array_buffer_sweeper()->EnsureFinished();
+}
+
 void Heap::UpdateCurrentEpoch(GarbageCollector collector) {
   if (IsYoungGenerationCollector(collector)) {
     epoch_young_ = next_epoch();

src/heap/heap.h

@@ -514,7 +514,6 @@ class Heap {
   void NotifyOldGenerationExpansion(AllocationSpace space, MemoryChunk* chunk);
 
   void UpdateCurrentEpoch(GarbageCollector collector);
-  void UpdateEpochFull();
 
   inline Address* NewSpaceAllocationTopAddress();
   inline Address* NewSpaceAllocationLimitAddress();
@@ -1073,6 +1072,9 @@ class Heap {
   V8_EXPORT_PRIVATE void FinalizeIncrementalMarkingAtomically(
       GarbageCollectionReason gc_reason);
 
+  void CompleteSweepingFull();
+  void CompleteSweepingYoung(GarbageCollector collector);
+
   IncrementalMarking* incremental_marking() {
     return incremental_marking_.get();
   }
@@ -1566,6 +1568,8 @@ class Heap {
   CollectionEpoch epoch_young() { return epoch_young_; }
   CollectionEpoch epoch_full() { return epoch_full_; }
 
+  void UpdateEpochFull();
+
  private:
   using ExternalStringTableUpdaterCallback = String (*)(Heap* heap,
                                                         FullObjectSlot pointer);

src/heap/incremental-marking.cc

@@ -6,7 +6,6 @@
 
 #include "src/codegen/compilation-cache.h"
 #include "src/execution/vm-state-inl.h"
-#include "src/heap/array-buffer-sweeper.h"
 #include "src/heap/concurrent-marking.h"
 #include "src/heap/embedder-tracing.h"
 #include "src/heap/gc-idle-time-handler.h"
@@ -22,7 +21,6 @@
 #include "src/heap/objects-visiting-inl.h"
 #include "src/heap/objects-visiting.h"
 #include "src/heap/safepoint.h"
-#include "src/heap/sweeper.h"
 #include "src/init/v8.h"
 #include "src/numbers/conversions.h"
 #include "src/objects/data-handler-inl.h"
@@ -148,6 +146,8 @@ bool IncrementalMarking::IsBelowActivationThresholds() const {
 }
 
 void IncrementalMarking::Start(GarbageCollectionReason gc_reason) {
+  DCHECK(!collector_->sweeping_in_progress());
+
   if (FLAG_trace_incremental_marking) {
     const size_t old_generation_size_mb =
         heap()->OldGenerationSizeOfObjects() / MB;
@@ -194,17 +194,6 @@ void IncrementalMarking::Start(GarbageCollectionReason gc_reason) {
   bytes_marked_concurrently_ = 0;
   was_activated_ = true;
 
-  {
-    TRACE_GC(heap()->tracer(),
-             GCTracer::Scope::MC_INCREMENTAL_SWEEP_ARRAY_BUFFERS);
-    heap_->array_buffer_sweeper()->EnsureFinished();
-  }
-
-  collector_->EnsureSweepingCompleted();
-  DCHECK(!collector_->sweeping_in_progress());
-#ifdef DEBUG
-  heap_->VerifyCountersAfterSweeping();
-#endif
   StartMarking();
 
   heap_->AddAllocationObserversToAllSpaces(&old_generation_observer_,

src/heap/mark-compact.cc

@@ -837,15 +837,7 @@ void MarkCompactCollector::Prepare() {
 #endif
 
   DCHECK(!FLAG_never_compact || !FLAG_always_compact);
-
-  // Instead of waiting we could also abort the sweeper threads here.
-  EnsureSweepingCompleted();
-
-  {
-    TRACE_GC(heap()->tracer(),
-             GCTracer::Scope::MC_COMPLETE_SWEEP_ARRAY_BUFFERS);
-    heap_->array_buffer_sweeper()->EnsureFinished();
-  }
+  DCHECK(!sweeping_in_progress());
 
   if (!was_marked_incrementally_) {
     {

src/heap/scavenger.cc

@@ -253,12 +253,6 @@ class V8_NODISCARD ScopedFullHeapCrashKey {
 void ScavengerCollector::CollectGarbage() {
   ScopedFullHeapCrashKey collect_full_heap_dump_if_crash(isolate_);
 
-  {
-    TRACE_GC(heap_->tracer(),
-             GCTracer::Scope::SCAVENGER_COMPLETE_SWEEP_ARRAY_BUFFERS);
-    heap_->array_buffer_sweeper()->EnsureFinished();
-  }
-
   DCHECK(surviving_new_large_objects_.empty());
   std::vector<std::unique_ptr<Scavenger>> scavengers;
   Worklist<MemoryChunk*, 64> empty_chunks;

src/heap/sweeper.cc

@@ -89,8 +89,8 @@ class Sweeper::SweeperJob final : public JobTask {
       TRACE_GC(tracer_, GCTracer::Scope::MC_SWEEP);
       RunImpl(delegate);
     } else {
-      TRACE_GC1(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING,
-                ThreadKind::kBackground);
+      TRACE_GC_EPOCH(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING,
+                     ThreadKind::kBackground);
       RunImpl(delegate);
     }
   }
@@ -602,8 +602,8 @@ class Sweeper::IterabilityTask final : public CancelableTask {
 
  private:
   void RunInternal() final {
-    TRACE_GC1(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING,
-              ThreadKind::kBackground);
+    TRACE_GC_EPOCH(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING,
+                   ThreadKind::kBackground);
     for (Page* page : sweeper_->iterability_list_) {
       sweeper_->MakeIterable(page);
     }

src/init/heap-symbols.h

@@ -403,7 +403,6 @@
   F(MC_INCREMENTAL_FINALIZE_BODY)                                  \
   F(MC_INCREMENTAL_LAYOUT_CHANGE)                                  \
   F(MC_INCREMENTAL_START)                                          \
-  F(MC_INCREMENTAL_SWEEP_ARRAY_BUFFERS)                            \
   F(MC_INCREMENTAL_SWEEPING)
 
 #define TOP_MC_SCOPES(F) \
@@ -438,6 +437,7 @@
   F(MC_CLEAR_WEAK_LISTS)                             \
   F(MC_CLEAR_WEAK_REFERENCES)                        \
   F(MC_COMPLETE_SWEEP_ARRAY_BUFFERS)                 \
+  F(MC_COMPLETE_SWEEPING)                            \
   F(MC_EVACUATE_CANDIDATES)                          \
   F(MC_EVACUATE_CLEAN_UP)                            \
   F(MC_EVACUATE_COPY)                                \
@@ -473,6 +473,7 @@
   F(MINOR_MC_CLEAR)                                  \
   F(MINOR_MC_CLEAR_STRING_TABLE)                     \
   F(MINOR_MC_CLEAR_WEAK_LISTS)                       \
+  F(MINOR_MC_COMPLETE_SWEEP_ARRAY_BUFFERS)           \
   F(MINOR_MC_EVACUATE)                               \
   F(MINOR_MC_EVACUATE_CLEAN_UP)                      \
   F(MINOR_MC_EVACUATE_COPY)                          \
@@ -524,10 +525,12 @@
   F(MINOR_MC_BACKGROUND_MARKING)                  \
   F(SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL)
 
-#define TRACER_YOUNG_EPOCH_SCOPES(F)     \
-  F(BACKGROUND_YOUNG_ARRAY_BUFFER_SWEEP) \
-  F(MINOR_MARK_COMPACTOR)                \
-  F(SCAVENGER)                           \
-  F(SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL)
+#define TRACER_YOUNG_EPOCH_SCOPES(F)        \
+  F(BACKGROUND_YOUNG_ARRAY_BUFFER_SWEEP)    \
+  F(MINOR_MARK_COMPACTOR)                   \
+  F(MINOR_MC_COMPLETE_SWEEP_ARRAY_BUFFERS)  \
+  F(SCAVENGER)                              \
+  F(SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL) \
+  F(SCAVENGER_COMPLETE_SWEEP_ARRAY_BUFFERS)
 
 #endif  // V8_INIT_HEAP_SYMBOLS_H_