From 224cbd21795445fddbd59a315979f8812e1ae871 Mon Sep 17 00:00:00 2001 From: Mikhail Khokhlov Date: Thu, 1 Dec 2022 10:22:28 +0000 Subject: [PATCH] [Tracing w/SDK] Replace TraceStateObserver with TrackEventSessionObserver With v8_use_perfetto = true, tracing sessions are controlled by Perfetto, not TraceLog. This can lead to inconsistencies, e.g. TraceLog signalling trace start while V8's TrackEvent datasource hasn't been initialized yet. This CL removes the TraceStateObserver interface and replaces its uses with perfetto::TrackEventSessionObserver which correctly tracks Perfetto tracing sessions start and end. See also crrev.com/c/4066184 for the corresponding Chrome change. Bug: chromium:1006766 Change-Id: I94b2189c8b28aec8b17ec8fc1246e27c904e4ee9 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4062038 Reviewed-by: Camillo Bruni Commit-Queue: Mikhail Khokhlov Cr-Commit-Position: refs/heads/main@{#84590} --- include/libplatform/v8-tracing.h | 4 ++-- include/v8-platform.h | 12 +++++++++-- src/libplatform/tracing/tracing-controller.cc | 11 +++++----- src/profiler/tracing-cpu-profiler.cc | 19 ++++++++++++++++++ src/profiler/tracing-cpu-profiler.h | 11 ++++++++++ src/tracing/tracing-category-observer.cc | 20 +++++++++++++++++++ src/tracing/tracing-category-observer.h | 14 ++++++++++++- .../unittests/libplatform/tracing-unittest.cc | 10 +++++----- 8 files changed, 86 insertions(+), 15 deletions(-) diff --git a/include/libplatform/v8-tracing.h b/include/libplatform/v8-tracing.h index 12489327c5..6039a9c520 100644 --- a/include/libplatform/v8-tracing.h +++ b/include/libplatform/v8-tracing.h @@ -282,12 +282,12 @@ class V8_PLATFORM_EXPORT TracingController const char* name, uint64_t handle) override; static const char* GetCategoryGroupName(const uint8_t* category_enabled_flag); -#endif // !defined(V8_USE_PERFETTO) void AddTraceStateObserver( v8::TracingController::TraceStateObserver* observer) override; void RemoveTraceStateObserver( v8::TracingController::TraceStateObserver* observer) override; +#endif // !defined(V8_USE_PERFETTO) void StartTracing(TraceConfig* trace_config); void StopTracing(); @@ -307,7 +307,6 @@ class V8_PLATFORM_EXPORT TracingController std::unique_ptr mutex_; std::unique_ptr trace_config_; std::atomic_bool recording_{false}; - std::unordered_set observers_; #if defined(V8_USE_PERFETTO) std::ostream* output_stream_ = nullptr; @@ -316,6 +315,7 @@ class V8_PLATFORM_EXPORT TracingController TraceEventListener* listener_for_testing_ = nullptr; std::unique_ptr tracing_session_; #else // !defined(V8_USE_PERFETTO) + std::unordered_set observers_; std::unique_ptr trace_buffer_; #endif // !defined(V8_USE_PERFETTO) diff --git a/include/v8-platform.h b/include/v8-platform.h index 32a82f881e..32898e7ef9 100644 --- a/include/v8-platform.h +++ b/include/v8-platform.h @@ -285,6 +285,8 @@ class ConvertableToTraceFormat { * V8 Tracing controller. * * Can be implemented by an embedder to record trace events from V8. + * + * Will become obsolete in Perfetto SDK build (v8_use_perfetto = true). */ class TracingController { public: @@ -348,10 +350,16 @@ class TracingController { virtual void OnTraceDisabled() = 0; }; - /** Adds tracing state change observer. */ + /** + * Adds tracing state change observer. + * Does nothing in Perfetto SDK build (v8_use_perfetto = true). + */ virtual void AddTraceStateObserver(TraceStateObserver*) {} - /** Removes tracing state change observer. */ + /** + * Removes tracing state change observer. + * Does nothing in Perfetto SDK build (v8_use_perfetto = true). + */ virtual void RemoveTraceStateObserver(TraceStateObserver*) {} }; diff --git a/src/libplatform/tracing/tracing-controller.cc b/src/libplatform/tracing/tracing-controller.cc index 55ca063184..0d235e8566 100644 --- a/src/libplatform/tracing/tracing-controller.cc +++ b/src/libplatform/tracing/tracing-controller.cc @@ -206,18 +206,19 @@ void TracingController::StartTracing(TraceConfig* trace_config) { #endif // V8_USE_PERFETTO trace_config_.reset(trace_config); + recording_.store(true, std::memory_order_release); + +#ifndef V8_USE_PERFETTO std::unordered_set observers_copy; { base::MutexGuard lock(mutex_.get()); - recording_.store(true, std::memory_order_release); -#ifndef V8_USE_PERFETTO UpdateCategoryGroupEnabledFlags(); -#endif observers_copy = observers_; } for (auto o : observers_copy) { o->OnTraceEnabled(); } +#endif } void TracingController::StopTracing() { @@ -227,7 +228,6 @@ void TracingController::StopTracing() { } #ifndef V8_USE_PERFETTO UpdateCategoryGroupEnabledFlags(); -#endif std::unordered_set observers_copy; { base::MutexGuard lock(mutex_.get()); @@ -236,6 +236,7 @@ void TracingController::StopTracing() { for (auto o : observers_copy) { o->OnTraceDisabled(); } +#endif #ifdef V8_USE_PERFETTO tracing_session_->StopBlocking(); @@ -341,7 +342,6 @@ const uint8_t* TracingController::GetCategoryGroupEnabled( } return category_group_enabled; } -#endif // !defined(V8_USE_PERFETTO) void TracingController::AddTraceStateObserver( v8::TracingController::TraceStateObserver* observer) { @@ -360,6 +360,7 @@ void TracingController::RemoveTraceStateObserver( DCHECK(observers_.find(observer) != observers_.end()); observers_.erase(observer); } +#endif // !defined(V8_USE_PERFETTO) } // namespace tracing } // namespace platform diff --git a/src/profiler/tracing-cpu-profiler.cc b/src/profiler/tracing-cpu-profiler.cc index d18ae09fb1..6d341282e4 100644 --- a/src/profiler/tracing-cpu-profiler.cc +++ b/src/profiler/tracing-cpu-profiler.cc @@ -14,16 +14,31 @@ namespace internal { TracingCpuProfilerImpl::TracingCpuProfilerImpl(Isolate* isolate) : isolate_(isolate), profiling_enabled_(false) { +#if defined(V8_USE_PERFETTO) + TrackEvent::AddSessionObserver(this); + // Fire the observer if tracing is already in progress. + if (TrackEvent::IsEnabled()) OnStart({}); +#else V8::GetCurrentPlatform()->GetTracingController()->AddTraceStateObserver(this); +#endif } TracingCpuProfilerImpl::~TracingCpuProfilerImpl() { StopProfiling(); +#if defined(V8_USE_PERFETTO) + TrackEvent::RemoveSessionObserver(this); +#else V8::GetCurrentPlatform()->GetTracingController()->RemoveTraceStateObserver( this); +#endif } +#if defined(V8_USE_PERFETTO) +void TracingCpuProfilerImpl::OnStart( + const perfetto::DataSourceBase::StartArgs&) { +#else void TracingCpuProfilerImpl::OnTraceEnabled() { +#endif bool enabled; TRACE_EVENT_CATEGORY_GROUP_ENABLED( TRACE_DISABLED_BY_DEFAULT("v8.cpu_profiler"), &enabled); @@ -36,7 +51,11 @@ void TracingCpuProfilerImpl::OnTraceEnabled() { this); } +#if defined(V8_USE_PERFETTO) +void TracingCpuProfilerImpl::OnStop(const perfetto::DataSourceBase::StopArgs&) { +#else void TracingCpuProfilerImpl::OnTraceDisabled() { +#endif base::MutexGuard lock(&mutex_); if (!profiling_enabled_) return; profiling_enabled_ = false; diff --git a/src/profiler/tracing-cpu-profiler.h b/src/profiler/tracing-cpu-profiler.h index 3adcaa63d7..0ccf7f0e5d 100644 --- a/src/profiler/tracing-cpu-profiler.h +++ b/src/profiler/tracing-cpu-profiler.h @@ -11,6 +11,7 @@ #include "src/base/atomic-utils.h" #include "src/base/macros.h" #include "src/base/platform/mutex.h" +#include "src/tracing/trace-event.h" namespace v8 { namespace internal { @@ -19,16 +20,26 @@ class CpuProfiler; class Isolate; class TracingCpuProfilerImpl final +#if defined(V8_USE_PERFETTO) + : public perfetto::TrackEventSessionObserver { +#else : private v8::TracingController::TraceStateObserver { +#endif public: explicit TracingCpuProfilerImpl(Isolate*); ~TracingCpuProfilerImpl() override; TracingCpuProfilerImpl(const TracingCpuProfilerImpl&) = delete; TracingCpuProfilerImpl& operator=(const TracingCpuProfilerImpl&) = delete; +#if defined(V8_USE_PERFETTO) + // perfetto::TrackEventSessionObserver + void OnStart(const perfetto::DataSourceBase::StartArgs&) override; + void OnStop(const perfetto::DataSourceBase::StopArgs&) override; +#else // v8::TracingController::TraceStateObserver void OnTraceEnabled() final; void OnTraceDisabled() final; +#endif private: void StartProfiling(); diff --git a/src/tracing/tracing-category-observer.cc b/src/tracing/tracing-category-observer.cc index 3debacb548..feee3807c6 100644 --- a/src/tracing/tracing-category-observer.cc +++ b/src/tracing/tracing-category-observer.cc @@ -16,17 +16,32 @@ TracingCategoryObserver* TracingCategoryObserver::instance_ = nullptr; void TracingCategoryObserver::SetUp() { TracingCategoryObserver::instance_ = new TracingCategoryObserver(); +#if defined(V8_USE_PERFETTO) + TrackEvent::AddSessionObserver(instance_); + // Fire the observer if tracing is already in progress. + if (TrackEvent::IsEnabled()) instance_->OnStart({}); +#else i::V8::GetCurrentPlatform()->GetTracingController()->AddTraceStateObserver( TracingCategoryObserver::instance_); +#endif } void TracingCategoryObserver::TearDown() { +#if defined(V8_USE_PERFETTO) + TrackEvent::RemoveSessionObserver(TracingCategoryObserver::instance_); +#else i::V8::GetCurrentPlatform()->GetTracingController()->RemoveTraceStateObserver( TracingCategoryObserver::instance_); +#endif delete TracingCategoryObserver::instance_; } +#if defined(V8_USE_PERFETTO) +void TracingCategoryObserver::OnStart( + const perfetto::DataSourceBase::StartArgs&) { +#else void TracingCategoryObserver::OnTraceEnabled() { +#endif bool enabled = false; TRACE_EVENT_CATEGORY_GROUP_ENABLED( TRACE_DISABLED_BY_DEFAULT("v8.runtime_stats"), &enabled); @@ -66,7 +81,12 @@ void TracingCategoryObserver::OnTraceEnabled() { } } +#if defined(V8_USE_PERFETTO) +void TracingCategoryObserver::OnStop( + const perfetto::DataSourceBase::StopArgs&) { +#else void TracingCategoryObserver::OnTraceDisabled() { +#endif i::TracingFlags::runtime_stats.fetch_and( ~(ENABLED_BY_TRACING | ENABLED_BY_SAMPLING), std::memory_order_relaxed); diff --git a/src/tracing/tracing-category-observer.h b/src/tracing/tracing-category-observer.h index 858bf0bdf8..bf9687b5c2 100644 --- a/src/tracing/tracing-category-observer.h +++ b/src/tracing/tracing-category-observer.h @@ -6,11 +6,17 @@ #define V8_TRACING_TRACING_CATEGORY_OBSERVER_H_ #include "include/v8-platform.h" +#include "src/tracing/trace-event.h" namespace v8 { namespace tracing { -class TracingCategoryObserver : public TracingController::TraceStateObserver { +class TracingCategoryObserver +#if defined(V8_USE_PERFETTO) + : public perfetto::TrackEventSessionObserver { +#else + : public TracingController::TraceStateObserver { +#endif public: enum Mode { ENABLED_BY_NATIVE = 1 << 0, @@ -21,9 +27,15 @@ class TracingCategoryObserver : public TracingController::TraceStateObserver { static void SetUp(); static void TearDown(); +#if defined(V8_USE_PERFETTO) + // perfetto::TrackEventSessionObserver + void OnStart(const perfetto::DataSourceBase::StartArgs&) override; + void OnStop(const perfetto::DataSourceBase::StopArgs&) override; +#else // v8::TracingController::TraceStateObserver void OnTraceEnabled() final; void OnTraceDisabled() final; +#endif private: static TracingCategoryObserver* instance_; diff --git a/test/unittests/libplatform/tracing-unittest.cc b/test/unittests/libplatform/tracing-unittest.cc index 76f78af14c..d6cf31cb2a 100644 --- a/test/unittests/libplatform/tracing-unittest.cc +++ b/test/unittests/libplatform/tracing-unittest.cc @@ -389,6 +389,10 @@ TEST_F(PlatformTracingTest, TestTracingControllerMultipleArgsAndCopy) { } #endif // !defined(V8_USE_PERFETTO) +// In Perfetto build there are no TracingObservers. Instead the code relies on +// TrackEventSessionObserver to track tracing sessions, which is tested +// upstream. +#if !defined(V8_USE_PERFETTO) namespace { class TraceStateObserverImpl : public TracingController::TraceStateObserver { @@ -412,16 +416,11 @@ TEST_F(PlatformTracingTest, TracingObservers) { v8::platform::tracing::TracingController* tracing_controller = tracing.get(); static_cast(default_platform.get()) ->SetTracingController(std::move(tracing)); -#ifdef V8_USE_PERFETTO - std::ostringstream sstream; - tracing_controller->InitializeForPerfetto(&sstream); -#else MockTraceWriter* writer = new MockTraceWriter(); v8::platform::tracing::TraceBuffer* ring_buffer = v8::platform::tracing::TraceBuffer::CreateTraceBufferRingBuffer(1, writer); tracing_controller->Initialize(ring_buffer); -#endif v8::platform::tracing::TraceConfig* trace_config = new v8::platform::tracing::TraceConfig(); trace_config->AddIncludedCategory("v8"); @@ -469,6 +468,7 @@ TEST_F(PlatformTracingTest, TracingObservers) { i::V8::SetPlatformForTesting(old_platform); } +#endif // !defined(V8_USE_PERFETTO) // With Perfetto the tracing controller doesn't observe events. #if !defined(V8_USE_PERFETTO)