Get rid of UnsafeCurrent in Sampler

src/profiler/sampler.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/profiler/sampler.h"
 
 #if V8_OS_POSIX && !V8_OS_CYGWIN
 
 #define USE_SIGNALS
 
@@ skipping 24 matching lines @@
     !defined(__BIONIC_HAVE_STRUCT_SIGCONTEXT)
 #include <asm/sigcontext.h>  // NOLINT
 #endif
 
 #elif V8_OS_WIN || V8_OS_CYGWIN
 
 #include "src/base/win32-headers.h"
 
 #endif
 
+#include "src/atomic-utils.h"
 #include "src/base/platform/platform.h"
 #include "src/flags.h"
 #include "src/frames-inl.h"
 #include "src/log.h"
 #include "src/profiler/cpu-profiler-inl.h"
 #include "src/simulator.h"
 #include "src/v8threads.h"
 #include "src/vm-state-inl.h"
 
 
@@ skipping 174 matching lines @@
         // pessimistically assume it could be the entire pattern match.
         MSAN_MEMORY_IS_INITIALIZED(pc, pattern->bytes_count - offset);
         if (!memcmp(pc, pattern->bytes + offset, pattern->bytes_count - offset))
           return true;
       }
     }
   }
   return false;
 }
 
+typedef List<Sampler*> SamplerList;
+
+#if defined(USE_SIGNALS)
+class ThreadSamplersPair {

[alph, 2016/04/06 22:21:26]

Is it possible to use std::pair?

[lpy, 2016/04/06 23:51:37]

Done.

+ public:
+  ThreadSamplersPair(pthread_t thread_id, SamplerList* samplers)
+      : thread_id_(thread_id), samplers_(samplers) {}
+  ~ThreadSamplersPair() {
+    delete samplers_;
+  }
+  pthread_t thread_id() const { return thread_id_; }
+  SamplerList* samplers() const { return samplers_; }
+  bool operator==(const ThreadSamplersPair& rhs) {
+    return thread_id_ == rhs.thread_id();
+  }
+ private:
+  pthread_t thread_id_;
+  SamplerList* samplers_;
+  ThreadSamplersPair() {}
+};
+
+typedef List<ThreadSamplersPair*> ThreadSamplersList;
+
+template <typename T>

[alph, 2016/04/06 22:21:26]

Why a template? Do you plan to use it elsewhere with a different type?

[lpy, 2016/04/06 23:51:37]

Done.

