heap: Fix incremental-concurrent processing of large FixedArray

FixedArray object in LO space are processed incrementally in ranges of slots
size kProgressBarScanningChunk to reduce latency when returning to the
processing loop is critical. A progress bar stores how much slots have been
processed already.

In the case of regular concurrent marking there was a guarantee that the
object was only processed by one thread (main *or* concurrent marking
thread) at the same time.

However, some optimizations that avoid write barriers for each
individual write operation emit a batched write barrier that requires
re-visiting the FixedArray for the marking barrier. In such cases, the
progress bar would be reset using relaxed stores which is problematic as
the concurrent marking thread could race on setting its own progress on the
progress bar. As a result, the array would only be re-scanned partially.

The fix involves using CAS to set the progress bar and bail out in the
case an inconsistent state was observed.

In the following:
MT... main thread
CM... concurrent marking thread

The interesting cases are:
1. MT *or* CM processes the array without interfering: Progress bar is
   updated monotonically without failing.
3. MT interferes with itself: The progress bar is just reset and the main
   thread will restart scanning from index 0. The object is added twice to
   the marking worklist and processed each time one of the entries is
   retrieved from the worklist.
4. MT interferes with CM:
   4.a.: CM processes a range of slots and re-adds the left overs by
   setting the progress bar and re-adding the array to the worklist.  In
   this case CM *and* MT process the array from index 0. The first time
   the CAS for setting the progress bar fails on either of the threads,
   the looser will bail out and leave processing for the winner.
   4.b.: CM is interrupted while processing a range of the array and
   fails in setting the progress bar for the left overs. In this case
   the CM bails out right away and the main thread starts processing
   from index 0.

In addition, there is a transition from index 0 to the index of the
first actual slot. This transition makes it possible to observe a reset
while processing the first actual chunk of slots.

Bug: chromium:942699
Change-Id: I0b06f47ee075030dadfc959528cd77b6b69bbec2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1532325Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60385}

src/heap/concurrent-marking.cc

@@ -325,13 +325,25 @@ class ConcurrentMarkingVisitor final
     DCHECK(marking_state_.IsBlackOrGrey(object));
     marking_state_.GreyToBlack(object);
     int size = FixedArray::BodyDescriptor::SizeOf(map, object);
