[Bink Heap]: Record concurrent marked bytes early.

Jobs API causes longer running concurrent marking task. Since marked
bytes is only recorded when the task exists, this causes the mutator
thread to underestimate progress and spend more time marking.
To avoid such regression, this CL makes concurrently_marked_bytes_
atomic and updates it more frequently.
Drive by: Remove unused kConcurrentMarkingStepDuration param that was
previously used for yielding.

Bug: 1119552
Change-Id: Ia91c994a4ad417456bbe07bd3f4190f69cba5c95
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2368498Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Omer Katz <omerkatz@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801006}

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

@@ -42,6 +42,7 @@
 #include "third_party/blink/renderer/platform/heap/blink_gc_memory_dump_provider.h"
 #include "third_party/blink/renderer/platform/heap/heap_compact.h"
 #include "third_party/blink/renderer/platform/heap/heap_stats_collector.h"
+#include "third_party/blink/renderer/platform/heap/marking_scheduling_oracle.h"
 #include "third_party/blink/renderer/platform/heap/marking_visitor.h"
 #include "third_party/blink/renderer/platform/heap/page_bloom_filter.h"
 #include "third_party/blink/renderer/platform/heap/page_memory.h"
@@ -341,18 +342,6 @@ bool DrainWorklistWithDeadline(base::TimeTicks deadline,
       [deadline]() { return deadline <= base::TimeTicks::Now(); }, task_id);
 }
 
