Oilpan: Incremental Marking using Tasks

This patch moves incremental marking steps in Oilpan
from safe points to V8 task runner thereby letting
scheduler know about GC existence.

This is expected to regress atomic-pause duration,
since incremental steps are not as prioritized as
they used to be, but at the same time latency should
improve.

Bug: 1002998

Change-Id: I1f0fa3468160a3505fcf71f85655c0fd41dc1c67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1607642
Commit-Queue: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708724}

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

@@ -49,8 +49,7 @@ IncrementalMarkingTestDriver::~IncrementalMarkingTestDriver() {
 }
 
 void IncrementalMarkingTestDriver::Start() {
-  thread_state_->IncrementalMarkingStart(
-      BlinkGC::GCReason::kForcedGCForTesting);
+  thread_state_->IncrementalMarkingStartForTesting();
 }
 
 bool IncrementalMarkingTestDriver::SingleStep(BlinkGC::StackState stack_state) {
@@ -75,7 +74,7 @@ void IncrementalMarkingTestDriver::FinishGC(bool complete_sweep) {
   FinishSteps(BlinkGC::StackState::kNoHeapPointersOnStack);
   CHECK_EQ(ThreadState::kIncrementalMarkingFinalizeScheduled,
            thread_state_->GetGCState());
-  thread_state_->RunScheduledGC(BlinkGC::StackState::kNoHeapPointersOnStack);
+  thread_state_->IncrementalMarkingFinalize();
   CHECK(!thread_state_->IsIncrementalMarking());
   if (complete_sweep) {
     thread_state_->CompleteSweep();

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

@@ -253,7 +253,6 @@ class PLATFORM_EXPORT ThreadState final {
   void PerformConcurrentSweep();
 
   void SchedulePreciseGC();
-  void ScheduleIncrementalGC(BlinkGC::GCReason);
   void ScheduleForcedGCForTesting();
   void ScheduleGCIfNeeded();
   void WillStartV8GC(BlinkGC::V8GCType);
@@ -261,6 +260,10 @@ class PLATFORM_EXPORT ThreadState final {
   GCState GetGCState() const { return gc_state_; }
   void SetGCPhase(GCPhase);
 
+  // Immediately starts incremental marking and schedules further steps if
+  // necessary.
+  void StartIncrementalMarking(BlinkGC::GCReason);
+
   // Returns true if marking is in progress.
   bool IsMarkingInProgress() const { return gc_phase_ == GCPhase::kMarking; }
 
@@ -286,9 +289,6 @@ class PLATFORM_EXPORT ThreadState final {
 
   void EnableCompactionForNextGCForTesting();
 
-  void IncrementalMarkingStart(BlinkGC::GCReason);
-  void IncrementalMarkingStep(BlinkGC::StackState);
-  void IncrementalMarkingFinalize();
   bool FinishIncrementalMarkingIfRunning(BlinkGC::StackState,
                                          BlinkGC::MarkingType,
                                          BlinkGC::SweepingType,
@@ -382,6 +382,13 @@ class PLATFORM_EXPORT ThreadState final {
   bool IsVerifyMarkingEnabled() const;
 
  private:
+  class IncrementalMarkingScheduler;
+
+  // Duration of one incremental marking step. Should be short enough that it
+  // doesn't cause jank even though it is scheduled as a normal task.
+  static constexpr base::TimeDelta kDefaultIncrementalMarkingStepDuration =
+      base::TimeDelta::FromMilliseconds(2);
+
   // Stores whether some ThreadState is currently in incremental marking.
   static AtomicEntryFlag incremental_marking_flag_;
 
@@ -482,9 +489,15 @@ class PLATFORM_EXPORT ThreadState final {
   // Visit all DOM wrappers allocatd on this thread.
   void VisitDOMWrappers(Visitor*);
 
+  // Incremental marking implementation functions.
+  void IncrementalMarkingStartForTesting();
+  void IncrementalMarkingStart(BlinkGC::GCReason);
+  void IncrementalMarkingStep(
+      BlinkGC::StackState,
+      base::TimeDelta duration = kDefaultIncrementalMarkingStepDuration);
+  void IncrementalMarkingFinalize();
+
   // Schedule helpers.
-  void ScheduleIncrementalMarkingStep();
-  void ScheduleIncrementalMarkingFinalize();
   void ScheduleIdleLazySweep();
   void ScheduleConcurrentAndLazySweep();
 
@@ -493,8 +506,6 @@ class PLATFORM_EXPORT ThreadState final {
 
   void RunScheduledGC(BlinkGC::StackState);
 
-  void UpdateIncrementalMarkingStepDuration();
-
   void SynchronizeAndFinishConcurrentSweeping();
 
   void InvokePreFinalizers();
@@ -540,9 +551,6 @@ class PLATFORM_EXPORT ThreadState final {
   size_t gc_forbidden_count_ = 0;
   size_t static_persistent_registration_disabled_count_ = 0;
 
-  base::TimeDelta next_incremental_marking_step_duration_;
-  base::TimeDelta previous_incremental_marking_time_left_;
-
   GCState gc_state_ = GCState::kNoGCScheduled;
   GCPhase gc_phase_ = GCPhase::kNone;
   BlinkGC::GCReason reason_for_scheduled_gc_ =
@@ -583,6 +591,8 @@ class PLATFORM_EXPORT ThreadState final {
   };
   GCData current_gc_data_;
 
+  std::unique_ptr<IncrementalMarkingScheduler> incremental_marking_scheduler_;
+
   std::unique_ptr<CancelableTaskScheduler> marker_scheduler_;
   uint8_t active_markers_ = 0;
   base::Lock active_concurrent_markers_lock_;

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

@@ -2,15 +2,30 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/run_loop.h"
 #include "base/test/scoped_feature_list.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/renderer/platform/heap/heap_test_utilities.h"
 #include "third_party/blink/renderer/platform/heap/thread_state_scopes.h"
+#include "third_party/blink/renderer/platform/scheduler/public/thread_scheduler.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
+#include "third_party/blink/renderer/platform/wtf/functional.h"
 
 namespace blink {
 
+namespace {
+
+void RunLoop() {
+  base::RunLoop rl;
+  // Push quit task.
+  ThreadScheduler::Current()->V8TaskRunner()->PostNonNestableTask(
+      FROM_HERE, WTF::Bind(rl.QuitWhenIdleClosure()));
+  rl.Run();
+}
+
+}  // namespace
+
 class ThreadStateSchedulingTest : public TestSupportingGC {
  public:
   void SetUp() override {
@@ -26,13 +41,8 @@ class ThreadStateSchedulingTest : public TestSupportingGC {
     EXPECT_FALSE(state_->IsSweepingInProgress());
   }
 
-  void StartIncrementalMarking() {
-    EXPECT_EQ(ThreadState::kNoGCScheduled, state_->GetGCState());
-    state_->ScheduleIncrementalGC(BlinkGC::GCReason::kForcedGCForTesting);
-    state_->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
-    EXPECT_EQ(ThreadState::kIncrementalMarkingStepScheduled,
-              state_->GetGCState());
-    EXPECT_TRUE(state_->IsMarkingInProgress());
+  BlinkGC::GCReason LastReason() const {
+    return state_->reason_for_scheduled_gc_;
   }
 
   void StartLazySweepingForPreciseGC() {
@@ -58,21 +68,17 @@ class ThreadStateSchedulingTest : public TestSupportingGC {
   int initial_gc_age_;
 };
 
-TEST_F(ThreadStateSchedulingTest, ScheduleIncrementalV8FollowupGCAgain) {
+TEST_F(ThreadStateSchedulingTest, RunIncrementalGCForTesting) {
   ThreadStateSchedulingTest* test = this;
 
   EXPECT_EQ(ThreadState::kNoGCScheduled, test->state()->GetGCState());
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-
-  // Calling ScheduleIncrementalV8FollowupGC() while one is already scheduled
-  // will do nothing.
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
+  test->state()->StartIncrementalMarking(
+      BlinkGC::GCReason::kForcedGCForTesting);
+  EXPECT_EQ(ThreadState::kIncrementalMarkingStepScheduled,
+            test->state()->GetGCState());
 
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-  EXPECT_EQ(0, test->GCCount());
+  RunLoop();
+  EXPECT_EQ(ThreadState::kNoGCScheduled, test->state()->GetGCState());
 }
 
 TEST_F(ThreadStateSchedulingTest, SchedulePreciseGCWhileLazySweeping) {
@@ -87,27 +93,13 @@ TEST_F(ThreadStateSchedulingTest, SchedulePreciseGCWhileLazySweeping) {
   EXPECT_EQ(ThreadState::kPreciseGCScheduled, test->state()->GetGCState());
 }
 
-TEST_F(ThreadStateSchedulingTest,
-       ScheduleIncrementalV8FollowupGCWhileLazySweeping) {
-  ThreadStateSchedulingTest* test = this;
-
-  test->StartLazySweepingForPreciseGC();
-
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
-
-  // Scheduling a IncrementalV8FollowupGC should finish lazy sweeping.
-  EXPECT_FALSE(test->state()->IsSweepingInProgress());
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-}
-
 TEST_F(ThreadStateSchedulingTest, SchedulePreciseGCWhileIncrementalMarking) {
   ThreadStateSchedulingTest* test = this;
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
       blink::features::kBlinkHeapIncrementalMarking);
-
-  test->StartIncrementalMarking();
+  test->state()->StartIncrementalMarking(
+      BlinkGC::GCReason::kForcedGCForTesting);
   test->state()->SchedulePreciseGC();
   // Scheduling a precise GC should cancel incremental marking tasks.
   EXPECT_EQ(ThreadState::kPreciseGCScheduled, test->state()->GetGCState());
@@ -122,54 +114,10 @@ TEST_F(ThreadStateSchedulingTest, SchedulePreciseGCWhileIncrementalMarking) {
   EXPECT_EQ(0, test->GCCount());
   test->state()->CompleteSweep();
   EXPECT_EQ(1, test->GCCount());
-}
-
-TEST_F(ThreadStateSchedulingTest,
-       ScheduleIncrementalV8FollowupGCWhileIncrementalMarking) {
-  ThreadStateSchedulingTest* test = this;
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(
-      blink::features::kBlinkHeapIncrementalMarking);
-
-  test->StartIncrementalMarking();
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
-  // Scheduling another incremental GC should not cancel incremental marking
-  // tasks.
-  EXPECT_EQ(ThreadState::kIncrementalMarkingStepScheduled,
-            test->state()->GetGCState());
-}
-
-TEST_F(ThreadStateSchedulingTest,
-       ScheduleIncrementalV8FollowupGCWhileGCForbidden) {
-  ThreadStateSchedulingTest* test = this;
-
-  EXPECT_EQ(ThreadState::kNoGCScheduled, test->state()->GetGCState());
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-
-  ThreadState::GCForbiddenScope gc_forbidden_scope(test->state());
-  test->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
 
-  // Starting an IncrementalV8FollowupGC while GC is forbidden should do
-  // nothing.
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-  EXPECT_EQ(0, GCCount());
-}
-
-TEST_F(ThreadStateSchedulingTest, RunIncrementalV8FollowupGC) {
-  ThreadStateSchedulingTest* test = this;
-
-  EXPECT_EQ(ThreadState::kNoGCScheduled, test->state()->GetGCState());
-  test->state()->ScheduleIncrementalGC(
-      BlinkGC::GCReason::kIncrementalV8FollowupGC);
-  EXPECT_EQ(ThreadState::kIncrementalGCScheduled, test->state()->GetGCState());
-
-  test->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
-
-  EXPECT_EQ(ThreadState::kIncrementalMarkingStepScheduled,
-            test->state()->GetGCState());
+  // Check that incremental GC hasn't been run.
+  RunLoop();
+  EXPECT_EQ(1, test->GCCount());
 }
 
 }  // namespace blink

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

@@ -50,13 +50,12 @@ void UnifiedHeapController::TracePrologue(
       BlinkGC::kConcurrentAndLazySweeping,
       thread_state_->current_gc_data_.reason);
 
-  // Reset any previously scheduled garbage collections.
   thread_state_->SetGCState(ThreadState::kNoGCScheduled);
   BlinkGC::GCReason gc_reason =
       (v8_flags & v8::EmbedderHeapTracer::TraceFlags::kReduceMemory)
           ? BlinkGC::GCReason::kUnifiedHeapForMemoryReductionGC
           : BlinkGC::GCReason::kUnifiedHeapGC;
-  thread_state_->IncrementalMarkingStart(gc_reason);
+  thread_state_->StartIncrementalMarking(gc_reason);
 
   is_tracing_done_ = false;
 }

third_party/blink/renderer/platform/lifecycle_context_test.cc

@@ -29,7 +29,9 @@
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/heap/heap.h"
+#include "third_party/blink/renderer/platform/heap/heap_test_utilities.h"
 #include "third_party/blink/renderer/platform/heap/persistent.h"
+#include "third_party/blink/renderer/platform/heap/thread_state.h"
 #include "third_party/blink/renderer/platform/lifecycle_notifier.h"
 #include "third_party/blink/renderer/platform/lifecycle_observer.h"
 #include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
@@ -149,8 +151,8 @@ TEST(LifecycleContextTest, ShouldNotHitCFICheckOnIncrementalMarking) {
       {blink::features::kBlinkHeapIncrementalMarking},
       {blink::features::kBlinkHeapConcurrentMarking,
        blink::features::kBlinkHeapConcurrentSweeping});
-  ThreadState* thread_state = ThreadState::Current();
-  thread_state->IncrementalMarkingStart(BlinkGC::GCReason::kForcedGCForTesting);
+  IncrementalMarkingTestDriver driver(ThreadState::Current());
+  driver.Start();
 
   auto* context = MakeGarbageCollected<DummyContext>();
 
@@ -163,10 +165,7 @@ TEST(LifecycleContextTest, ShouldNotHitCFICheckOnIncrementalMarking) {
   EXPECT_TRUE(observer->ContextDestroyedCalled());
   context = nullptr;
 
-  while (thread_state->GetGCState() ==
-         ThreadState::kIncrementalMarkingStepScheduled)
-    thread_state->IncrementalMarkingStep(BlinkGC::kNoHeapPointersOnStack);
-  thread_state->IncrementalMarkingFinalize();
+  driver.FinishGC();
 }
 
 TEST(LifecycleContextTest, ForEachObserver) {