[cpu-profiler] Return CpuStartProfilingStatus when starting profiling

 - Created status enum with statuses kStarted, kAlreadyStarted and
kErrorTooManyProfilers, returning when StartProfiling is invoked
 - Tests spin up one profiler, check kStarted returned; spin up
another with same name, check kAlreadyStarted returned; Spin up 99
more profilers (100 total), check each returning kStarted, and
one more, expecting 101st to return kErrorTooManyProfilers

R=acomminos@fb.com, petermarshall@chromium.org, ulan@chromium.org

Change-Id: I64e2e6396775f90f9f49f75331a075a47efa7fca
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2486240Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70808}

include/v8-profiler.h

@@ -249,6 +249,15 @@ enum CpuProfilingLoggingMode {
   kEagerLogging,
 };
 
+// Enum for returning profiling status. Once StartProfiling is called,
+// we want to return to clients whether the profiling was able to start
+// correctly, or return a descriptive error.
+enum class CpuProfilingStatus {
+  kStarted,
+  kAlreadyStarted,
+  kErrorTooManyProfilers
+};
+
 /**
  * Optional profiling attributes.
  */
@@ -337,7 +346,8 @@ class V8_EXPORT CpuProfiler {
    * profiles may be collected at once. Attempts to start collecting several
    * profiles with the same title are silently ignored.
    */
-  void StartProfiling(Local<String> title, CpuProfilingOptions options);
+  CpuProfilingStatus StartProfiling(Local<String> title,
+                                    CpuProfilingOptions options);
 
   /**
    * Starts profiling with the same semantics as above, except with expanded
@@ -350,7 +360,7 @@ class V8_EXPORT CpuProfiler {
    * recorded by the profiler. Samples obtained after this limit will be
    * discarded.
    */
-  void StartProfiling(
+  CpuProfilingStatus StartProfiling(
       Local<String> title, CpuProfilingMode mode, bool record_samples = false,
       unsigned max_samples = CpuProfilingOptions::kNoSampleLimit);
   /**
@@ -358,7 +368,8 @@ class V8_EXPORT CpuProfiler {
    * kLeafNodeLineNumbers mode, which was the previous default behavior of the
    * profiler.
    */
-  void StartProfiling(Local<String> title, bool record_samples = false);
+  CpuProfilingStatus StartProfiling(Local<String> title,
+                                    bool record_samples = false);
 
   /**
    * Stops collecting CPU profile with a given title and returns it.

src/api/api.cc

@@ -10693,24 +10693,27 @@ void CpuProfiler::SetUsePreciseSampling(bool use_precise_sampling) {
       use_precise_sampling);
 }
 
-void CpuProfiler::StartProfiling(Local<String> title,
-                                 CpuProfilingOptions options) {
-  reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
+CpuProfilingStatus CpuProfiler::StartProfiling(Local<String> title,
+                                               CpuProfilingOptions options) {
+  return reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
       *Utils::OpenHandle(*title), options);
 }
 
-void CpuProfiler::StartProfiling(Local<String> title, bool record_samples) {
+CpuProfilingStatus CpuProfiler::StartProfiling(Local<String> title,
+                                               bool record_samples) {
   CpuProfilingOptions options(
       kLeafNodeLineNumbers,
       record_samples ? CpuProfilingOptions::kNoSampleLimit : 0);
-  reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
+  return reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
       *Utils::OpenHandle(*title), options);
 }
 
-void CpuProfiler::StartProfiling(Local<String> title, CpuProfilingMode mode,
-                                 bool record_samples, unsigned max_samples) {
+CpuProfilingStatus CpuProfiler::StartProfiling(Local<String> title,
+                                               CpuProfilingMode mode,
+                                               bool record_samples,
+                                               unsigned max_samples) {
   CpuProfilingOptions options(mode, record_samples ? max_samples : 0);
-  reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
+  return reinterpret_cast<i::CpuProfiler*>(this)->StartProfiling(
       *Utils::OpenHandle(*title), options);
 }
 

src/profiler/cpu-profiler.cc

@@ -535,17 +535,25 @@ void CpuProfiler::CollectSample() {
   }
 }
 
-void CpuProfiler::StartProfiling(const char* title,
-                                 CpuProfilingOptions options) {
-  if (profiles_->StartProfiling(title, options)) {
+CpuProfilingStatus CpuProfiler::StartProfiling(const char* title,
+                                               CpuProfilingOptions options) {
+  StartProfilingStatus status = profiles_->StartProfiling(title, options);
+
+  // TODO(nicodubus): Revisit logic for if we want to do anything different for
+  // kAlreadyStarted
+  if (status == CpuProfilingStatus::kStarted ||
+      status == CpuProfilingStatus::kAlreadyStarted) {
     TRACE_EVENT0("v8", "CpuProfiler::StartProfiling");
     AdjustSamplingInterval();
     StartProcessorIfNotStarted();
   }
+
+  return status;
 }
 
-void CpuProfiler::StartProfiling(String title, CpuProfilingOptions options) {
-  StartProfiling(profiles_->GetName(title), options);
+CpuProfilingStatus CpuProfiler::StartProfiling(String title,
+                                               CpuProfilingOptions options) {
+  return StartProfiling(profiles_->GetName(title), options);
 }
 
 void CpuProfiler::StartProcessorIfNotStarted() {

src/profiler/cpu-profiler.h

@@ -308,13 +308,16 @@ class V8_EXPORT_PRIVATE CpuProfiler {
   using ProfilingMode = v8::CpuProfilingMode;
   using NamingMode = v8::CpuProfilingNamingMode;
   using LoggingMode = v8::CpuProfilingLoggingMode;
+  using StartProfilingStatus = CpuProfilingStatus;
 
   base::TimeDelta sampling_interval() const { return base_sampling_interval_; }
   void set_sampling_interval(base::TimeDelta value);
   void set_use_precise_sampling(bool);
   void CollectSample();
-  void StartProfiling(const char* title, CpuProfilingOptions options = {});
-  void StartProfiling(String title, CpuProfilingOptions options = {});
+  StartProfilingStatus StartProfiling(const char* title,
+                                      CpuProfilingOptions options = {});
+  StartProfilingStatus StartProfiling(String title,
+                                      CpuProfilingOptions options = {});
 
   CpuProfile* StopProfiling(const char* title);
   CpuProfile* StopProfiling(String title);

src/profiler/profile-generator.cc

@@ -733,24 +733,26 @@ void CodeMap::Print() {
 CpuProfilesCollection::CpuProfilesCollection(Isolate* isolate)
     : profiler_(nullptr), current_profiles_semaphore_(1) {}
 
-bool CpuProfilesCollection::StartProfiling(const char* title,
-                                           CpuProfilingOptions options) {
+CpuProfilingStatus CpuProfilesCollection::StartProfiling(
+    const char* title, CpuProfilingOptions options) {
   current_profiles_semaphore_.Wait();
+
   if (static_cast<int>(current_profiles_.size()) >= kMaxSimultaneousProfiles) {
     current_profiles_semaphore_.Signal();
-    return false;
+
+    return CpuProfilingStatus::kErrorTooManyProfilers;
   }
   for (const std::unique_ptr<CpuProfile>& profile : current_profiles_) {
     if (strcmp(profile->title(), title) == 0) {
       // Ignore attempts to start profile with the same title...
       current_profiles_semaphore_.Signal();
-      // ... though return true to force it collect a sample.
-      return true;
+      // ... though return kAlreadyStarted to force it collect a sample.
+      return CpuProfilingStatus::kAlreadyStarted;
     }
   }
   current_profiles_.emplace_back(new CpuProfile(profiler_, title, options));
   current_profiles_semaphore_.Signal();
-  return true;
+  return CpuProfilingStatus::kStarted;
 }
 
 CpuProfile* CpuProfilesCollection::StopProfiling(const char* title) {
@@ -775,7 +777,6 @@ CpuProfile* CpuProfilesCollection::StopProfiling(const char* title) {
   return profile;
 }
 
-
 bool CpuProfilesCollection::IsLastProfile(const char* title) {
   // Called from VM thread, and only it can mutate the list,
   // so no locking is needed here.

src/profiler/profile-generator.h

@@ -446,7 +446,8 @@ class V8_EXPORT_PRIVATE CpuProfilesCollection {
   explicit CpuProfilesCollection(Isolate* isolate);
 
   void set_cpu_profiler(CpuProfiler* profiler) { profiler_ = profiler; }
-  bool StartProfiling(const char* title, CpuProfilingOptions options = {});
+  CpuProfilingStatus StartProfiling(const char* title,
+                                    CpuProfilingOptions options = {});
 
   CpuProfile* StopProfiling(const char* title);
   std::vector<std::unique_ptr<CpuProfile>>* profiles() {

test/cctest/test-cpu-profiler.cc

@@ -3366,6 +3366,54 @@ TEST(FastStopProfiling) {
   CHECK_LT(duration, kWaitThreshold.InMillisecondsF());
 }
 
+// Tests that when current_profiles->size() is greater than the max allowable
+// number of concurrent profiles (100), we don't allow a new Profile to be
+// profiled
+TEST(MaxSimultaneousProfiles) {
+  LocalContext env;
+  i::Isolate* isolate = CcTest::i_isolate();
+  i::HandleScope scope(isolate);
+
+  v8::CpuProfiler* profiler = v8::CpuProfiler::New(env->GetIsolate());
+
+  // Spin up first profiler. Verify that status is kStarted
+  CpuProfilingStatus firstStatus = profiler->StartProfiling(
+      v8_str("1us"), {v8::CpuProfilingMode::kLeafNodeLineNumbers,
+                      v8::CpuProfilingOptions::kNoSampleLimit, 1});
+
+  CHECK_EQ(firstStatus, CpuProfilingStatus::kStarted);
+
+  // Spin up profiler with same title. Verify that status is kAlreadyStarted
+  CpuProfilingStatus startedStatus = profiler->StartProfiling(
+      v8_str("1us"), {v8::CpuProfilingMode::kLeafNodeLineNumbers,
+                      v8::CpuProfilingOptions::kNoSampleLimit, 1});
+
+  CHECK_EQ(startedStatus, CpuProfilingStatus::kAlreadyStarted);
+
+  // Spin up 99 more profilers, maxing out CpuProfilersCollection.
+  // Check they all return status of kStarted
+  for (int i = 2; i <= CpuProfilesCollection::kMaxSimultaneousProfiles; i++) {
+    CpuProfilingStatus status =
+        profiler->StartProfiling(v8_str((std::to_string(i) + "us").c_str()),
+                                 {v8::CpuProfilingMode::kLeafNodeLineNumbers,
+                                  v8::CpuProfilingOptions::kNoSampleLimit, i});
+    CHECK_EQ(status, CpuProfilingStatus::kStarted);
+  }
+
+  // Spin up 101st profiler. Verify status is kErrorTooManyProfilers
+  CpuProfilingStatus errorStatus = profiler->StartProfiling(
+      v8_str("101us"), {v8::CpuProfilingMode::kLeafNodeLineNumbers,
+                        v8::CpuProfilingOptions::kNoSampleLimit, 2});
+
+  CHECK_EQ(errorStatus, CpuProfilingStatus::kErrorTooManyProfilers);
+
+  // Clean up, otherwise will show a crash.
+  for (int i = 1; i <= CpuProfilesCollection::kMaxSimultaneousProfiles + 1;
+       i++) {
+    profiler->StopProfiling(v8_str((std::to_string(i) + "us").c_str()));
+  }
+}
+
 TEST(LowPrecisionSamplingStartStopInternal) {
   i::Isolate* isolate = CcTest::i_isolate();
   CpuProfilesCollection profiles(isolate);

test/cctest/test-profile-generator.cc

@@ -615,10 +615,12 @@ TEST(Issue51919) {
   for (int i = 0; i < CpuProfilesCollection::kMaxSimultaneousProfiles; ++i) {
     i::Vector<char> title = i::Vector<char>::New(16);
     i::SNPrintF(title, "%d", i);
-    CHECK(collection.StartProfiling(title.begin()));
+    CHECK_EQ(CpuProfilingStatus::kStarted,
+             collection.StartProfiling(title.begin()));
     titles[i] = title.begin();
   }
-  CHECK(!collection.StartProfiling("maximum"));
+  CHECK_EQ(CpuProfilingStatus::kErrorTooManyProfilers,
+           collection.StartProfiling("maximum"));
   for (int i = 0; i < CpuProfilesCollection::kMaxSimultaneousProfiles; ++i)
     i::DeleteArray(titles[i]);
 }