From 051f5493bd8a65697960c5d6c13fc1bf6aee7869 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Tue, 1 Dec 2020 11:13:35 +0000 Subject: [PATCH] Revert "[heap] Add epoch to GC tracing events" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit be52501d5214e25567e28d92d940e4e15011f345. Reason for revert: Multiple TSan issues: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN/34457/overview (and others) Original change's description: > [heap] Add epoch to GC tracing events > > This CL adds the TRACE_GC_EPOCH macro, which adds the epoch as attribute > to the trace event. Use TRACE_GC_EPOCH for top-level events, nested > events can get the information from its parent. > > V8's GC needs an epoch for young and full collections, since scavenges > also occur during incremental marking. The epoch is also process-wide, > so different isolates do not reuse the same id. > > Change-Id: I8889bccce51e008374b4796445a50062bd87a45d > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2565247 > Commit-Queue: Dominik Inführ > Reviewed-by: Ulan Degenbaev > Cr-Commit-Position: refs/heads/master@{#71521} TBR=ulan@chromium.org,dinfuehr@chromium.org Change-Id: I8219595f0751de84cbea7e047ef21aa95da32f07 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2567696 Reviewed-by: Clemens Backes Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/master@{#71523} --- src/heap/array-buffer-sweeper.cc | 2 +- src/heap/concurrent-marking.cc | 4 +-- src/heap/gc-tracer.cc | 21 ---------------- src/heap/gc-tracer.h | 10 -------- src/heap/heap.cc | 43 ++------------------------------ src/heap/heap.h | 12 --------- src/heap/incremental-marking.cc | 11 +++----- src/heap/mark-compact.cc | 12 ++++----- src/heap/memory-allocator.cc | 4 +-- src/heap/scavenger.cc | 6 ++--- src/heap/sweeper.cc | 8 +++--- src/init/heap-symbols.h | 9 ------- 12 files changed, 24 insertions(+), 118 deletions(-) diff --git a/src/heap/array-buffer-sweeper.cc b/src/heap/array-buffer-sweeper.cc index 8af2a60e9c..108a3df6c7 100644 --- a/src/heap/array-buffer-sweeper.cc +++ b/src/heap/array-buffer-sweeper.cc @@ -154,7 +154,7 @@ void ArrayBufferSweeper::RequestSweep(SweepingScope scope) { scope == SweepingScope::kYoung ? GCTracer::Scope::BACKGROUND_YOUNG_ARRAY_BUFFER_SWEEP : GCTracer::Scope::BACKGROUND_FULL_ARRAY_BUFFER_SWEEP; - TRACE_GC_EPOCH(heap_->tracer(), scope_id, ThreadKind::kBackground); + TRACE_GC1(heap_->tracer(), scope_id, ThreadKind::kBackground); base::MutexGuard guard(&sweeping_mutex_); job_->Sweep(); job_finished_.NotifyAll(); diff --git a/src/heap/concurrent-marking.cc b/src/heap/concurrent-marking.cc index 9f2da557d5..dc8d95fea9 100644 --- a/src/heap/concurrent-marking.cc +++ b/src/heap/concurrent-marking.cc @@ -398,8 +398,8 @@ ConcurrentMarking::ConcurrentMarking(Heap* heap, void ConcurrentMarking::Run(JobDelegate* delegate, unsigned mark_compact_epoch, bool is_forced_gc) { - TRACE_GC_EPOCH(heap_->tracer(), GCTracer::Scope::MC_BACKGROUND_MARKING, - ThreadKind::kBackground); + TRACE_GC1(heap_->tracer(), GCTracer::Scope::MC_BACKGROUND_MARKING, + ThreadKind::kBackground); size_t kBytesUntilInterruptCheck = 64 * KB; int kObjectsUntilInterrupCheck = 1000; uint8_t task_id = delegate->GetTaskId() + 1; diff --git a/src/heap/gc-tracer.cc b/src/heap/gc-tracer.cc index 170645cbf8..de1375f109 100644 --- a/src/heap/gc-tracer.cc +++ b/src/heap/gc-tracer.cc @@ -48,14 +48,6 @@ double GCTracer::MonotonicallyIncreasingTimeInMs() { } } -CollectionEpoch GCTracer::CurrentEpoch(Scope::ScopeId scope_id) { - if (Scope::NeedsYoungEpoch(scope_id)) { - return heap_->epoch_young(); - } else { - return heap_->epoch_full(); - } -} - GCTracer::Scope::Scope(GCTracer* tracer, ScopeId scope, ThreadKind thread_kind) : tracer_(tracer), scope_(scope), thread_kind_(thread_kind) { start_time_ = tracer_->MonotonicallyIncreasingTimeInMs(); @@ -103,19 +95,6 @@ const char* GCTracer::Scope::Name(ScopeId id) { return nullptr; } -bool GCTracer::Scope::NeedsYoungEpoch(ScopeId id) { -#define CASE(scope) \ - case Scope::scope: \ - return true; - switch (id) { - TRACER_YOUNG_EPOCH_SCOPES(CASE) - default: - return false; - } -#undef CASE - UNREACHABLE(); -} - GCTracer::Event::Event(Type type, GarbageCollectionReason gc_reason, const char* collector_reason) : type(type), diff --git a/src/heap/gc-tracer.h b/src/heap/gc-tracer.h index 515714b6de..f29cb63199 100644 --- a/src/heap/gc-tracer.h +++ b/src/heap/gc-tracer.h @@ -41,13 +41,6 @@ enum ScavengeSpeedMode { kForAllObjects, kForSurvivedObjects }; GCTracer::Scope gc_tracer_scope(tracer, gc_tracer_scope_id, thread_kind); \ TRACE_EVENT0(TRACE_GC_CATEGORIES, GCTracer::Scope::Name(gc_tracer_scope_id)) -#define TRACE_GC_EPOCH(tracer, scope_id, thread_kind) \ - GCTracer::Scope::ScopeId gc_tracer_scope_id(scope_id); \ - GCTracer::Scope gc_tracer_scope(tracer, gc_tracer_scope_id, thread_kind); \ - CollectionEpoch epoch = tracer->CurrentEpoch(scope_id); \ - TRACE_EVENT1(TRACE_GC_CATEGORIES, GCTracer::Scope::Name(gc_tracer_scope_id), \ - "epoch", epoch) - // GCTracer collects and prints ONE line after each garbage collector // invocation IFF --trace_gc is used. class V8_EXPORT_PRIVATE GCTracer { @@ -101,7 +94,6 @@ class V8_EXPORT_PRIVATE GCTracer { Scope(GCTracer* tracer, ScopeId scope, ThreadKind thread_kind); ~Scope(); static const char* Name(ScopeId id); - static bool NeedsYoungEpoch(ScopeId id); private: GCTracer* tracer_; @@ -342,8 +334,6 @@ class V8_EXPORT_PRIVATE GCTracer { WorkerThreadRuntimeCallStats* worker_thread_runtime_call_stats(); - CollectionEpoch CurrentEpoch(Scope::ScopeId id); - private: FRIEND_TEST(GCTracer, AverageSpeed); FRIEND_TEST(GCTracerTest, AllocationThroughput); diff --git a/src/heap/heap.cc b/src/heap/heap.cc index 98cad22d24..efc0471308 100644 --- a/src/heap/heap.cc +++ b/src/heap/heap.cc @@ -104,14 +104,6 @@ namespace v8 { namespace internal { -namespace { -std::atomic global_epoch{0}; - -CollectionEpoch next_epoch() { - return global_epoch.fetch_add(1, std::memory_order_relaxed) + 1; -} -} // namespace - #ifdef V8_ENABLE_THIRD_PARTY_HEAP Isolate* Heap::GetIsolateFromWritableObject(HeapObject object) { return reinterpret_cast( @@ -1740,8 +1732,6 @@ int Heap::NotifyContextDisposed(bool dependant_context) { void Heap::StartIncrementalMarking(int gc_flags, GarbageCollectionReason gc_reason, GCCallbackFlags gc_callback_flags) { - epoch_full_ = next_epoch(); - DCHECK(incremental_marking()->IsStopped()); SafepointScope safepoint(this); set_current_gc_flags(gc_flags); @@ -1956,43 +1946,23 @@ void Heap::UpdateSurvivalStatistics(int start_new_space_size) { tracer()->AddSurvivalRatio(survival_rate); } -namespace { -GCTracer::Scope::ScopeId CollectorScopeId(GarbageCollector collector) { - switch (collector) { - case MARK_COMPACTOR: - return GCTracer::Scope::ScopeId::MARK_COMPACTOR; - case MINOR_MARK_COMPACTOR: - return GCTracer::Scope::ScopeId::MINOR_MARK_COMPACTOR; - case SCAVENGER: - return GCTracer::Scope::ScopeId::SCAVENGER; - } - UNREACHABLE(); -} -} // namespace - size_t Heap::PerformGarbageCollection( GarbageCollector collector, const v8::GCCallbackFlags gc_callback_flags) { DisallowJavascriptExecution no_js(isolate()); base::Optional optional_safepoint_scope; - UpdateCurrentEpoch(collector); - // Stop time-to-collection timer before safepoint - we do not want to measure // time for safepointing. collection_barrier_->StopTimeToCollectionTimer(); - TRACE_GC_EPOCH(tracer(), CollectorScopeId(collector), ThreadKind::kMain); - if (FLAG_local_heaps) { optional_safepoint_scope.emplace(this); } - #ifdef VERIFY_HEAP if (FLAG_verify_heap) { Verify(); } #endif - tracer()->StartInSafepoint(); GarbageCollectionPrologueInSafepoint(); @@ -2072,14 +2042,6 @@ size_t Heap::PerformGarbageCollection( return freed_global_handles; } -void Heap::UpdateCurrentEpoch(GarbageCollector collector) { - if (IsYoungGenerationCollector(collector)) { - epoch_young_ = next_epoch(); - } else if (incremental_marking()->IsStopped()) { - epoch_full_ = next_epoch(); - } -} - void Heap::RecomputeLimits(GarbageCollector collector) { if (!((collector == MARK_COMPACTOR) || (HasLowYoungGenerationAllocationRate() && @@ -3454,9 +3416,8 @@ void Heap::FinalizeIncrementalMarkingIncrementally( HistogramTimerScope incremental_marking_scope( isolate()->counters()->gc_incremental_marking_finalize()); - TRACE_EVENT1("v8", "V8.GCIncrementalMarkingFinalize", "epoch", epoch_full()); - TRACE_GC_EPOCH(tracer(), GCTracer::Scope::MC_INCREMENTAL_FINALIZE, - ThreadKind::kMain); + TRACE_EVENT0("v8", "V8.GCIncrementalMarkingFinalize"); + TRACE_GC(tracer(), GCTracer::Scope::MC_INCREMENTAL_FINALIZE); SafepointScope safepoint(this); InvokeIncrementalMarkingPrologueCallbacks(); diff --git a/src/heap/heap.h b/src/heap/heap.h index ef644016e8..d17eead4e9 100644 --- a/src/heap/heap.h +++ b/src/heap/heap.h @@ -249,8 +249,6 @@ using EphemeronRememberedSet = std::unordered_map, Object::Hasher>; -using CollectionEpoch = uint32_t; - class Heap { public: // Stores ephemeron entries where the EphemeronHashTable is in old-space, @@ -513,8 +511,6 @@ class Heap { void NotifyOldGenerationExpansion(AllocationSpace space, MemoryChunk* chunk); - void UpdateCurrentEpoch(GarbageCollector collector); - inline Address* NewSpaceAllocationTopAddress(); inline Address* NewSpaceAllocationLimitAddress(); inline Address* OldSpaceAllocationTopAddress(); @@ -1562,9 +1558,6 @@ class Heap { static Isolate* GetIsolateFromWritableObject(HeapObject object); - CollectionEpoch epoch_young() { return epoch_young_; } - CollectionEpoch epoch_full() { return epoch_full_; } - private: using ExternalStringTableUpdaterCallback = String (*)(Heap* heap, FullObjectSlot pointer); @@ -2338,11 +2331,6 @@ class Heap { std::unique_ptr tp_heap_; - // We need two epochs, since there can be scavenges during incremental - // marking. - CollectionEpoch epoch_young_ = 0; - CollectionEpoch epoch_full_ = 0; - // Classes in "heap" can be friends. friend class AlwaysAllocateScope; friend class ArrayBufferCollector; diff --git a/src/heap/incremental-marking.cc b/src/heap/incremental-marking.cc index 68ed03ebc9..87f40a93ca 100644 --- a/src/heap/incremental-marking.cc +++ b/src/heap/incremental-marking.cc @@ -178,10 +178,8 @@ void IncrementalMarking::Start(GarbageCollectionReason gc_reason) { static_cast(gc_reason)); HistogramTimerScope incremental_marking_scope( counters->gc_incremental_marking_start()); - TRACE_EVENT1("v8", "V8.GCIncrementalMarkingStart", "epoch", - heap_->epoch_full()); - TRACE_GC_EPOCH(heap()->tracer(), GCTracer::Scope::MC_INCREMENTAL_START, - ThreadKind::kMain); + TRACE_EVENT0("v8", "V8.GCIncrementalMarkingStart"); + TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_INCREMENTAL_START); heap_->tracer()->NotifyIncrementalMarkingStart(); start_time_ms_ = heap()->MonotonicallyIncreasingTimeInMs(); @@ -784,9 +782,8 @@ StepResult IncrementalMarking::AdvanceWithDeadline( StepOrigin step_origin) { HistogramTimerScope incremental_marking_scope( heap_->isolate()->counters()->gc_incremental_marking()); - TRACE_EVENT1("v8", "V8.GCIncrementalMarking", "epoch", heap_->epoch_full()); - TRACE_GC_EPOCH(heap_->tracer(), GCTracer::Scope::MC_INCREMENTAL, - ThreadKind::kMain); + TRACE_EVENT0("v8", "V8.GCIncrementalMarking"); + TRACE_GC(heap_->tracer(), GCTracer::Scope::MC_INCREMENTAL); DCHECK(!IsStopped()); ScheduleBytesToMarkBasedOnTime(heap()->MonotonicallyIncreasingTimeInMs()); diff --git a/src/heap/mark-compact.cc b/src/heap/mark-compact.cc index 007215f9aa..8c74af8ebe 100644 --- a/src/heap/mark-compact.cc +++ b/src/heap/mark-compact.cc @@ -3119,8 +3119,8 @@ class PageEvacuationJob : public v8::JobTask { TRACE_GC(tracer_, evacuator->GetTracingScope()); ProcessItems(delegate, evacuator); } else { - TRACE_GC_EPOCH(tracer_, evacuator->GetBackgroundTracingScope(), - ThreadKind::kBackground); + TRACE_GC1(tracer_, evacuator->GetBackgroundTracingScope(), + ThreadKind::kBackground); ProcessItems(delegate, evacuator); } } @@ -3493,7 +3493,7 @@ class PointersUpdatingJob : public v8::JobTask { TRACE_GC(tracer_, scope_); UpdatePointers(delegate); } else { - TRACE_GC_EPOCH(tracer_, background_scope_, ThreadKind::kBackground); + TRACE_GC1(tracer_, background_scope_, ThreadKind::kBackground); UpdatePointers(delegate); } } @@ -4866,9 +4866,9 @@ class YoungGenerationMarkingJob : public v8::JobTask { GCTracer::Scope::MINOR_MC_MARK_PARALLEL); ProcessItems(delegate); } else { - TRACE_GC_EPOCH(collector_->heap()->tracer(), - GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING, - ThreadKind::kBackground); + TRACE_GC1(collector_->heap()->tracer(), + GCTracer::Scope::MINOR_MC_BACKGROUND_MARKING, + ThreadKind::kBackground); ProcessItems(delegate); } } diff --git a/src/heap/memory-allocator.cc b/src/heap/memory-allocator.cc index cfaa16af53..dd78d090d8 100644 --- a/src/heap/memory-allocator.cc +++ b/src/heap/memory-allocator.cc @@ -160,8 +160,8 @@ class MemoryAllocator::Unmapper::UnmapFreeMemoryJob : public JobTask { : unmapper_(unmapper), tracer_(isolate->heap()->tracer()) {} void Run(JobDelegate* delegate) override { - TRACE_GC_EPOCH(tracer_, GCTracer::Scope::BACKGROUND_UNMAPPER, - ThreadKind::kBackground); + TRACE_GC1(tracer_, GCTracer::Scope::BACKGROUND_UNMAPPER, + ThreadKind::kBackground); unmapper_->PerformFreeMemoryOnQueuedChunks( delegate); if (FLAG_trace_unmapper) { diff --git a/src/heap/scavenger.cc b/src/heap/scavenger.cc index bf0d284d9f..e35e6cc40c 100644 --- a/src/heap/scavenger.cc +++ b/src/heap/scavenger.cc @@ -182,9 +182,9 @@ void ScavengerCollector::JobTask::Run(JobDelegate* delegate) { GCTracer::Scope::SCAVENGER_SCAVENGE_PARALLEL); ProcessItems(delegate, scavenger); } else { - TRACE_GC_EPOCH(outer_->heap_->tracer(), - GCTracer::Scope::SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL, - ThreadKind::kBackground); + TRACE_GC1(outer_->heap_->tracer(), + GCTracer::Scope::SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL, + ThreadKind::kBackground); ProcessItems(delegate, scavenger); } } diff --git a/src/heap/sweeper.cc b/src/heap/sweeper.cc index 8b7f805383..8b363f9034 100644 --- a/src/heap/sweeper.cc +++ b/src/heap/sweeper.cc @@ -86,8 +86,8 @@ class Sweeper::SweeperJob final : public JobTask { TRACE_GC(tracer_, GCTracer::Scope::MC_SWEEP); RunImpl(delegate); } else { - TRACE_GC_EPOCH(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING, - ThreadKind::kBackground); + TRACE_GC1(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING, + ThreadKind::kBackground); RunImpl(delegate); } } @@ -596,8 +596,8 @@ class Sweeper::IterabilityTask final : public CancelableTask { private: void RunInternal() final { - TRACE_GC_EPOCH(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING, - ThreadKind::kBackground); + TRACE_GC1(tracer_, GCTracer::Scope::MC_BACKGROUND_SWEEPING, + ThreadKind::kBackground); for (Page* page : sweeper_->iterability_list_) { sweeper_->MakeIterable(page); } diff --git a/src/init/heap-symbols.h b/src/init/heap-symbols.h index 8d4b2f363a..92f4fc6d2a 100644 --- a/src/init/heap-symbols.h +++ b/src/init/heap-symbols.h @@ -425,7 +425,6 @@ F(HEAP_EXTERNAL_WEAK_GLOBAL_HANDLES) \ F(HEAP_PROLOGUE) \ F(HEAP_PROLOGUE_SAFEPOINT) \ - F(MARK_COMPACTOR) \ TOP_MC_SCOPES(F) \ F(MC_CLEAR_DEPENDENT_CODE) \ F(MC_CLEAR_FLUSHABLE_BYTECODE) \ @@ -467,7 +466,6 @@ F(MC_SWEEP_CODE) \ F(MC_SWEEP_MAP) \ F(MC_SWEEP_OLD) \ - F(MINOR_MARK_COMPACTOR) \ F(MINOR_MC) \ F(MINOR_MC_CLEAR) \ F(MINOR_MC_CLEAR_STRING_TABLE) \ @@ -493,7 +491,6 @@ F(MINOR_MC_MARKING_DEQUE) \ F(MINOR_MC_RESET_LIVENESS) \ F(MINOR_MC_SWEEPING) \ - F(SCAVENGER) \ F(SCAVENGER_COMPLETE_SWEEP_ARRAY_BUFFERS) \ F(SCAVENGER_FAST_PROMOTE) \ F(SCAVENGER_FREE_REMEMBERED_SET) \ @@ -523,10 +520,4 @@ F(MINOR_MC_BACKGROUND_MARKING) \ F(SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL) -#define TRACER_YOUNG_EPOCH_SCOPES(F) \ - F(BACKGROUND_YOUNG_ARRAY_BUFFER_SWEEP) \ - F(MINOR_MARK_COMPACTOR) \ - F(SCAVENGER) \ - F(SCAVENGER_BACKGROUND_SCAVENGE_PARALLEL) - #endif // V8_INIT_HEAP_SYMBOLS_H_