+class AtomicGuard {
+ public:
+  explicit AtomicGuard(AtomicValue<T>* atomic, T expected, T desired,

[alph, 2016/04/06 22:21:26]

Drop explicit
Also, can you just hardcode expected=0, desired=1? 

[lpy, 2016/04/06 23:51:37]

I don't understand, what's the benefit to hard code?

[alph, 2016/04/07 00:40:10]

It is always 0,1 at the call sites. So you use it as a classic mutex everywhere.
Why would you expose this complication to the class usages?

+                       bool is_block)
+      : atomic_(atomic),
+        expected_(expected),
+        desired_(desired),
+        is_success_(false) {
+    do {
+      is_success_ = atomic_->TrySetValue(expected, desired);
+      if (is_success_) break;
+    } while (is_block);

[alph, 2016/04/06 22:21:26]

while (is_block && !is_success_)

[lpy, 2016/04/06 23:51:37]

if is_block is set to false, then we only want to try to set the value of the
atomic once.

[alph, 2016/04/07 00:40:10]

the code I suggest is semantically equivalent to your code. It just more compact
and straightforward.

+  }
+
+  bool is_success() { return is_success_; }
+
+  ~AtomicGuard() {
+    if (is_success_)
+      atomic_->SetValue(expected_);

[alph, 2016/04/06 22:21:25]

v8 style requires block statement here

[lpy, 2016/04/06 23:51:37]

Done.

+    atomic_ = NULL;
+  }
+
+ private:
+  AtomicValue<T>* atomic_;
+  T expected_;
+  T desired_;

[alph, 2016/04/06 22:21:25]

not used

[lpy, 2016/04/06 23:51:37]

Done.

+  bool is_success_;
+};
+#endif  // USE_SIGNALS
+
 }  // namespace
 
 #if defined(USE_SIGNALS)
 
 class Sampler::PlatformData : public PlatformDataCommon {
  public:
   PlatformData() : vm_tid_(pthread_self()) {}
   pthread_t vm_tid() const { return vm_tid_; }
 
  private:
@@ skipping 118 matching lines @@
 
   static void DecreaseSamplerCount() {
     base::LockGuard<base::Mutex> lock_guard(mutex_);
     if (--client_count_ == 0) Restore();
   }
 
   static bool Installed() {
     return signal_handler_installed_;
   }
 
+#if !V8_OS_NACL
+  static void CollectSample(void* context, Sampler* sampler);
+#endif
+
  private:
   static void Install() {
 #if !V8_OS_NACL
     struct sigaction sa;
     sa.sa_sigaction = &HandleProfilerSignal;
     sigemptyset(&sa.sa_mask);
 #if V8_OS_QNX
     sa.sa_flags = SA_SIGINFO;
 #else
     sa.sa_flags = SA_RESTART | SA_SIGINFO;
@@ skipping 24 matching lines @@
 
 
 base::Mutex* SignalHandler::mutex_ = NULL;
 int SignalHandler::client_count_ = 0;
 struct sigaction SignalHandler::old_signal_handler_;
 bool SignalHandler::signal_handler_installed_ = false;
 
 
 // As Native Client does not support signal handling, profiling is disabled.
 #if !V8_OS_NACL
-void SignalHandler::HandleProfilerSignal(int signal, siginfo_t* info,
-                                         void* context) {
-  USE(info);
-  if (signal != SIGPROF) return;
-  Isolate* isolate = Isolate::UnsafeCurrent();
-  if (isolate == NULL || !isolate->IsInUse()) {
-    // We require a fully initialized and entered isolate.
+void SignalHandler::CollectSample(void* context, Sampler* sampler) {
+  if (sampler == NULL || (!sampler->IsProfiling() && !sampler->IsRegistered()))
     return;
-  }
+  Isolate* isolate = sampler->isolate();
+
+  // We require a fully initialized and entered isolate.
+  if (isolate == NULL || !isolate->IsInUse()) return;
+
   if (v8::Locker::IsActive() &&
       !isolate->thread_manager()->IsLockedByCurrentThread()) {
     return;
   }
 
-  Sampler* sampler = isolate->logger()->sampler();
-  if (sampler == NULL) return;
-
   v8::RegisterState state;
 
 #if defined(USE_SIMULATOR)
   SimulatorHelper helper;
   if (!helper.Init(isolate)) return;
   helper.FillRegisters(&state);
   // It possible that the simulator is interrupted while it is updating
   // the sp or fp register. ARM64 simulator does this in two steps:
   // first setting it to zero and then setting it to the new value.
   // Bailout if sp/fp doesn't contain the new value.
@@ skipping 152 matching lines @@
     bool need_to_start = false;
     base::LockGuard<base::Mutex> lock_guard(mutex_);
     if (instance_ == NULL) {
       // Start a thread that will send SIGPROF signal to VM threads,
       // when CPU profiling will be enabled.
       instance_ = new SamplerThread(sampler->interval());
       need_to_start = true;
     }
 
     DCHECK(sampler->IsActive());
+    DCHECK(instance_->interval_ == sampler->interval());
+
+#if defined(USE_SIGNALS)
+    AddSampler(sampler);
+#else
     DCHECK(!instance_->active_samplers_.Contains(sampler));
-    DCHECK(instance_->interval_ == sampler->interval());
     instance_->active_samplers_.Add(sampler);
+#endif  // USE_SIGNALS
 
     if (need_to_start) instance_->StartSynchronously();
   }
 
-  static void RemoveActiveSampler(Sampler* sampler) {
+  static void RemoveSampler(Sampler* sampler) {
     SamplerThread* instance_to_remove = NULL;
     {
       base::LockGuard<base::Mutex> lock_guard(mutex_);
 
-      DCHECK(sampler->IsActive());
+      DCHECK(sampler->IsActive() || sampler->IsRegistered());
+#if defined(USE_SIGNALS)
+      {
+        AtomicGuard<int> atomic_guard(&sampler_list_access_counter_,
+                                      0, 1, true);
+        // Remove sampler from map.
+        pthread_t thread_id = sampler->platform_data()->vm_tid();
+        SamplerList* samplers = NULL;
+        int i = 0;
+        for (; i < thread_id_to_samplers_.length(); ++i) {
+          ThreadSamplersPair* tsp = thread_id_to_samplers_.at(i);
+          if (pthread_equal(tsp->thread_id(), thread_id) != 0) {
+            samplers = tsp->samplers();
+            break;
+          }
+        }
+        if (samplers != NULL) {
+          samplers->RemoveElement(sampler);
+          if (samplers->is_empty()) {
+            ThreadSamplersPair* tsp = thread_id_to_samplers_.Remove(i);
+            delete tsp;
+          }
+        }
+        if (thread_id_to_samplers_.is_empty()) {
+          instance_to_remove = instance_;
+          instance_ = NULL;
+        }
+      }
+#else
       bool removed = instance_->active_samplers_.RemoveElement(sampler);
       DCHECK(removed);
       USE(removed);
 
       // We cannot delete the instance immediately as we need to Join() the
       // thread but we are holding mutex_ and the thread may try to acquire it.
       if (instance_->active_samplers_.is_empty()) {
         instance_to_remove = instance_;
         instance_ = NULL;
       }
+#endif  // USE_SIGNALS
     }
 
     if (!instance_to_remove) return;
     instance_to_remove->Join();
     delete instance_to_remove;
   }
 
+  // Unlike AddActiveSampler, this method only adds a sampler,
+  // but won't start the sampler thread.
+  static void RegisterSampler(Sampler* sampler) {
+    base::LockGuard<base::Mutex> lock_guard(mutex_);
+#if defined(USE_SIGNALS)
+    AddSampler(sampler);
+#endif  // USE_SIGNALS
+  }
+
   // Implement Thread::Run().
   virtual void Run() {
     while (true) {
       {
         base::LockGuard<base::Mutex> lock_guard(mutex_);
+#if defined(USE_SIGNALS)
+        if (thread_id_to_samplers_.is_empty()) break;
+        if (SignalHandler::Installed()) {
+          for (int i = 0; i < thread_id_to_samplers_.length(); ++i) {
+            pthread_t thread_id = thread_id_to_samplers_.at(i)->thread_id();
+            pthread_kill(thread_id, SIGPROF);
+          }
+        }
+#else
         if (active_samplers_.is_empty()) break;
         // When CPU profiling is enabled both JavaScript and C++ code is
         // profiled. We must not suspend.
         for (int i = 0; i < active_samplers_.length(); ++i) {
           Sampler* sampler = active_samplers_.at(i);
           if (!sampler->IsProfiling()) continue;
           sampler->DoSample();
         }
+#endif  // USE_SIGNALS
       }
       base::OS::Sleep(base::TimeDelta::FromMilliseconds(interval_));
     }
   }
 
  private:
   // Protects the process wide state below.
   static base::Mutex* mutex_;
   static SamplerThread* instance_;
 
   const int interval_;
-  List<Sampler*> active_samplers_;
+
+#if defined(USE_SIGNALS)
+  friend class SignalHandler;
+  static ThreadSamplersList thread_id_to_samplers_;
+  static AtomicValue<int> sampler_list_access_counter_;
+  static void AddSampler(Sampler* sampler) {
+    AtomicGuard<int> atomic_guard(&SamplerThread::sampler_list_access_counter_,
+                                  0, 1, true);
+    // Add sampler into map if needed.
+    pthread_t thread_id = sampler->platform_data()->vm_tid();
+    SamplerList* samplers = NULL;
+    for (int i = 0; i < thread_id_to_samplers_.length(); ++i) {
+      ThreadSamplersPair* tsp = thread_id_to_samplers_.at(i);
+      if (pthread_equal(tsp->thread_id(), thread_id) != 0) {
+        samplers = tsp->samplers();
+        break;
+      }
+    }
+    if (samplers != NULL) {
+      if (!samplers->Contains(sampler))
+        samplers->Add(sampler);

[alph, 2016/04/06 22:21:26]

use {}

[lpy, 2016/04/06 23:51:37]

Done.

+    } else {
+      samplers = new SamplerList();
+      samplers->Add(sampler);
+      thread_id_to_samplers_.Add(new ThreadSamplersPair(thread_id, samplers));
+    }
+  }
+#else
+  SamplerList active_samplers_;
+#endif  // USE_SIGNALS
 
   DISALLOW_COPY_AND_ASSIGN(SamplerThread);
 };
 
 
 base::Mutex* SamplerThread::mutex_ = NULL;
 SamplerThread* SamplerThread::instance_ = NULL;
+#if defined(USE_SIGNALS)
+ThreadSamplersList SamplerThread::thread_id_to_samplers_;
+AtomicValue<int> SamplerThread::sampler_list_access_counter_(0);
+#endif  // USE_SIGNALS
+
+// As Native Client does not support signal handling, profiling is disabled.
+#if defined(USE_SIGNALS)

[alph, 2016/04/06 22:21:25]

you can have a single #if - #endif block for USE_SIGNALS

[lpy, 2016/04/06 23:51:37]

Done.

+#if !V8_OS_NACL
+void SignalHandler::HandleProfilerSignal(int signal, siginfo_t* info,
+                                         void* context) {
+  USE(info);
+  if (signal != SIGPROF) return;
+  AtomicGuard<int> atomic_guard(&SamplerThread::sampler_list_access_counter_,
+                                0, 1, false);
+  if (!atomic_guard.is_success()) return;
+  pthread_t thread_id = pthread_self();
+  SamplerList* samplers = NULL;
+  for (int i = 0; i < SamplerThread::thread_id_to_samplers_.length(); ++i) {

[alph, 2016/04/06 22:21:26]

Looks like thread_id_to_samplers_ should be a map, rather than list.

[lpy, 2016/04/06 23:51:38]

Done.

+    ThreadSamplersPair* tsp = SamplerThread::thread_id_to_samplers_.at(i);
+    if (pthread_equal(tsp->thread_id(), thread_id) != 0) {
+      samplers = tsp->samplers();
+      break;
+    }
+  }
+  DCHECK(samplers != NULL);

[alph, 2016/04/06 22:21:26]

This is not true. The samplers could get deleted while the signal was in flight.

[lpy, 2016/04/06 23:51:37]

Done.

+  for (int i = 0; i < samplers->length(); ++i) {
+    Sampler* sampler = samplers->at(i);
+    SignalHandler::CollectSample(context, sampler);

[alph, 2016/04/06 22:21:26]

drop SignalHandler::

[lpy, 2016/04/06 23:51:37]

Done.

+  }
+}
+#endif  // !V8_OS_NACL
+#endif  // USE_SIGNALs
 
 
 //
 // StackTracer implementation
 //
 DISABLE_ASAN void TickSample::Init(Isolate* isolate,
                                    const v8::RegisterState& regs,
                                    RecordCEntryFrame record_c_entry_frame,
                                    bool update_stats) {
   timestamp = base::TimeTicks::HighResolutionNow();
@@ skipping 100 matching lines @@
   SignalHandler::TearDown();
 #endif
 }
 
 Sampler::Sampler(Isolate* isolate, int interval)
     : isolate_(isolate),
       interval_(interval),
       profiling_(false),
       has_processing_thread_(false),
       active_(false),
+      registered_(false),
       is_counting_samples_(false),
       js_sample_count_(0),
       external_sample_count_(0) {
   data_ = new PlatformData;
 }
 
 Sampler::~Sampler() {
   DCHECK(!IsActive());
+  if (IsRegistered())
+    SamplerThread::RemoveSampler(this);

[alph, 2016/04/06 22:21:26]

{}

[lpy, 2016/04/06 23:51:37]

Done.

   delete data_;
 }
 
 void Sampler::Start() {
   DCHECK(!IsActive());
   SetActive(true);
   SamplerThread::AddActiveSampler(this);
 }
 
 
 void Sampler::Stop() {
   DCHECK(IsActive());
-  SamplerThread::RemoveActiveSampler(this);
+  SamplerThread::RemoveSampler(this);
   SetActive(false);
+  SetRegistered(false);
 }
 
 
 void Sampler::IncreaseProfilingDepth() {
   base::NoBarrier_AtomicIncrement(&profiling_, 1);
 #if defined(USE_SIGNALS)
   SignalHandler::IncreaseSamplerCount();
 #endif
 }
 
@@ skipping 19 matching lines @@
   if (sample != &sample_obj) {
     isolate_->cpu_profiler()->FinishTickSample();
   }
 }
 
 
 #if defined(USE_SIGNALS)
 
 void Sampler::DoSample() {
   if (!SignalHandler::Installed()) return;
+  if (!IsActive() && !IsRegistered()) {
+    SamplerThread::RegisterSampler(this);
+    SetRegistered(true);
+  }
   pthread_kill(platform_data()->vm_tid(), SIGPROF);
 }
 
 #elif V8_OS_WIN || V8_OS_CYGWIN
 
 void Sampler::DoSample() {
   HANDLE profiled_thread = platform_data()->profiled_thread();
   if (profiled_thread == NULL) return;
 
 #if defined(USE_SIMULATOR)
@@ skipping 26 matching lines @@
     SampleStack(state);
   }
   ResumeThread(profiled_thread);
 }
 
 #endif  // USE_SIGNALS
 
 
 }  // namespace internal
 }  // namespace v8