Initial GetSample implementation.

src/sampler.cc

Index: src/sampler.cc
diff --git a/src/sampler.cc b/src/sampler.cc
index 0ed4b379626a9746038b6e05b1661cee522f4f1f..a339fbc404d6e2b9669f22cda9fdcde559c5617d 100644
--- a/src/sampler.cc
+++ b/src/sampler.cc
@@ -1,7 +1,6 @@
 // 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/sampler.h"
 
 #if V8_OS_POSIX && !V8_OS_CYGWIN
@@ -42,6 +41,8 @@
 
 #endif
 
+#include "include/v8-sampler.h"
+
 #include "src/v8.h"
 
 #include "src/base/platform/platform.h"
@@ -269,8 +270,16 @@ class SimulatorHelper {
 
 class SignalHandler : public AllStatic {
  public:
-  static void SetUp() { if (!mutex_) mutex_ = new base::Mutex(); }
-  static void TearDown() { delete mutex_; }
+  static void SetUp() {
+    if (!mutex_) mutex_ = new base::Mutex();
+    if (!sampling_semaphore_)
+      sampling_semaphore_ = new base::Semaphore(0);
+  }
+
+  static void TearDown() {
+    delete mutex_;
+    delete sampling_semaphore_;
+  }
 
   static void IncreaseSamplerCount() {
     base::LockGuard<base::Mutex> lock_guard(mutex_);
@@ -308,14 +317,45 @@ class SignalHandler : public AllStatic {
   }
 
   static void HandleProfilerSignal(int signal, siginfo_t* info, void* context);
+
+  // The SIGPROF could have come from DoSample or from GetSample.
+  // GetSample is the internal endpoint for the public
+  // GetSample API, which aims to bypass all the internal buffers
+  // and return just one sample instead, rightaway.
+  // TODO(gholap): Eventually, get rid of DoSample.
+  //               Only GetSample should remain.
+
+  // This is the sample which will be filled by the call from GetSample.
+  static TickSample sample_;
+
+  // For the SIGPROF handler to know whether the call was from GetSample.
+  static bool called_from_get_sample_;
+
+  // GetSample waits synchronously till the SIGPROF handler returns.
+  // this semaphore is used to signal GetSample.
+  static base::Semaphore* sampling_semaphore_;
+
+  // It is not that every time HandleProfilerSignal is invoked,
+  // it succeeds in obtaining a sample.
+  // This provides GetSample with the information whether
+  // the handler finished with or without getting the sample.
+  static bool sample_available_;
+
   // Protects the process wide state below.
   static base::Mutex* mutex_;
   static int client_count_;
   static bool signal_handler_installed_;
   static struct sigaction old_signal_handler_;
+
+  friend class Sampler;
 };
 
 
+TickSample SignalHandler::sample_;
+bool SignalHandler::called_from_get_sample_ = false;
+base::Semaphore* SignalHandler::sampling_semaphore_ = NULL;
+bool SignalHandler::sample_available_ = false;
+
 base::Mutex* SignalHandler::mutex_ = NULL;
 int SignalHandler::client_count_ = 0;
 struct sigaction SignalHandler::old_signal_handler_;
@@ -329,32 +369,46 @@ void SignalHandler::HandleProfilerSignal(int signal, siginfo_t* info,
   // is disabled.
   return;
 #else
+
+  // Even if we return from the signal handler without
+  // successfully getting a sampler, the caller of GetSample
+  // needs to know that the signal handler's work is finished.
+  // Otherwise, the caller will synchronously wait indefinitely.
+  // Also, the caller should have a way of knowing whether the
+  // signal handler successfully collected a sample or not.
+#define SIGNAL_AND_RETURN(GOT_NEW)              \
+  {                                             \
+    sample_available_ = GOT_NEW;                \
+    sampling_semaphore_->Signal();              \
+    return;                                     \

[Yang, 2014/08/12 14:21:36]

Using a scope, which signals in its destructor, seems a cleaner solution.

[gholap, 2014/08/13 21:09:21]

Done.

+  }
+
   USE(info);
-  if (signal != SIGPROF) return;
+  if (signal != SIGPROF) SIGNAL_AND_RETURN(false);
   Isolate* isolate = Isolate::UncheckedReentrantCurrent();
   if (isolate == NULL || !isolate->IsInitialized() || !isolate->IsInUse()) {
     // We require a fully initialized and entered isolate.
-    return;
+    SIGNAL_AND_RETURN(false);
   }
   if (v8::Locker::IsActive() &&
       !isolate->thread_manager()->IsLockedByCurrentThread()) {
-    return;
+    SIGNAL_AND_RETURN(false);
   }
 
   Sampler* sampler = isolate->logger()->sampler();
-  if (sampler == NULL) return;
+  if (sampler == NULL) SIGNAL_AND_RETURN(false);
 
   RegisterState state;
 
 #if defined(USE_SIMULATOR)
   SimulatorHelper helper;
-  if (!helper.Init(sampler, isolate)) return;
+  if (!helper.Init(sampler, isolate)) SIGNAL_AND_RETURN(false);
   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.
-  if (state.sp == 0 || state.fp == 0) return;
+  if (state.sp == 0 || state.fp == 0) SIGNAL_AND_RETURN(false);
 #else
   // Extracting the sample from the context is extremely machine dependent.
   ucontext_t* ucontext = reinterpret_cast<ucontext_t*>(context);
@@ -466,7 +520,13 @@ void SignalHandler::HandleProfilerSignal(int signal, siginfo_t* info,
 #endif  // V8_HOST_ARCH_*
 #endif  // V8_OS_QNX
 #endif  // USE_SIMULATOR
-  sampler->SampleStack(state);
+  if (called_from_get_sample_) {
+    sample_.Init(sampler->isolate(), state);
+  } else {
+    sampler->SampleStack(state);
+  }
+  SIGNAL_AND_RETURN(true);
+#undef SIGNAL_AND_RETURN
 #endif  // V8_OS_NACL
 }
 
@@ -693,19 +753,39 @@ void Sampler::DoSample() {
   pthread_kill(platform_data()->vm_tid(), SIGPROF);
 }
 
+
+TickSample* Sampler::GetSample(TickSample* sample) {
+  if (!SignalHandler::Installed()) return NULL;
+  SignalHandler::called_from_get_sample_ = true;

[Yang, 2014/08/12 14:21:36]

Don't we need a memory barrier here? Is it guaranteed that this value change is
observed by the signal handler?

[Benedikt Meurer, 2014/08/13 06:29:11]

At the very least, this has to be volatile.

[gholap, 2014/08/13 21:09:21]

Done.

[gholap, 2014/08/13 21:09:21]

Yes. Made it volatile.
This would just be a transient issue, though. Once the API is done, the only
thing that's going to send the signal is GetSample, eliminating the internal
code-path, and the need for this variable.

+  pthread_kill(platform_data()->vm_tid(), SIGPROF);
+  SignalHandler::sampling_semaphore_->Wait();
+  if (SignalHandler::sample_available_) {
+    sample->state = SignalHandler::sample_.state;

[Yang, 2014/08/12 14:21:36]

Why doesn't GetSample simply set a static pointer for the return value, and the
signal handler calls Init on that pointer? We would get away without copying
everything.

If we really have to copy, why not copy constructor?

[gholap, 2014/08/13 21:09:21]

Let's call the code that uses the API as "consumer".

GetSample takes a Sample* for which, the memory is allocated and owned by the
consumer.

We don't want to have an Init method in Sample class because it doesn't make
sense for the consumer. We don't want to mirror the Sample class internally with
an additional Init method because of the fragility issue.

Also, as memories for both TickSample and Sample that we are dealing with here
are pre-allocated, a copy constructor won't work either.

Moreover, as we flesh out the API and make it more and more comprehensive, we'll
eventually get rid of TickSample and the copying logic...

+    for (int i = 0; i < SignalHandler::sample_.frames_count; i++) {
+      sample->stack[i] = SignalHandler::sample_.stack[i];
+    }
+    sample->frames_count = SignalHandler::sample_.frames_count;
+  } else {
+    sample = NULL;
+  }
+  SignalHandler::called_from_get_sample_ = false;
+  return sample;
+}
+
 #elif V8_OS_WIN || V8_OS_CYGWIN
 
-void Sampler::DoSample() {
+TickSample* Sampler::GetSampleHelper_(TickSample * sample,

[Yang, 2014/08/12 14:21:36]

Please omit that _ in the method name.

[gholap, 2014/08/13 21:09:21]

Done.

+                                      bool called_from_get_sample) {
   HANDLE profiled_thread = platform_data()->profiled_thread();
-  if (profiled_thread == NULL) return;
+  if (profiled_thread == NULL) return NULL;
 
 #if defined(USE_SIMULATOR)
   SimulatorHelper helper;
-  if (!helper.Init(this, isolate())) return;
+  if (!helper.Init(this, isolate())) return NULL;
 #endif
 
   const DWORD kSuspendFailed = static_cast<DWORD>(-1);
-  if (SuspendThread(profiled_thread) == kSuspendFailed) return;
+  if (SuspendThread(profiled_thread) == kSuspendFailed) return NULL;
 
   // Context used for sampling the register state of the profiled thread.
   CONTEXT context;
@@ -726,9 +806,24 @@ void Sampler::DoSample() {
     state.fp = reinterpret_cast<Address>(context.Ebp);
 #endif
 #endif  // USE_SIMULATOR
-    SampleStack(state);
+    if (called_from_get_sample) {
+      sample->Init(sampler->isolate(), state);
+    } else {
+      SampleStack(state);
+    }
   }
   ResumeThread(profiled_thread);
+  return sample;
+}
+
+
+void Sampler::DoSample() {
+  GetSampleHelper_(NULL, false);
+}
+
+
+TickSample* Sampler::GetSample(TickSample* sample) {
+  return GetSampleHelper_(sample, true);
 }
 
 #endif  // USE_SIGNALS