[heap] Restructure RunAtomicPause helpers

Bug: chromium:843903
Change-Id: I4c5cce82cec4d60cf1d5cd4508031601363f16aa
Reviewed-on: https://chromium-review.googlesource.com/1193225
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587569}

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -966,7 +966,6 @@ void ThreadState::FinishSnapshot() {
   gc_state_ = kNoGCScheduled;
   SetGCPhase(GCPhase::kSweeping);
   SetGCPhase(GCPhase::kNone);
-  Heap().stats_collector()->NotifySweepingCompleted();
 }
 
 void ThreadState::AtomicPauseEpilogue(BlinkGC::MarkingType marking_type,
@@ -1045,11 +1044,19 @@ void ThreadState::CompleteSweep() {
     return;
 
   {
-    AtomicPauseScope atomic_pause_scope(this);
+    // CompleteSweep may be called during regular mutator exececution, from a
+    // task, or from the atomic pause in which the atomic scope has already been
+    // opened.
+    const bool was_in_atomic_pause = in_atomic_pause();
+    if (!was_in_atomic_pause)
+      EnterAtomicPause();
+    ScriptForbiddenScope script_forbidden;
     SweepForbiddenScope scope(this);
     ThreadHeapStatsCollector::EnabledScope stats_scope(
         Heap().stats_collector(), ThreadHeapStatsCollector::kCompleteSweep);
     Heap().CompleteSweep();
+    if (!was_in_atomic_pause)
+      LeaveAtomicPause();
   }
   PostSweep();
 }
@@ -1215,6 +1222,16 @@ void UpdateHistograms(const ThreadHeapStatsCollector::Event& event) {
 
 }  // namespace
 
+void ThreadState::UpdateStatisticsAfterSweeping() {
+  DCHECK(!IsSweepingInProgress());
+  DCHECK(Heap().stats_collector()->is_started());
+  Heap().stats_collector()->NotifySweepingCompleted();
+  if (IsMainThread())
+    UpdateHistograms(Heap().stats_collector()->previous());
+  // Emit trace counters for all threads.
+  UpdateTraceCounters(*Heap().stats_collector());
+}
+
 void ThreadState::PostSweep() {
   DCHECK(CheckThread());
 
@@ -1227,11 +1244,10 @@ void ThreadState::PostSweep() {
   for (auto* const observer : observers_)
     observer->OnCompleteSweepDone();
 
-  Heap().stats_collector()->NotifySweepingCompleted();
-  if (IsMainThread())
-    UpdateHistograms(Heap().stats_collector()->previous());
-  // Emit trace counters for all threads.
-  UpdateTraceCounters(*Heap().stats_collector());
+  if (!in_atomic_pause()) {
+    // Immediately update the statistics if running outside of the atomic pause.
+    UpdateStatisticsAfterSweeping();
+  }
 }
 
 void ThreadState::SafePoint(BlinkGC::StackState stack_state) {
@@ -1515,6 +1531,44 @@ void ThreadState::CollectGarbage(BlinkGC::StackState stack_state,
           << " reason: " << GcReasonString(reason);
 }
 
+void ThreadState::AtomicPauseMarkPrologue(BlinkGC::StackState stack_state,
+                                          BlinkGC::MarkingType marking_type,
+                                          BlinkGC::GCReason reason) {
+  AtomicPausePrologue(stack_state, marking_type, reason);
+  MarkPhaseVisitRoots();
+  MarkPhaseVisitNotFullyConstructedObjects();
+}
+
+void ThreadState::AtomicPauseMarkTransitiveClosure() {
+  CHECK(MarkPhaseAdvanceMarking(TimeTicks::Max()));
+}
+
+void ThreadState::AtomicPauseMarkEpilogue(BlinkGC::MarkingType marking_type) {
+  MarkPhaseEpilogue(marking_type);
+}
+
+void ThreadState::AtomicPauseSweepAndCompact(
+    BlinkGC::MarkingType marking_type,
+    BlinkGC::SweepingType sweeping_type) {
+  AtomicPauseScope atomic_pause_scope(this);
+  AtomicPauseEpilogue(marking_type, sweeping_type);
+  if (marking_type == BlinkGC::kTakeSnapshot) {
+    FinishSnapshot();
+    CHECK(!IsSweepingInProgress());
+    CHECK_EQ(GetGCState(), kNoGCScheduled);
+    return;
+  }
+  DCHECK(IsSweepingInProgress());
+  if (sweeping_type == BlinkGC::kEagerSweeping) {
+    // Eager sweeping should happen only in testing.
+    CompleteSweep();
+  } else {
+    DCHECK(sweeping_type == BlinkGC::kLazySweeping);
+    // The default behavior is lazy sweeping.
+    ScheduleIdleLazySweep();
+  }
+}
+
 void ThreadState::RunAtomicPause(BlinkGC::StackState stack_state,
                                  BlinkGC::MarkingType marking_type,
                                  BlinkGC::SweepingType sweeping_type,
@@ -1522,35 +1576,23 @@ void ThreadState::RunAtomicPause(BlinkGC::StackState stack_state,
   {
     ThreadHeapStatsCollector::DevToolsScope stats1(
         Heap().stats_collector(), ThreadHeapStatsCollector::kAtomicPhase);
-    AtomicPauseScope atomic_pause_scope(this);
     {
+      AtomicPauseScope atomic_pause_scope(this);
       ThreadHeapStatsCollector::EnabledScope stats2(
           Heap().stats_collector(),
           ThreadHeapStatsCollector::kAtomicPhaseMarking, "lazySweeping",
           sweeping_type == BlinkGC::kLazySweeping ? "yes" : "no", "gcReason",
           GcReasonString(reason));
-      AtomicPausePrologue(stack_state, marking_type, reason);
-      MarkPhaseVisitRoots();
-      MarkPhaseVisitNotFullyConstructedObjects();
-      CHECK(MarkPhaseAdvanceMarking(TimeTicks::Max()));
-      MarkPhaseEpilogue(marking_type);
+      AtomicPauseMarkPrologue(stack_state, marking_type, reason);
+      AtomicPauseMarkTransitiveClosure();
+      AtomicPauseMarkEpilogue(marking_type);
     }
-    AtomicPauseEpilogue(marking_type, sweeping_type);
+    AtomicPauseSweepAndCompact(marking_type, sweeping_type);
   }
-  if (marking_type == BlinkGC::kTakeSnapshot) {
-    FinishSnapshot();
-    CHECK(!IsSweepingInProgress());
-    CHECK_EQ(GetGCState(), kNoGCScheduled);
-    return;
-  }
-  DCHECK(IsSweepingInProgress());
-  if (sweeping_type == BlinkGC::kEagerSweeping) {
-    // Eager sweeping should happen only in testing.
-    CompleteSweep();
-  } else {
-    DCHECK(sweeping_type == BlinkGC::kLazySweeping);
-    // The default behavior is lazy sweeping.
-    ScheduleIdleLazySweep();
+  if (!IsSweepingInProgress()) {
+    // Sweeping was finished during the atomic pause. Update statistics needs to
+    // run outside of the top-most stats scope.
+    UpdateStatisticsAfterSweeping();
   }
 }
 

third_party/blink/renderer/platform/heap/thread_state.h

@@ -560,14 +560,6 @@ class PLATFORM_EXPORT ThreadState final
   }
 
  private:
-  // Needs to set up visitor for testing purposes.
-  friend class incremental_marking_test::IncrementalMarkingScope;
-  friend class incremental_marking_test::IncrementalMarkingTestDriver;
-  template <typename T>
-  friend class PrefinalizerRegistration;
-  friend class TestGCScope;
-  friend class ThreadStateSchedulingTest;
-
   // Number of ThreadState's that are currently in incremental marking. The
   // counter is incremented by one when some ThreadState enters incremental
   // marking and decremented upon finishing.
@@ -579,6 +571,24 @@ class PLATFORM_EXPORT ThreadState final
   ThreadState();
   ~ThreadState() override;
 
+  // The following methods are used to compose RunAtomicPause. Public users
+  // should use the CollectGarbage entrypoint. Internal users should use these
+  // methods to compose a full garbage collection.
+  void AtomicPauseMarkPrologue(BlinkGC::StackState,
+                               BlinkGC::MarkingType,
+                               BlinkGC::GCReason);
+  void AtomicPauseMarkTransitiveClosure();
+  void AtomicPauseMarkEpilogue(BlinkGC::MarkingType);
+  void AtomicPauseSweepAndCompact(BlinkGC::MarkingType marking_type,
+                                  BlinkGC::SweepingType sweeping_type);
+
+  void RunAtomicPause(BlinkGC::StackState,
+                      BlinkGC::MarkingType,
+                      BlinkGC::SweepingType,
+                      BlinkGC::GCReason);
+
+  void UpdateStatisticsAfterSweeping();
+
   // The version is needed to be able to start incremental marking.
   void MarkPhasePrologue(BlinkGC::StackState,
                          BlinkGC::MarkingType,
@@ -593,11 +603,6 @@ class PLATFORM_EXPORT ThreadState final
   bool MarkPhaseAdvanceMarking(TimeTicks deadline);
   void VerifyMarking(BlinkGC::MarkingType);
 
-  void RunAtomicPause(BlinkGC::StackState,
-                      BlinkGC::MarkingType,
-                      BlinkGC::SweepingType,
-                      BlinkGC::GCReason);
-
   bool ShouldVerifyMarking() const;
 
   // shouldScheduleIdleGC and shouldForceConservativeGC
@@ -748,6 +753,14 @@ class PLATFORM_EXPORT ThreadState final
   };
   GCData current_gc_data_;
 
+  // Needs to set up visitor for testing purposes.
+  friend class incremental_marking_test::IncrementalMarkingScope;
+  friend class incremental_marking_test::IncrementalMarkingTestDriver;
+  template <typename T>
+  friend class PrefinalizerRegistration;
+  friend class TestGCScope;
+  friend class ThreadStateSchedulingTest;
+
   DISALLOW_COPY_AND_ASSIGN(ThreadState);
 };