[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 <[email protected]> Reviewed-by: Ulan Degenbaev <[email protected]> Reviewed-by: Peter Marshall <[email protected]> Cr-Commit-Position: refs/heads/master@{#72794}
youtube · Nov 30, 2023 · c0d7739 · c0d7739
1 parent 84cb637
commit c0d7739
Show file tree

Hide file tree

Showing 7 changed files with 184 additions and 23 deletions.
diff --git 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<String> title,
-                                    CpuProfilingOptions options);
+  CpuProfilingStatus StartProfiling(
+      Local<String> title, CpuProfilingOptions options,
+      std::unique_ptr<DiscardedSamplesDelegate> 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
@@ -10750,10 +10750,11 @@ void CpuProfiler::SetUsePreciseSampling(bool use_precise_sampling) {
       use_precise_sampling);
 }
 
-CpuProfilingStatus CpuProfiler::StartProfiling(Local<String> title,
-                                               CpuProfilingOptions options) {
+CpuProfilingStatus CpuProfiler::StartProfiling(
+    Local<String> title, CpuProfilingOptions options,
+    std::unique_ptr<DiscardedSamplesDelegate> delegate) {
   return reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
-      *Utils::OpenHandle(*title), options);
+      *Utils::OpenHandle(*title), options, std::move(delegate));
 }
 
 CpuProfilingStatus CpuProfiler::StartProfiling(Local<String> title,

diff --git 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<DiscardedSamplesDelegate> 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<DiscardedSamplesDelegate> 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
@@ -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<DiscardedSamplesDelegate> delegate = nullptr);
+  StartProfilingStatus StartProfiling(
+      String title, CpuProfilingOptions options = {},
+      std::unique_ptr<DiscardedSamplesDelegate> 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
@@ -468,9 +468,11 @@ using v8::tracing::TracedValue;
 std::atomic<uint32_t> CpuProfile::last_id_;
 
 CpuProfile::CpuProfile(CpuProfiler* profiler, const char* title,
-                       CpuProfilingOptions options)
+                       CpuProfilingOptions options,
+                       std::unique_ptr<DiscardedSamplesDelegate> 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<v8::Isolate*>(profiler_->isolate()));
+
+    task_runner->PostTask(std::make_unique<CpuProfileMaxSamplesCallbackTask>(
+        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<DiscardedSamplesDelegate> delegate) {
   current_profiles_semaphore_.Wait();
 
   if (static_cast<int>(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
@@ -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<DiscardedSamplesDelegate> 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<DiscardedSamplesDelegate> delegate_;
   base::TimeTicks start_time_;
   base::TimeTicks end_time_;
   std::deque<SampleInfo> samples_;
@@ -402,6 +404,18 @@ class CpuProfile {
   DISALLOW_COPY_AND_ASSIGN(CpuProfile);
 };
 
+class CpuProfileMaxSamplesCallbackTask : public v8::Task {
+ public:
+  CpuProfileMaxSamplesCallbackTask(
+      std::unique_ptr<DiscardedSamplesDelegate> delegate)
+      : delegate_(std::move(delegate)) {}
+
+  void Run() override { delegate_->Notify(); }
+
+ private:
+  std::unique_ptr<DiscardedSamplesDelegate> 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<DiscardedSamplesDelegate> delegate = nullptr);
 
   CpuProfile* StopProfiling(const char* title);
   std::vector<std::unique_ptr<CpuProfile>>* profiles() {

diff --git 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<v8::TaskRunner> 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<v8::Task> task) override {
+      task->Run();
+      posted_count_++;
+    }
+
+    void PostDelayedTask(std::unique_ptr<Task> task,
+                         double delay_in_seconds) override {
+      task_ = std::move(task);
+      delay_ = delay_in_seconds;
+    }
+
+    void PostIdleTask(std::unique_ptr<IdleTask> 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> task_;
+  };
+
+  v8::Platform* old_platform_;
+  std::shared_ptr<MockTaskRunner> 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<DiscardedSamplesDelegateImpl> impl =
+      std::make_unique<DiscardedSamplesDelegateImpl>(
+          DiscardedSamplesDelegateImpl());
+  profiles.StartProfiling("",
+                          {v8::CpuProfilingMode::kLeafNodeLineNumbers, 1, 1,
+                           MaybeLocal<v8::Context>()},
+                          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();