[cpu-profiler] Remove registration and sampling depth from Sampler

Simplify the internal state of Sampler a bit. There are basically two
users of Sampler - the CpuSampler used by the CpuProfiler and the
Ticker used by log.cc. Ticker calls Start/Stop to manage the Sampler
lifetime, but CpuProfiler does not. This leads to much confusion and
overlap of functionality.

Fix that here by removing the distinction between active, registered
and isProfiling states. These are now all the same thing and are
represented by IsActive(). The state is set to active when Start is
called, and set inactive when Stop is called. Both users of Sampler
now call Start and Stop at appropriate times.

The concept of profiling depth was not used - each Sampler would
only ever have a sampling depth of 1. We still need to call
SignalHandler::IncreaseSamplerCount(), so we do that in Start
and the corresponding DecreaseSamplerCount() in Stop.

Change-Id: I16a9435d26169a7dd00b1c7876e66af45f12e4b0
Reviewed-on: https://chromium-review.googlesource.com/c/1424337
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58955}

src/libsampler/sampler.cc

@@ -197,7 +197,7 @@ class Sampler::PlatformData {
 
 void SamplerManager::AddSampler(Sampler* sampler) {
   AtomicGuard atomic_guard(&samplers_access_counter_);
-  DCHECK(sampler->IsActive() || !sampler->IsRegistered());
+  DCHECK(sampler->IsActive());
   pthread_t thread_id = sampler->platform_data()->vm_tid();
   auto it = sampler_map_.find(thread_id);
   if (it == sampler_map_.end()) {
@@ -213,7 +213,7 @@ void SamplerManager::AddSampler(Sampler* sampler) {
 
 void SamplerManager::RemoveSampler(Sampler* sampler) {
   AtomicGuard atomic_guard(&samplers_access_counter_);
-  DCHECK(sampler->IsActive() || sampler->IsRegistered());
+  DCHECK(sampler->IsActive());
   pthread_t thread_id = sampler->platform_data()->vm_tid();
   auto it = sampler_map_.find(thread_id);
   DCHECK_NE(it, sampler_map_.end());
@@ -515,63 +515,33 @@ void SignalHandler::FillRegisterState(void* context, RegisterState* state) {
 Sampler::Sampler(Isolate* isolate)
     : isolate_(isolate), data_(base::make_unique<PlatformData>()) {}
 
-void Sampler::UnregisterIfRegistered() {
-#if defined(USE_SIGNALS)
-  if (IsRegistered()) {
-    SamplerManager::instance()->RemoveSampler(this);
-    SetRegistered(false);
-  }
-#endif
-}
-
 Sampler::~Sampler() {
   DCHECK(!IsActive());
-  DCHECK(!IsRegistered());
 }
 
 void Sampler::Start() {
   DCHECK(!IsActive());
   SetActive(true);
 #if defined(USE_SIGNALS)
+  SignalHandler::IncreaseSamplerCount();
   SamplerManager::instance()->AddSampler(this);
 #endif
 }
 
-
 void Sampler::Stop() {
 #if defined(USE_SIGNALS)
   SamplerManager::instance()->RemoveSampler(this);
+  SignalHandler::DecreaseSamplerCount();
 #endif
   DCHECK(IsActive());
   SetActive(false);
-  SetRegistered(false);
 }
 
-
-void Sampler::IncreaseProfilingDepth() {
-  profiling_++;
-#if defined(USE_SIGNALS)
-  SignalHandler::IncreaseSamplerCount();
-#endif
-}
-
-
-void Sampler::DecreaseProfilingDepth() {
-#if defined(USE_SIGNALS)
-  SignalHandler::DecreaseSamplerCount();
-#endif
-  profiling_--;
-}
-
-
 #if defined(USE_SIGNALS)
 
 void Sampler::DoSample() {
   if (!SignalHandler::Installed()) return;
-  if (!IsActive() && !IsRegistered()) {
-    SamplerManager::instance()->AddSampler(this);
-    SetRegistered(true);
-  }
+  DCHECK(IsActive());
   pthread_kill(platform_data()->vm_tid(), SIGPROF);
 }
 

src/libsampler/sampler.h

@@ -46,29 +46,9 @@ class Sampler {
   void Start();
   void Stop();
 
-  // Whether the sampling thread should use this Sampler for CPU profiling?
-  bool IsProfiling() const {
-    return profiling_.load(std::memory_order_relaxed) > 0;
-  }
-  void IncreaseProfilingDepth();
-  void DecreaseProfilingDepth();
-
-  // Whether the sampler is running (that is, consumes resources).
+  // Whether the sampler is running (start has been called).
   bool IsActive() const { return active_.load(std::memory_order_relaxed); }
 
-  // CpuProfiler collects samples by calling DoSample directly
-  // without calling Start. To keep it working, we register the sampler
-  // with the CpuProfiler.
-  bool IsRegistered() const {
-    return registered_.load(std::memory_order_relaxed);
-  }
-
-  // The sampler must be unregistered with the SamplerManager before ~Sampler()
-  // is called. If this doesn't happen, the signal handler might interrupt
-  // during the destructor and call DoSample(), which calls the pure virtual
-  // function Sampler::SampleStack(), causing a crash.
-  void UnregisterIfRegistered();
-
   void DoSample();
 
   // Used in tests to make sure that stack sampling is performed.
@@ -92,14 +72,9 @@ class Sampler {
   void SetActive(bool value) {
     active_.store(value, std::memory_order_relaxed);
   }
-  void SetRegistered(bool value) {
-    registered_.store(value, std::memory_order_relaxed);
-  }
 
   Isolate* isolate_;
-  std::atomic<std::int32_t> profiling_{0};
   std::atomic_bool active_{false};
-  std::atomic_bool registered_{false};
   std::unique_ptr<PlatformData> data_;  // Platform specific data.
   DISALLOW_IMPLICIT_CONSTRUCTORS(Sampler);
 };

src/log.cc

@@ -754,7 +754,7 @@ class SamplingThread : public base::Thread {
         sampler_(sampler),
         interval_microseconds_(interval_microseconds) {}
   void Run() override {
-    while (sampler_->IsProfiling()) {
+    while (sampler_->IsActive()) {
       sampler_->DoSample();
       base::OS::Sleep(
           base::TimeDelta::FromMicroseconds(interval_microseconds_));
@@ -854,7 +854,6 @@ class Ticker: public sampler::Sampler {
   void SetProfiler(Profiler* profiler) {
     DCHECK_NULL(profiler_);
     profiler_ = profiler;
-    IncreaseProfilingDepth();
     if (!IsActive()) Start();
     sampling_thread_->StartSynchronously();
   }
@@ -862,7 +861,6 @@ class Ticker: public sampler::Sampler {
   void ClearProfiler() {
     profiler_ = nullptr;
     if (IsActive()) Stop();
-    DecreaseProfilingDepth();
     sampling_thread_->Join();
   }
 

src/profiler/cpu-profiler.cc

@@ -60,13 +60,10 @@ SamplingEventsProcessor::SamplingEventsProcessor(Isolate* isolate,
     : ProfilerEventsProcessor(isolate, generator),
       sampler_(new CpuSampler(isolate, this)),
       period_(period) {
-  sampler_->IncreaseProfilingDepth();
+  sampler_->Start();
 }
 
-SamplingEventsProcessor::~SamplingEventsProcessor() {
-  sampler_->DecreaseProfilingDepth();
-  sampler_->UnregisterIfRegistered();
-}
+SamplingEventsProcessor::~SamplingEventsProcessor() { sampler_->Stop(); }
 
 ProfilerEventsProcessor::~ProfilerEventsProcessor() = default;
 

test/cctest/libsampler/test-sampler.cc

@@ -26,7 +26,7 @@ class TestSamplingThread : public base::Thread {
 
   // Implement Thread::Run().
   void Run() override {
-    while (sampler_->IsProfiling()) {
+    while (sampler_->IsActive()) {
       sampler_->DoSample();
       base::OS::Sleep(base::TimeDelta::FromMilliseconds(1));
     }
@@ -75,7 +75,6 @@ static void RunSampler(v8::Local<v8::Context> env,
                        unsigned min_external_samples = 0) {
   TestSampler sampler(env->GetIsolate());
   TestSamplingThread thread(&sampler);
-  sampler.IncreaseProfilingDepth();
   sampler.Start();
   sampler.StartCountingSamples();
   thread.StartSynchronously();
@@ -84,7 +83,6 @@ static void RunSampler(v8::Local<v8::Context> env,
   } while (sampler.js_sample_count() < min_js_samples ||
            sampler.external_sample_count() < min_external_samples);
   sampler.Stop();
-  sampler.DecreaseProfilingDepth();
   thread.Join();
 }
 
@@ -154,6 +152,7 @@ TEST(SamplerManager_AddRemoveSampler) {
 
   SamplerManager* manager = SamplerManager::instance();
   CountingSampler sampler1(isolate);
+  sampler1.set_active(true);
   CHECK_EQ(0, sampler1.sample_count());
 
   manager->AddSampler(&sampler1);
@@ -177,12 +176,14 @@ TEST(SamplerManager_DoesNotReAdd) {
   // Add the same sampler twice, but check we only get one sample for it.
   SamplerManager* manager = SamplerManager::instance();
   CountingSampler sampler1(isolate);
+  sampler1.set_active(true);
   manager->AddSampler(&sampler1);
   manager->AddSampler(&sampler1);
 
   RegisterState state;
   manager->DoSample(state);
   CHECK_EQ(1, sampler1.sample_count());
+  sampler1.set_active(false);
 }
 
 TEST(AtomicGuard_GetNonBlockingSuccess) {