From dc5c00614894571c1c14f55ce1b10a38e5088a9d Mon Sep 17 00:00:00 2001 From: Nicolas Dubus Date: Mon, 15 Feb 2021 20:05:32 -1000 Subject: [PATCH] [Callback] Execute DiscardedSamplesDelegate Callback when samples being discarded - Passed in as CpuProfilingOptions parameter, client is responsible for determining if function is still safe to execute. Includes unit tests - Client (blink) side CR: https://chromium-review.googlesource.com/c/chromium/src/+/2649617, - Client (blink) side CR requires this to be pushed prior to it being pushed (cherry picked from commit eec25f2199e726d7915b15e29ad7718ae5e09711) Change-Id: I3ef4640186115d4e14c1b73f902c889c776e310f Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2652206 Commit-Queue: Nicolas Dubus Reviewed-by: Ulan Degenbaev Reviewed-by: Peter Marshall Cr-Commit-Position: refs/heads/master@{#72794} --- third_party/v8/include/v8-profiler.h | 16 ++- third_party/v8/src/api/api.cc | 7 +- third_party/v8/src/profiler/cpu-profiler.cc | 16 ++- third_party/v8/src/profiler/cpu-profiler.h | 10 +- .../v8/src/profiler/profile-generator.cc | 24 +++- .../v8/src/profiler/profile-generator.h | 23 +++- .../v8/test/cctest/test-profile-generator.cc | 111 ++++++++++++++++++ 7 files changed, 184 insertions(+), 23 deletions(-) diff --git a/third_party/v8/include/v8-profiler.h b/third_party/v8/include/v8-profiler.h index 74b6df884d37..85d3f8a4821a 100644 --- a/third_party/v8/include/v8-profiler.h +++ b/third_party/v8/include/v8-profiler.h @@ -258,6 +258,17 @@ enum class CpuProfilingStatus { kErrorTooManyProfilers }; +/** + * Delegate for when max samples reached and samples are discarded. + */ +class V8_EXPORT DiscardedSamplesDelegate { + public: + DiscardedSamplesDelegate() {} + + virtual ~DiscardedSamplesDelegate() = default; + virtual void Notify() = 0; +}; + /** * Optional profiling attributes. */ @@ -346,8 +357,9 @@ class V8_EXPORT CpuProfiler { * profiles may be collected at once. Attempts to start collecting several * profiles with the same title are silently ignored. */ - CpuProfilingStatus StartProfiling(Local title, - CpuProfilingOptions options); + CpuProfilingStatus StartProfiling( + Local title, CpuProfilingOptions options, + std::unique_ptr delegate = nullptr); /** * Starts profiling with the same semantics as above, except with expanded diff --git a/third_party/v8/src/api/api.cc b/third_party/v8/src/api/api.cc index 6d4e3227e04c..74ea43bc769a 100644 --- a/third_party/v8/src/api/api.cc +++ b/third_party/v8/src/api/api.cc @@ -10750,10 +10750,11 @@ void CpuProfiler::SetUsePreciseSampling(bool use_precise_sampling) { use_precise_sampling); } -CpuProfilingStatus CpuProfiler::StartProfiling(Local title, - CpuProfilingOptions options) { +CpuProfilingStatus CpuProfiler::StartProfiling( + Local title, CpuProfilingOptions options, + std::unique_ptr delegate) { return reinterpret_cast(this)->StartProfiling( - *Utils::OpenHandle(*title), options); + *Utils::OpenHandle(*title), options, std::move(delegate)); } CpuProfilingStatus CpuProfiler::StartProfiling(Local title, diff --git a/third_party/v8/src/profiler/cpu-profiler.cc b/third_party/v8/src/profiler/cpu-profiler.cc index 6ee7539dda72..81e4c09e64fc 100644 --- a/third_party/v8/src/profiler/cpu-profiler.cc +++ b/third_party/v8/src/profiler/cpu-profiler.cc @@ -535,9 +535,11 @@ void CpuProfiler::CollectSample() { } } -CpuProfilingStatus CpuProfiler::StartProfiling(const char* title, - CpuProfilingOptions options) { - StartProfilingStatus status = profiles_->StartProfiling(title, options); +CpuProfilingStatus CpuProfiler::StartProfiling( + const char* title, CpuProfilingOptions options, + std::unique_ptr delegate) { + StartProfilingStatus status = + profiles_->StartProfiling(title, options, std::move(delegate)); // TODO(nicodubus): Revisit logic for if we want to do anything different for // kAlreadyStarted @@ -551,9 +553,11 @@ CpuProfilingStatus CpuProfiler::StartProfiling(const char* title, return status; } -CpuProfilingStatus CpuProfiler::StartProfiling(String title, - CpuProfilingOptions options) { - return StartProfiling(profiles_->GetName(title), options); +CpuProfilingStatus CpuProfiler::StartProfiling( + String title, CpuProfilingOptions options, + std::unique_ptr delegate) { + return StartProfiling(profiles_->GetName(title), options, + std::move(delegate)); } void CpuProfiler::StartProcessorIfNotStarted() { diff --git a/third_party/v8/src/profiler/cpu-profiler.h b/third_party/v8/src/profiler/cpu-profiler.h index e7ca3fbd7b23..292b4a97bf20 100644 --- a/third_party/v8/src/profiler/cpu-profiler.h +++ b/third_party/v8/src/profiler/cpu-profiler.h @@ -314,10 +314,12 @@ class V8_EXPORT_PRIVATE CpuProfiler { void set_sampling_interval(base::TimeDelta value); void set_use_precise_sampling(bool); void CollectSample(); - StartProfilingStatus StartProfiling(const char* title, - CpuProfilingOptions options = {}); - StartProfilingStatus StartProfiling(String title, - CpuProfilingOptions options = {}); + StartProfilingStatus StartProfiling( + const char* title, CpuProfilingOptions options = {}, + std::unique_ptr delegate = nullptr); + StartProfilingStatus StartProfiling( + String title, CpuProfilingOptions options = {}, + std::unique_ptr delegate = nullptr); CpuProfile* StopProfiling(const char* title); CpuProfile* StopProfiling(String title); diff --git a/third_party/v8/src/profiler/profile-generator.cc b/third_party/v8/src/profiler/profile-generator.cc index f3344c57a010..1694aad3e60d 100644 --- a/third_party/v8/src/profiler/profile-generator.cc +++ b/third_party/v8/src/profiler/profile-generator.cc @@ -468,9 +468,11 @@ using v8::tracing::TracedValue; std::atomic CpuProfile::last_id_; CpuProfile::CpuProfile(CpuProfiler* profiler, const char* title, - CpuProfilingOptions options) + CpuProfilingOptions options, + std::unique_ptr delegate) : title_(title), options_(options), + delegate_(std::move(delegate)), start_time_(base::TimeTicks::HighResolutionNow()), top_down_(profiler->isolate()), profiler_(profiler), @@ -517,8 +519,19 @@ void CpuProfile::AddPath(base::TimeTicks timestamp, (options_.max_samples() == CpuProfilingOptions::kNoSampleLimit || samples_.size() < options_.max_samples()); - if (should_record_sample) + if (should_record_sample) { samples_.push_back({top_frame_node, timestamp, src_line}); + } + + if (!should_record_sample && delegate_ != nullptr) { + const auto task_runner = V8::GetCurrentPlatform()->GetForegroundTaskRunner( + reinterpret_cast(profiler_->isolate())); + + task_runner->PostTask(std::make_unique( + std::move(delegate_))); + // std::move ensures that the delegate_ will be null on the next sample, + // so we don't post a task multiple times. + } const int kSamplesFlushCount = 100; const int kNodesFlushCount = 10; @@ -734,7 +747,8 @@ CpuProfilesCollection::CpuProfilesCollection(Isolate* isolate) : profiler_(nullptr), current_profiles_semaphore_(1) {} CpuProfilingStatus CpuProfilesCollection::StartProfiling( - const char* title, CpuProfilingOptions options) { + const char* title, CpuProfilingOptions options, + std::unique_ptr delegate) { current_profiles_semaphore_.Wait(); if (static_cast(current_profiles_.size()) >= kMaxSimultaneousProfiles) { @@ -750,7 +764,9 @@ CpuProfilingStatus CpuProfilesCollection::StartProfiling( return CpuProfilingStatus::kAlreadyStarted; } } - current_profiles_.emplace_back(new CpuProfile(profiler_, title, options)); + + current_profiles_.emplace_back( + new CpuProfile(profiler_, title, options, std::move(delegate))); current_profiles_semaphore_.Signal(); return CpuProfilingStatus::kStarted; } diff --git a/third_party/v8/src/profiler/profile-generator.h b/third_party/v8/src/profiler/profile-generator.h index 9183d56d4211..f819dfd98060 100644 --- a/third_party/v8/src/profiler/profile-generator.h +++ b/third_party/v8/src/profiler/profile-generator.h @@ -351,8 +351,9 @@ class CpuProfile { int line; }; - V8_EXPORT_PRIVATE CpuProfile(CpuProfiler* profiler, const char* title, - CpuProfilingOptions options); + V8_EXPORT_PRIVATE CpuProfile( + CpuProfiler* profiler, const char* title, CpuProfilingOptions options, + std::unique_ptr delegate = nullptr); // Checks whether or not the given TickSample should be (sub)sampled, given // the sampling interval of the profiler that recorded it (in microseconds). @@ -386,6 +387,7 @@ class CpuProfile { const char* title_; const CpuProfilingOptions options_; + std::unique_ptr delegate_; base::TimeTicks start_time_; base::TimeTicks end_time_; std::deque samples_; @@ -402,6 +404,18 @@ class CpuProfile { DISALLOW_COPY_AND_ASSIGN(CpuProfile); }; +class CpuProfileMaxSamplesCallbackTask : public v8::Task { + public: + CpuProfileMaxSamplesCallbackTask( + std::unique_ptr delegate) + : delegate_(std::move(delegate)) {} + + void Run() override { delegate_->Notify(); } + + private: + std::unique_ptr delegate_; +}; + class V8_EXPORT_PRIVATE CodeMap { public: CodeMap(); @@ -446,8 +460,9 @@ class V8_EXPORT_PRIVATE CpuProfilesCollection { explicit CpuProfilesCollection(Isolate* isolate); void set_cpu_profiler(CpuProfiler* profiler) { profiler_ = profiler; } - CpuProfilingStatus StartProfiling(const char* title, - CpuProfilingOptions options = {}); + CpuProfilingStatus StartProfiling( + const char* title, CpuProfilingOptions options = {}, + std::unique_ptr delegate = nullptr); CpuProfile* StopProfiling(const char* title); std::vector>* profiles() { diff --git a/third_party/v8/test/cctest/test-profile-generator.cc b/third_party/v8/test/cctest/test-profile-generator.cc index 02e65e76edf6..1e22aaae088a 100644 --- a/third_party/v8/test/cctest/test-profile-generator.cc +++ b/third_party/v8/test/cctest/test-profile-generator.cc @@ -518,6 +518,117 @@ TEST(SampleIds) { } } +namespace { +class DiscardedSamplesDelegateImpl : public v8::DiscardedSamplesDelegate { + public: + DiscardedSamplesDelegateImpl() : DiscardedSamplesDelegate() {} + void Notify() override {} +}; + +class MockPlatform : public TestPlatform { + public: + MockPlatform() + : old_platform_(i::V8::GetCurrentPlatform()), + mock_task_runner_(new MockTaskRunner()) { + // Now that it's completely constructed, make this the current platform. + i::V8::SetPlatformForTesting(this); + } + + // When done, explicitly revert to old_platform_. + ~MockPlatform() override { i::V8::SetPlatformForTesting(old_platform_); } + + std::shared_ptr GetForegroundTaskRunner( + v8::Isolate*) override { + return mock_task_runner_; + } + + int posted_count() { return mock_task_runner_->posted_count(); } + + private: + class MockTaskRunner : public v8::TaskRunner { + public: + void PostTask(std::unique_ptr task) override { + task->Run(); + posted_count_++; + } + + void PostDelayedTask(std::unique_ptr task, + double delay_in_seconds) override { + task_ = std::move(task); + delay_ = delay_in_seconds; + } + + void PostIdleTask(std::unique_ptr task) override { + UNREACHABLE(); + } + + bool IdleTasksEnabled() override { return false; } + + int posted_count() { return posted_count_; } + + private: + int posted_count_ = 0; + double delay_ = -1; + std::unique_ptr task_; + }; + + v8::Platform* old_platform_; + std::shared_ptr mock_task_runner_; +}; +} // namespace + +TEST(MaxSamplesCallback) { + i::Isolate* isolate = CcTest::i_isolate(); + CpuProfilesCollection profiles(isolate); + CpuProfiler profiler(isolate); + profiles.set_cpu_profiler(&profiler); + MockPlatform* mock_platform = new MockPlatform(); + std::unique_ptr impl = + std::make_unique( + DiscardedSamplesDelegateImpl()); + profiles.StartProfiling("", + {v8::CpuProfilingMode::kLeafNodeLineNumbers, 1, 1, + MaybeLocal()}, + std::move(impl)); + + StringsStorage strings; + CodeMap code_map(strings); + Symbolizer symbolizer(&code_map); + TickSample sample1; + sample1.timestamp = v8::base::TimeTicks::HighResolutionNow(); + sample1.pc = ToPointer(0x1600); + sample1.stack[0] = ToPointer(0x1510); + sample1.frames_count = 1; + auto symbolized = symbolizer.SymbolizeTickSample(sample1); + profiles.AddPathToCurrentProfiles(sample1.timestamp, symbolized.stack_trace, + symbolized.src_line, true, + base::TimeDelta()); + CHECK_EQ(0, mock_platform->posted_count()); + TickSample sample2; + sample2.timestamp = v8::base::TimeTicks::HighResolutionNow(); + sample2.pc = ToPointer(0x1925); + sample2.stack[0] = ToPointer(0x1780); + sample2.frames_count = 2; + symbolized = symbolizer.SymbolizeTickSample(sample2); + profiles.AddPathToCurrentProfiles(sample2.timestamp, symbolized.stack_trace, + symbolized.src_line, true, + base::TimeDelta()); + CHECK_EQ(1, mock_platform->posted_count()); + TickSample sample3; + sample3.timestamp = v8::base::TimeTicks::HighResolutionNow(); + sample3.pc = ToPointer(0x1510); + sample3.frames_count = 3; + symbolized = symbolizer.SymbolizeTickSample(sample3); + profiles.AddPathToCurrentProfiles(sample3.timestamp, symbolized.stack_trace, + symbolized.src_line, true, + base::TimeDelta()); + CHECK_EQ(1, mock_platform->posted_count()); + + // Teardown + profiles.StopProfiling(""); + delete mock_platform; +} + TEST(NoSamples) { TestSetup test_setup; i::Isolate* isolate = CcTest::i_isolate();