-template <size_t kDeadlineCheckInterval = kDefaultDeadlineCheckInterval,
-          typename Worklist,
-          typename Callback>
-bool DrainWorklistWithYielding(base::JobDelegate* delegate,
-                               Worklist* worklist,
-                               Callback callback,
-                               int task_id) {
-  return DrainWorklist<kDeadlineCheckInterval>(
-      worklist, std::move(callback),
-      [delegate]() { return delegate->ShouldYield(); }, task_id);
-}
-
 }  // namespace
 
 bool ThreadHeap::InvokeEphemeronCallbacks(
@@ -524,30 +513,33 @@ size_t ThreadHeap::ConcurrentMarkingGlobalWorkSize() const {
          ephemeron_pairs_to_process_worklist_->GlobalPoolSize();
 }
 
-bool ThreadHeap::AdvanceConcurrentMarking(ConcurrentMarkingVisitor* visitor,
-                                          base::JobDelegate* delegate,
-                                          base::TimeTicks deadline) {
+bool ThreadHeap::AdvanceConcurrentMarking(
+    ConcurrentMarkingVisitor* visitor,
+    base::JobDelegate* delegate,
+    MarkingSchedulingOracle* marking_scheduler) {
+  auto should_yield_callback = [marking_scheduler, visitor, delegate]() {
+    marking_scheduler->AddConcurrentlyMarkedBytes(
+        visitor->RecentlyMarkedBytes());
+    return delegate->ShouldYield();
+  };
   bool finished;
   do {
     // Convert |previously_not_fully_constructed_worklist_| to
     // |marking_worklist_|. This merely re-adds items with the proper
     // callbacks.
-    finished =
-        DrainWorklistWithYielding<kDefaultConcurrentDeadlineCheckInterval>(
-            delegate, previously_not_fully_constructed_worklist_.get(),
-            [visitor](NotFullyConstructedItem& item) {
-              visitor->DynamicallyMarkAddress(
-                  reinterpret_cast<ConstAddress>(item));
-            },
-            visitor->task_id());
+    finished = DrainWorklist<kDefaultConcurrentDeadlineCheckInterval>(
+        previously_not_fully_constructed_worklist_.get(),
+        [visitor](NotFullyConstructedItem& item) {
+          visitor->DynamicallyMarkAddress(reinterpret_cast<ConstAddress>(item));
+        },
+        should_yield_callback, visitor->task_id());
     if (!finished)
       break;
 
     // Iteratively mark all objects that are reachable from the objects
     // currently pushed onto the marking worklist.
-    finished = DrainWorklistWithYielding<
-        kDefaultConcurrentDeadlineCheckInterval>(
-        delegate, marking_worklist_.get(),
+    finished = DrainWorklist<kDefaultConcurrentDeadlineCheckInterval>(
+        marking_worklist_.get(),
         [visitor](const MarkingItem& item) {
           HeapObjectHeader* header =
               HeapObjectHeader::FromPayload(item.base_object_payload);
@@ -558,13 +550,12 @@ bool ThreadHeap::AdvanceConcurrentMarking(ConcurrentMarkingVisitor* visitor,
           item.callback(visitor, item.base_object_payload);
           visitor->AccountMarkedBytes(header);
         },
-        visitor->task_id());
+        should_yield_callback, visitor->task_id());
     if (!finished)
       break;
 
-    finished = DrainWorklistWithYielding<
-        kDefaultConcurrentDeadlineCheckInterval>(
-        delegate, write_barrier_worklist_.get(),
+    finished = DrainWorklist<kDefaultConcurrentDeadlineCheckInterval>(
+        write_barrier_worklist_.get(),
         [visitor](HeapObjectHeader* header) {
           PageFromObject(header)->SynchronizedLoad();
           DCHECK(
@@ -573,7 +564,7 @@ bool ThreadHeap::AdvanceConcurrentMarking(ConcurrentMarkingVisitor* visitor,
           GCInfo::From(header->GcInfoIndex()).trace(visitor, header->Payload());
           visitor->AccountMarkedBytes(header);
         },
-        visitor->task_id());
+        should_yield_callback, visitor->task_id());
     if (!finished)
       break;
 
@@ -587,13 +578,13 @@ bool ThreadHeap::AdvanceConcurrentMarking(ConcurrentMarkingVisitor* visitor,
       // by the mutator thread and then invoked either concurrently or by the
       // mutator thread (in the atomic pause at latest).
       finished =
-          DrainWorklistWithYielding<kDefaultConcurrentDeadlineCheckInterval>(
-              delegate, ephemeron_pairs_to_process_worklist_.get(),
+          DrainWorklist<kDefaultConcurrentDeadlineCheckInterval>(
+              ephemeron_pairs_to_process_worklist_.get(),
               [visitor](EphemeronPairItem& item) {
                 visitor->VisitEphemeron(item.key, item.value,
                                         item.value_trace_callback);
               },
-              visitor->task_id());
+              should_yield_callback, visitor->task_id());
       if (!finished)
         break;
     }

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

@@ -62,6 +62,7 @@ class PageBloomFilter;
 class PagePool;
 class ProcessHeapReporter;
 class RegionTree;
+class MarkingSchedulingOracle;
 
 using MarkingItem = TraceDescriptor;
 using NotFullyConstructedItem = const void*;
@@ -307,7 +308,7 @@ class PLATFORM_EXPORT ThreadHeap {
   // Returns true if marker is done
   bool AdvanceConcurrentMarking(ConcurrentMarkingVisitor*,
                                 base::JobDelegate*,
-                                base::TimeTicks);
+                                MarkingSchedulingOracle* marking_scheduler);
 
   // Conservatively checks whether an address is a pointer in any of the
   // thread heaps.  If so marks the object pointed to as live.

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

@@ -32,13 +32,12 @@ void MarkingSchedulingOracle::UpdateIncrementalMarkingStats(
 }
 
 void MarkingSchedulingOracle::AddConcurrentlyMarkedBytes(size_t marked_bytes) {
-  base::AutoLock lock(concurrently_marked_bytes_lock_);
-  concurrently_marked_bytes_ += marked_bytes;
+  concurrently_marked_bytes_.fetch_add(marked_bytes, std::memory_order_relaxed);
 }
 
 size_t MarkingSchedulingOracle::GetOverallMarkedBytes() {
-  base::AutoLock lock(concurrently_marked_bytes_lock_);
-  return incrementally_marked_bytes_ + concurrently_marked_bytes_;
+  return incrementally_marked_bytes_ +
+         concurrently_marked_bytes_.load(std::memory_order_relaxed);
 }
 
 double MarkingSchedulingOracle::GetElapsedTimeInMs(base::TimeTicks start_time) {

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

@@ -5,7 +5,8 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_MARKING_SCHEDULING_ORACLE_H_
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_MARKING_SCHEDULING_ORACLE_H_
 
-#include "base/synchronization/lock.h"
+#include <atomic>
+
 #include "base/time/time.h"
 #include "third_party/blink/renderer/platform/heap/blink_gc.h"
 
@@ -51,8 +52,7 @@ class PLATFORM_EXPORT MarkingSchedulingOracle {
   base::TimeDelta incremental_marking_time_so_far_;
 
   size_t incrementally_marked_bytes_ = 0;
-  size_t concurrently_marked_bytes_ = 0;
-  base::Lock concurrently_marked_bytes_lock_;
+  std::atomic_size_t concurrently_marked_bytes_{0};
 
   // Using -1 as sentinel to denote
   static constexpr double kNoSetElapsedTimeForTesting = -1;

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

@@ -258,10 +258,15 @@ class PLATFORM_EXPORT ConcurrentMarkingVisitor : public MarkingVisitorBase {
     return true;
   }
 
+  size_t RecentlyMarkedBytes() {
+    return marked_bytes_ - std::exchange(last_marked_bytes_, marked_bytes_);
+  }
+
  private:
   NotSafeToConcurrentlyTraceWorklist::View
       not_safe_to_concurrently_trace_worklist_;
   NotFullyConstructedWorklist::View previously_not_fully_constructed_worklist_;
+  size_t last_marked_bytes_ = 0;
 };
 
 }  // namespace blink

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

@@ -99,14 +99,6 @@ uint8_t ThreadState::main_thread_state_storage_[sizeof(ThreadState)];
 
 namespace {
 
-// Concurrent marking should stop every once in a while to flush private
-// segments to v8 marking worklist. It should also stop to avoid priority
-// inversion.
-//
-// TODO(omerkatz): What is a good value to set here?
-constexpr base::TimeDelta kConcurrentMarkingStepDuration =
-    base::TimeDelta::FromMilliseconds(2);
-
 constexpr size_t kMaxTerminationGCLoops = 20;
 
 // Helper function to convert a byte count to a KB count, capping at
@@ -1722,12 +1714,8 @@ void ThreadState::PerformConcurrentMark(base::JobDelegate* job) {
                 this, GetMarkingMode(Heap().Compaction()->IsCompacting()),
                 task_id);
 
-  Heap().AdvanceConcurrentMarking(
-      concurrent_visitor.get(), job,
-      base::TimeTicks::Now() + kConcurrentMarkingStepDuration);
-
-  marking_scheduling_->AddConcurrentlyMarkedBytes(
-      concurrent_visitor->marked_bytes());
+  Heap().AdvanceConcurrentMarking(concurrent_visitor.get(), job,
+                                  marking_scheduling_.get());
 
   concurrent_visitor->FlushWorklists();
 }