From 3e17635bca7c90a88440f3fced7cfcf324ce1c19 Mon Sep 17 00:00:00 2001 From: Ryan Macnak Date: Wed, 8 Jun 2022 20:33:02 +0000 Subject: [PATCH] [vm, gc] Refactor incremental marking to not require reserving a visitor in advance. Allow multiple threads to contribute to incremental marking. Also, avoid incremental markers handling large arrays that exceed their budget. TEST=ci Change-Id: Icefe490538ff486063a098077aaa962f0a3f2203 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/247260 Commit-Queue: Ryan Macnak Reviewed-by: Siva Annamalai --- runtime/vm/heap/gc_shared.h | 6 +- runtime/vm/heap/heap.cc | 4 +- runtime/vm/heap/marker.cc | 126 ++++++++++++++++++++++++------------ runtime/vm/heap/marker.h | 12 ++-- runtime/vm/heap/pages.cc | 12 +++- runtime/vm/heap/pages.h | 3 +- 6 files changed, 107 insertions(+), 56 deletions(-) diff --git a/runtime/vm/heap/gc_shared.h b/runtime/vm/heap/gc_shared.h index a3112ffd605..4d6d0762f66 100644 --- a/runtime/vm/heap/gc_shared.h +++ b/runtime/vm/heap/gc_shared.h @@ -53,8 +53,10 @@ class GCLinkedList { } else { ASSERT(to->tail_ != Type::null()); ASSERT(to->tail_->untag()->next_seen_by_gc() == Type::null()); - to->tail_->untag()->next_seen_by_gc_ = head_; - to->tail_ = tail_; + if (head_ != Type::null()) { + to->tail_->untag()->next_seen_by_gc_ = head_; + to->tail_ = tail_; + } } Release(); } diff --git a/runtime/vm/heap/heap.cc b/runtime/vm/heap/heap.cc index 7e6102c05d3..9475b4f28ed 100644 --- a/runtime/vm/heap/heap.cc +++ b/runtime/vm/heap/heap.cc @@ -416,7 +416,9 @@ void Heap::NotifyIdle(int64_t deadline) { } } - old_space_.NotifyIdle(deadline); + if (FLAG_mark_when_idle) { + old_space_.IncrementalMarkWithTimeBudget(deadline); + } if (OS::GetCurrentMonotonicMicros() < deadline) { SemiSpace::ClearCache(); diff --git a/runtime/vm/heap/marker.cc b/runtime/vm/heap/marker.cc index 40fcf1713ee..3629c5f457f 100644 --- a/runtime/vm/heap/marker.cc +++ b/runtime/vm/heap/marker.cc @@ -84,7 +84,7 @@ class MarkingVisitorBase : public ObjectPointerVisitor { } void DrainMarkingStack() { - while (ProcessMarkingStack()) { + while (ProcessMarkingStack(kIntptrMax)) { } } @@ -94,12 +94,19 @@ class MarkingVisitorBase : public ObjectPointerVisitor { // by a conservative estimate of the duration of one batch of work. deadline -= 1500; + // A 512kB budget is choosen to be large enough that we don't waste too much + // time on the overhead of exiting ProcessMarkingStack, querying the clock, + // and re-entering, and small enough that a few batches can fit in the idle + // time between animation frames. This amount of marking takes ~1ms on a + // Pixel phone. + constexpr intptr_t kBudget = 512 * KB; + while ((OS::GetCurrentMonotonicMicros() < deadline) && - ProcessMarkingStack()) { + ProcessMarkingStack(kBudget)) { } } - bool ProcessMarkingStack() { + bool ProcessMarkingStack(intptr_t remaining_budget) { ObjectPtr raw_obj = work_list_.Pop(); if ((raw_obj == nullptr) && ProcessPendingWeakProperties()) { raw_obj = work_list_.Pop(); @@ -109,12 +116,6 @@ class MarkingVisitorBase : public ObjectPointerVisitor { return false; // No more work. } - // A 512kB budget is choosen to be large enough that we don't waste too much - // time on the overhead of exiting this function, querying the clock, and - // re-entering, and small enough that a few batches can fit in the idle time - // between animation frames. This amount of marking takes ~1ms on a Pixel - // phone. - intptr_t remaining_budget = 512 * KB; do { do { // First drain the marking stacks. @@ -131,6 +132,13 @@ class MarkingVisitorBase : public ObjectPointerVisitor { FinalizerEntryPtr raw_weak = static_cast(raw_obj); size = ProcessFinalizerEntry(raw_weak); } else { + if ((class_id == kArrayCid) || (class_id == kImmutableArrayCid)) { + size = raw_obj->untag()->HeapSize(); + if (size > remaining_budget) { + work_list_.Push(raw_obj); + return true; // More to mark. + } + } size = raw_obj->untag()->VisitPointersNonvirtual(this); } marked_bytes_ += size; @@ -347,6 +355,14 @@ class MarkingVisitorBase : public ObjectPointerVisitor { delayed_.Release(); } + void FinalizeIncremental(GCLinkedLists* global_list) { + work_list_.Flush(); + work_list_.Finalize(); + deferred_work_list_.Flush(); + deferred_work_list_.Finalize(); + delayed_.FlushInto(global_list); + } + private: void PushMarked(ObjectPtr raw_obj) { ASSERT(raw_obj->IsHeapObject()); @@ -835,6 +851,8 @@ GCMarker::GCMarker(IsolateGroup* isolate_group, Heap* heap) : isolate_group_(isolate_group), heap_(heap), marking_stack_(), + deferred_marking_stack_(), + global_list_(), visitors_(), marked_bytes_(0), marked_micros_(0) { @@ -862,9 +880,6 @@ void GCMarker::StartConcurrentMark(PageSpace* page_space) { &deferred_marking_stack_); const intptr_t num_tasks = FLAG_marker_tasks; - RELEASE_ASSERT(num_tasks >= 1); - const intptr_t num_concurrent_tasks = - num_tasks - (FLAG_mark_when_idle ? 1 : 0); { // Bulk increase task count before starting any task, instead of @@ -873,9 +888,9 @@ void GCMarker::StartConcurrentMark(PageSpace* page_space) { MonitorLocker ml(page_space->tasks_lock()); ASSERT(page_space->phase() == PageSpace::kDone); page_space->set_phase(PageSpace::kMarking); - page_space->set_tasks(page_space->tasks() + num_concurrent_tasks); + page_space->set_tasks(page_space->tasks() + num_tasks); page_space->set_concurrent_marker_tasks( - page_space->concurrent_marker_tasks() + num_concurrent_tasks); + page_space->concurrent_marker_tasks() + num_tasks); } ResetSlices(); @@ -901,16 +916,10 @@ void GCMarker::StartConcurrentMark(PageSpace* page_space) { THR_Print("Task marked %" Pd " bytes in %" Pd64 " micros.\n", visitor->marked_bytes(), visitor->marked_micros()); } - if (FLAG_mark_when_idle) { - // Not spawning a thread to continue processing with the last visitor. - // This visitor is instead left available for the main thread to - // contribute to marking during idle time. - } else { - // Continue non-root marking concurrently. - bool result = Dart::thread_pool()->Run( - this, isolate_group_, page_space, visitor); - ASSERT(result); - } + // Continue non-root marking concurrently. + bool result = Dart::thread_pool()->Run( + this, isolate_group_, page_space, visitor); + ASSERT(result); } } @@ -923,29 +932,60 @@ void GCMarker::StartConcurrentMark(PageSpace* page_space) { } } -void GCMarker::AssistConcurrentMark() { - if (!FLAG_mark_when_idle) return; +void GCMarker::IncrementalMarkWithUnlimitedBudget(PageSpace* page_space) { + TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), + "IncrementalMarkWithUnlimitedBudget"); - SyncMarkingVisitor* visitor = visitors_[FLAG_marker_tasks - 1]; - ASSERT(visitor != nullptr); - TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), "Mark"); + SyncMarkingVisitor visitor(isolate_group_, page_space, &marking_stack_, + &deferred_marking_stack_); int64_t start = OS::GetCurrentMonotonicMicros(); - visitor->DrainMarkingStack(); + visitor.DrainMarkingStack(); int64_t stop = OS::GetCurrentMonotonicMicros(); - visitor->AddMicros(stop - start); + visitor.AddMicros(stop - start); + { + MonitorLocker ml(page_space->tasks_lock()); + visitor.FinalizeIncremental(&global_list_); + marked_bytes_ += visitor.marked_bytes(); + marked_micros_ += visitor.marked_micros(); + } } -void GCMarker::NotifyIdle(int64_t deadline) { - if (!FLAG_mark_when_idle) return; +void GCMarker::IncrementalMarkWithSizeBudget(PageSpace* page_space, + intptr_t size) { + TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), + "IncrementalMarkWithSizeBudget"); - SyncMarkingVisitor* visitor = visitors_[FLAG_marker_tasks - 1]; - if (visitor == nullptr) return; - - TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), "IncrementalMark"); + SyncMarkingVisitor visitor(isolate_group_, page_space, &marking_stack_, + &deferred_marking_stack_); int64_t start = OS::GetCurrentMonotonicMicros(); - visitor->ProcessMarkingStackUntil(deadline); + visitor.ProcessMarkingStack(size); int64_t stop = OS::GetCurrentMonotonicMicros(); - visitor->AddMicros(stop - start); + visitor.AddMicros(stop - start); + { + MonitorLocker ml(page_space->tasks_lock()); + visitor.FinalizeIncremental(&global_list_); + marked_bytes_ += visitor.marked_bytes(); + marked_micros_ += visitor.marked_micros(); + } +} + +void GCMarker::IncrementalMarkWithTimeBudget(PageSpace* page_space, + int64_t deadline) { + TIMELINE_FUNCTION_GC_DURATION(Thread::Current(), + "IncrementalMarkWithTimeBudget"); + + SyncMarkingVisitor visitor(isolate_group_, page_space, &marking_stack_, + &deferred_marking_stack_); + int64_t start = OS::GetCurrentMonotonicMicros(); + visitor.ProcessMarkingStackUntil(deadline); + int64_t stop = OS::GetCurrentMonotonicMicros(); + visitor.AddMicros(stop - start); + { + MonitorLocker ml(page_space->tasks_lock()); + visitor.FinalizeIncremental(&global_list_); + marked_bytes_ += visitor.marked_bytes(); + marked_micros_ += visitor.marked_micros(); + } } void GCMarker::MarkObjects(PageSpace* page_space) { @@ -986,8 +1026,6 @@ void GCMarker::MarkObjects(PageSpace* page_space) { RelaxedAtomic num_busy = 0; // Phase 1: Iterate over roots and drain marking stack in tasks. - GCLinkedLists global_list; - for (intptr_t i = 0; i < num_tasks; ++i) { SyncMarkingVisitor* visitor = visitors_[i]; // Visitors may or may not have already been created depending on @@ -1003,7 +1041,7 @@ void GCMarker::MarkObjects(PageSpace* page_space) { // visitor might not get to run if it fails to reach TryEnter soon // enough, and we must fail to visit objects but they're sitting in // such a visitor's local blocks. - visitor->Flush(&global_list); + visitor->Flush(&global_list_); // Need to move weak property list too. if (i < (num_tasks - 1)) { @@ -1014,7 +1052,7 @@ void GCMarker::MarkObjects(PageSpace* page_space) { ASSERT(result); } else { // Last worker is the main thread. - visitor->Adopt(&global_list); + visitor->Adopt(&global_list_); ParallelMarkTask task(this, isolate_group_, &marking_stack_, barrier, visitor, &num_busy); task.RunEnteredIsolateGroup(); @@ -1031,6 +1069,8 @@ void GCMarker::MarkObjects(PageSpace* page_space) { delete visitor; visitors_[i] = nullptr; } + + ASSERT(global_list_.IsEmpty()); } } Epilogue(); diff --git a/runtime/vm/heap/marker.h b/runtime/vm/heap/marker.h index ec361708f02..6cc2653d5d4 100644 --- a/runtime/vm/heap/marker.h +++ b/runtime/vm/heap/marker.h @@ -6,6 +6,7 @@ #define RUNTIME_VM_HEAP_MARKER_H_ #include "vm/allocation.h" +#include "vm/heap/gc_shared.h" #include "vm/heap/pointer_block.h" #include "vm/os_thread.h" // Mutex. @@ -38,12 +39,10 @@ class GCMarker { // Marking must later be finalized by calling MarkObjects. void StartConcurrentMark(PageSpace* page_space); - // Called when a synchronous GC is required, but concurrent marking is still - // in progress. - void AssistConcurrentMark(); - - // Perform incremental marking if available. - void NotifyIdle(int64_t deadline); + // Contribute to marking. + void IncrementalMarkWithUnlimitedBudget(PageSpace* page_space); + void IncrementalMarkWithSizeBudget(PageSpace* page_space, intptr_t size); + void IncrementalMarkWithTimeBudget(PageSpace* page_space, int64_t deadline); // (Re)mark roots, drain the marking queue and finalize weak references. // Does not required StartConcurrentMark to have been previously called. @@ -71,6 +70,7 @@ class GCMarker { Heap* const heap_; MarkingStack marking_stack_; MarkingStack deferred_marking_stack_; + GCLinkedLists global_list_; MarkingVisitorBase** visitors_; NewPage* new_page_; diff --git a/runtime/vm/heap/pages.cc b/runtime/vm/heap/pages.cc index de7a382f498..e7f1933ffee 100644 --- a/runtime/vm/heap/pages.cc +++ b/runtime/vm/heap/pages.cc @@ -1034,16 +1034,22 @@ bool PageSpace::ShouldPerformIdleMarkCompact(int64_t deadline) { return estimated_mark_compact_completion <= deadline; } -void PageSpace::NotifyIdle(int64_t deadline) { +void PageSpace::IncrementalMarkWithSizeBudget(intptr_t size) { if (marker_ != nullptr) { - marker_->NotifyIdle(deadline); + marker_->IncrementalMarkWithSizeBudget(this, size); + } +} + +void PageSpace::IncrementalMarkWithTimeBudget(int64_t deadline) { + if (marker_ != nullptr) { + marker_->IncrementalMarkWithTimeBudget(this, deadline); } } void PageSpace::AssistTasks(MonitorLocker* ml) { if (phase() == PageSpace::kMarking) { ml->Exit(); - marker_->AssistConcurrentMark(); + marker_->IncrementalMarkWithUnlimitedBudget(this); ml->Enter(); } if ((phase() == kSweepingLarge) || (phase() == kSweepingRegular)) { diff --git a/runtime/vm/heap/pages.h b/runtime/vm/heap/pages.h index 28ee25e89dc..50f334f8e58 100644 --- a/runtime/vm/heap/pages.h +++ b/runtime/vm/heap/pages.h @@ -434,7 +434,8 @@ class PageSpace { bool ShouldStartIdleMarkSweep(int64_t deadline); bool ShouldPerformIdleMarkCompact(int64_t deadline); - void NotifyIdle(int64_t deadline); + void IncrementalMarkWithSizeBudget(intptr_t size); + void IncrementalMarkWithTimeBudget(int64_t deadline); void AssistTasks(MonitorLocker* ml); void AddGCTime(int64_t micros) { gc_time_micros_ += micros; }