-    int start =
-        Max(FixedArray::BodyDescriptor::kStartOffset, chunk->progress_bar());
+    size_t current_progress_bar = chunk->ProgressBar();
+    if (current_progress_bar == 0) {
+      // Try to move the progress bar forward to start offset. This solves the
+      // problem of not being able to observe a progress bar reset when
+      // processing the first kProgressBarScanningChunk.
+      if (!chunk->TrySetProgressBar(0,
+                                    FixedArray::BodyDescriptor::kStartOffset))
+        return 0;
+      current_progress_bar = FixedArray::BodyDescriptor::kStartOffset;
+    }
+    int start = static_cast<int>(current_progress_bar);
     int end = Min(size, start + kProgressBarScanningChunk);
     if (start < end) {
       VisitPointers(object, object.RawField(start), object.RawField(end));
-      chunk->set_progress_bar(end);
-      if (end < size) {
+      // Setting the progress bar can fail if the object that is currently
+      // scanned is also revisited. In this case, there may be two tasks racing
+      // on the progress counter. The looser can bail out because the progress
+      // bar is reset before the tasks race on the object.
+      if (chunk->TrySetProgressBar(current_progress_bar, end) && (end < size)) {
         // The object can be pushed back onto the marking worklist only after
         // progress bar was updated.
         shared_.Push(object);

src/heap/incremental-marking.cc

@@ -58,7 +58,6 @@ IncrementalMarking::IncrementalMarking(
       scheduled_bytes_to_mark_(0),
       schedule_update_time_ms_(0),
       bytes_marked_concurrently_(0),
-      unscanned_bytes_of_large_object_(0),
       is_compacting_(false),
       should_hurry_(false),
       was_activated_(false),
@@ -736,7 +735,8 @@ void IncrementalMarking::RevisitObject(HeapObject obj) {
   DCHECK(IsMarking());
   DCHECK(marking_state()->IsBlack(obj));
   Page* page = Page::FromHeapObject(obj);
-  if (page->owner()->identity() == LO_SPACE) {
+  if (page->owner()->identity() == LO_SPACE ||
+      page->owner()->identity() == NEW_LO_SPACE) {
     page->ResetProgressBar();
   }
   Map map = obj->map();
@@ -770,9 +770,7 @@ intptr_t IncrementalMarking::ProcessMarkingWorklist(
       DCHECK(!marking_state()->IsImpossible(obj));
       continue;
     }
-    unscanned_bytes_of_large_object_ = 0;
-    int size = VisitObject(obj->map(), obj);
-    bytes_processed += size - unscanned_bytes_of_large_object_;
+    bytes_processed += VisitObject(obj->map(), obj);
   }
   return bytes_processed;
 }

src/heap/incremental-marking.h

@@ -222,10 +222,6 @@ class V8_EXPORT_PRIVATE IncrementalMarking {
 
   bool IsCompacting() { return IsMarking() && is_compacting_; }
 
-  void NotifyIncompleteScanOfObject(int unscanned_bytes) {
-    unscanned_bytes_of_large_object_ = unscanned_bytes;
-  }
-
   void ProcessBlackAllocatedObject(HeapObject obj);
 
   Heap* heap() const { return heap_; }
@@ -338,7 +334,6 @@ class V8_EXPORT_PRIVATE IncrementalMarking {
   // incremental marking step. It is used for updating
   // bytes_marked_ahead_of_schedule_ with contribution of concurrent marking.
   size_t bytes_marked_concurrently_;
-  size_t unscanned_bytes_of_large_object_;
 
   // Must use SetState() above to update state_
   State state_;

src/heap/mark-compact-inl.h

@@ -397,24 +397,36 @@ int MarkingVisitor<fixed_array_mode, retaining_path_mode, MarkingState>::
   if (chunk->IsFlagSet(MemoryChunk::HAS_PROGRESS_BAR)) {
     DCHECK(FLAG_use_marking_progress_bar);
     DCHECK(heap_->IsLargeObject(object));
-    int start =
-        Max(FixedArray::BodyDescriptor::kStartOffset, chunk->progress_bar());
+    size_t current_progress_bar = chunk->ProgressBar();
+    if (current_progress_bar == 0) {
+      // Try to move the progress bar forward to start offset. This solves the
+      // problem of not being able to observe a progress bar reset when
+      // processing the first kProgressBarScanningChunk.
+      if (!chunk->TrySetProgressBar(0,
+                                    FixedArray::BodyDescriptor::kStartOffset))
+        return 0;
+      current_progress_bar = FixedArray::BodyDescriptor::kStartOffset;
+    }
+    int start = static_cast<int>(current_progress_bar);
     int end = Min(size, start + kProgressBarScanningChunk);
     if (start < end) {
       VisitPointers(object, object.RawField(start), object.RawField(end));
-      chunk->set_progress_bar(end);
-      if (end < size) {
+      // Setting the progress bar can fail if the object that is currently
+      // scanned is also revisited. In this case, there may be two tasks racing
+      // on the progress counter. The looser can bail out because the progress
+      // bar is reset before the tasks race on the object.
+      if (chunk->TrySetProgressBar(current_progress_bar, end) && (end < size)) {
         DCHECK(marking_state()->IsBlack(object));
         // The object can be pushed back onto the marking worklist only after
         // progress bar was updated.
         marking_worklist()->Push(object);
-        heap_->incremental_marking()->NotifyIncompleteScanOfObject(
-            size - (end - start));
       }
     }
-  } else {
-    FixedArray::BodyDescriptor::IterateBody(map, object, size, this);
+    return end - start;
   }
+
+  // Non-batched processing.
+  FixedArray::BodyDescriptor::IterateBody(map, object, size, this);
   return size;
 }
 

src/heap/spaces.h

@@ -379,7 +379,7 @@ class MemoryChunk {
       + kSystemPointerSize      // Address area_start_
       + kSystemPointerSize      // Address area_end_
       + kSystemPointerSize      // Address owner_
-      + kIntptrSize             // intptr_t progress_bar_
+      + kSizetSize              // size_t progress_bar_
       + kIntptrSize             // intptr_t live_byte_count_
       + kSystemPointerSize * NUMBER_OF_REMEMBERED_SET_TYPES  // SlotSet* array
       + kSystemPointerSize *
@@ -541,19 +541,20 @@ class MemoryChunk {
 
   Address HighWaterMark() { return address() + high_water_mark_; }
 
-  int progress_bar() {
+  size_t ProgressBar() {
     DCHECK(IsFlagSet<AccessMode::ATOMIC>(HAS_PROGRESS_BAR));
-    return static_cast<int>(progress_bar_.load(std::memory_order_relaxed));
+    return progress_bar_.load(std::memory_order_acquire);
   }
 
-  void set_progress_bar(int progress_bar) {
+  bool TrySetProgressBar(size_t old_value, size_t new_value) {
     DCHECK(IsFlagSet<AccessMode::ATOMIC>(HAS_PROGRESS_BAR));
-    progress_bar_.store(progress_bar, std::memory_order_relaxed);
+    return progress_bar_.compare_exchange_strong(old_value, new_value,
+                                                 std::memory_order_acq_rel);
   }
 
   void ResetProgressBar() {
     if (IsFlagSet(MemoryChunk::HAS_PROGRESS_BAR)) {
-      set_progress_bar(0);
+      progress_bar_.store(0, std::memory_order_release);
     }
   }
 
@@ -724,7 +725,7 @@ class MemoryChunk {
 
   // Used by the incremental marker to keep track of the scanning progress in
   // large objects that have a progress bar and are scanned in increments.
-  std::atomic<intptr_t> progress_bar_;
+  std::atomic<size_t> progress_bar_;
 
   // Count of bytes marked black on page.
   intptr_t live_byte_count_;

test/cctest/heap/test-heap.cc

@@ -5563,8 +5563,8 @@ TEST(Regress598319) {
     marking->V8Step(kSmallStepSizeInMs,
                     i::IncrementalMarking::NO_GC_VIA_STACK_GUARD,
                     StepOrigin::kV8);
-    if (page->IsFlagSet(Page::HAS_PROGRESS_BAR) && page->progress_bar() > 0) {
-      CHECK_NE(page->progress_bar(), arr.get()->Size());
+    if (page->IsFlagSet(Page::HAS_PROGRESS_BAR) && page->ProgressBar() > 0) {
+      CHECK_NE(page->ProgressBar(), arr.get()->Size());
       {
         // Shift by 1, effectively moving one white object across the progress
         // bar, meaning that we will miss marking it.