From c4eae87a1a475bb2821725642c522f941b9472c9 Mon Sep 17 00:00:00 2001 From: Michael Lippautz Date: Wed, 20 Mar 2019 20:01:07 +0100 Subject: [PATCH] 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/+/1532325 Reviewed-by: Hannes Payer Reviewed-by: Jaroslav Sevcik Commit-Queue: Michael Lippautz Cr-Commit-Position: refs/heads/master@{#60385} --- src/heap/concurrent-marking.cc | 20 ++++++++++++++++---- src/heap/incremental-marking.cc | 8 +++----- src/heap/incremental-marking.h | 5 ----- src/heap/mark-compact-inl.h | 28 ++++++++++++++++++++-------- src/heap/spaces.h | 15 ++++++++------- test/cctest/heap/test-heap.cc | 4 ++-- 6 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/heap/concurrent-marking.cc b/src/heap/concurrent-marking.cc index 5262fe3503..6b1c106b75 100644 --- a/src/heap/concurrent-marking.cc +++ b/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(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); diff --git a/src/heap/incremental-marking.cc b/src/heap/incremental-marking.cc index dfb53beed3..3332eff38d 100644 --- a/src/heap/incremental-marking.cc +++ b/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; } diff --git a/src/heap/incremental-marking.h b/src/heap/incremental-marking.h index 5b07feb13e..f3f0703bd1 100644 --- a/src/heap/incremental-marking.h +++ b/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_; diff --git a/src/heap/mark-compact-inl.h b/src/heap/mark-compact-inl.h index 90a3b1db5e..f36a4d2636 100644 --- a/src/heap/mark-compact-inl.h +++ b/src/heap/mark-compact-inl.h @@ -397,24 +397,36 @@ int MarkingVisitor:: 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(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; } diff --git a/src/heap/spaces.h b/src/heap/spaces.h index d8590cb9d7..1781948074 100644 --- a/src/heap/spaces.h +++ b/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(HAS_PROGRESS_BAR)); - return static_cast(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(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 progress_bar_; + std::atomic progress_bar_; // Count of bytes marked black on page. intptr_t live_byte_count_; diff --git a/test/cctest/heap/test-heap.cc b/test/cctest/heap/test-heap.cc index fa3d5fe163..4eb7dfb2cb 100644 --- a/test/cctest/heap/test-heap.cc +++ b/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.