V8 Sampling Profiler: Collect V8 JitCodeEvents in tracing

content/renderer/devtools/v8_sampling_profiler.cc

Index: content/renderer/devtools/v8_sampling_profiler.cc
diff --git a/content/renderer/devtools/v8_sampling_profiler.cc b/content/renderer/devtools/v8_sampling_profiler.cc
index 3aff56ba16f54c6d91f11a6d160e8f9d55721481..c387edc3fc967ffc872e1c9cdf2b9281a5c703cd 100644
--- a/content/renderer/devtools/v8_sampling_profiler.cc
+++ b/content/renderer/devtools/v8_sampling_profiler.cc
@@ -5,14 +5,154 @@
 #include "content/renderer/devtools/v8_sampling_profiler.h"
 
 #include "base/debug/trace_event.h"
+#include "base/debug/trace_event_argument.h"
+#include "base/strings/string_util.h"
 #include "base/synchronization/cancellation_flag.h"
 #include "base/threading/platform_thread.h"
+#include "content/renderer/render_thread_impl.h"
+#include "v8/include/v8.h"
+
+using v8::Isolate;
+using base::debug::TracedValue;
 
 namespace content {
 
+namespace {
+
+std::string PtrToString(const void* value) {
+  char buffer[20];
+  base::snprintf(buffer, sizeof(buffer), "%p", value);
+  return buffer;
+}
+
+// The class implements a sampler responsible for sampling a single thread.
+class Sampler {
+ public:
+  Sampler(base::PlatformThreadHandle handle, Isolate* isolate)
+      : handle_(handle), isolate_(isolate) {
+    DCHECK(isolate_);
+  }
+  virtual ~Sampler() {}
+
+  void Start();

[yurys, 2015/01/14 13:41:36]

Please add a comment that these methods can be called on different thread.

[alph, 2015/01/14 14:31:10]

Done.

+  void Stop();
+
+  void Sample();
+
+ private:
+  static void InstallJitCodeEventHandler(Isolate* isolate, void* data);
+  static void JitCodeEventHandler(const v8::JitCodeEvent* event);
+  static scoped_refptr<base::debug::ConvertableToTraceFormat>
+  JitCodeEventToTraceFormat(const v8::JitCodeEvent* event);

[yurys, 2015/01/14 13:41:37]

Wrong indentation.

[alph, 2015/01/14 14:31:10]

That's not me! That's git cl format

+
+  base::PlatformThreadHandle handle_;
+  Isolate* isolate_;
+};
+
+void Sampler::Start() {
+  v8::JitCodeEventHandler handler = &JitCodeEventHandler;
+  isolate_->RequestInterrupt(&InstallJitCodeEventHandler,
+                             reinterpret_cast<void*>(handler));
+}
+
+void Sampler::Stop() {
+  isolate_->RequestInterrupt(&InstallJitCodeEventHandler, nullptr);
+}
+
+void Sampler::Sample() {
+}
+
+// static
+void Sampler::InstallJitCodeEventHandler(Isolate* isolate, void* data) {
+  // Called on the sampled V8 thread.
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"),
+               "Sampler::InstallJitCodeEventHandler");
+  v8::JitCodeEventHandler handler =
+      reinterpret_cast<v8::JitCodeEventHandler>(data);
+  isolate->SetJitCodeEventHandler(
+      v8::JitCodeEventOptions::kJitCodeEventEnumExisting, handler);
+}
+
+// static
+void Sampler::JitCodeEventHandler(const v8::JitCodeEvent* event) {
+  // Called on the sampled V8 thread.
+  switch (event->type) {
+    case v8::JitCodeEvent::CODE_ADDED:
+      TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"),

[yurys, 2015/01/14 13:41:36]

How does this relate to the stack trace format described in [1]? Should we
reconsider the format for the JS samples and reflect it in that document?

Personally I like the approach when we write code events and stack samples as
instant events and do all the post-processing later on the front-end. This would
allow to drastically simplify the back-end as we wouldn't need to keep the code
tree there and could probably get rid of the additional processing thread. On
the other hand that would require two duplicate implementations of the processor
- one in DevTools front-end and another in the trace viewer. WDYT? In any case
the document [1] should describe actual format we want to implement. 

Also it is not quite clear how these events correspond to the first step
mentioned in the bug when we store whole profile in a single instant event.

[1]
https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...

[alph, 2015/01/14 14:31:10]

Yes, it's a different format. I'm not feeling comfortable to break into the
document. We could wait for Nat commenting on the topic.

The good point about the format introduced with this patch is that there's no in
fact a new format. Everything is described in the trace events sequence terms.
Just as close to the probed data as possible. The client may then do any
postprocessing it need, e.g. aggregate data, coalesce neighbor samples, etc.
I see the following pros/cons in the [1]:
  + it requires small amount of processing on the client side.
  - the format introduces new entities instead of plain array of trace events.
  - it aggregates data on samples, so it becomes impossible to draw profile in a
timeline (flamechart) way.
  - it requires backend side post-processing.
We decided not to go this way.

+                                   "JitCodeAdded", "data",
+                                   JitCodeEventToTraceFormat(event));
+      break;
+
+    case v8::JitCodeEvent::CODE_MOVED:
+      TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"),

[yurys, 2015/01/14 13:41:36]

Why is this metadata event rather than instant one?

[alph, 2015/01/14 14:31:10]

I tried not to garble the trace view with the samples.
But yes, if one has enabled the sampling category let him see the samples and
jit code events.

Turned them back into instant events.

+                                   "JitCodeMoved", "data",
+                                   JitCodeEventToTraceFormat(event));
+      break;
+
+    case v8::JitCodeEvent::CODE_REMOVED:
+      TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"),
+                                   "JitCodeRemoved", "data",
+                                   JitCodeEventToTraceFormat(event));
+      break;
+
+    case v8::JitCodeEvent::CODE_ADD_LINE_POS_INFO:
+    case v8::JitCodeEvent::CODE_START_LINE_INFO_RECORDING:
+    case v8::JitCodeEvent::CODE_END_LINE_INFO_RECORDING:
+      break;
+  }
+}
+
+// static
+scoped_refptr<base::debug::ConvertableToTraceFormat>
+Sampler::JitCodeEventToTraceFormat(const v8::JitCodeEvent* event) {
+  // Called on the sampled thread.
+  switch (event->type) {
+    case v8::JitCodeEvent::CODE_ADDED: {
+      scoped_refptr<TracedValue> data(new TracedValue());
+      data->SetString("code_start", PtrToString(event->code_start));

[yurys, 2015/01/14 13:41:36]

Do we want to see isolate identifier as one of the properties?

[alph, 2015/01/14 14:31:10]

It is not necessary. The code_start uniquely identifies code objects. We may add
Isolate later if needed, but I assume it will just be the same for all events on
the thread.

+      data->SetInteger("code_len", static_cast<unsigned>(event->code_len));
+      data->SetString("name", std::string(event->name.str, event->name.len));
+      return data;
+    }
+
+    case v8::JitCodeEvent::CODE_MOVED: {
+      scoped_refptr<TracedValue> data(new TracedValue());
+      data->SetString("code_start", PtrToString(event->code_start));
+      data->SetInteger("code_len", static_cast<unsigned>(event->code_len));
+      data->SetString("new_code_start", PtrToString(event->new_code_start));
+      return data;
+    }
+
+    case v8::JitCodeEvent::CODE_REMOVED: {
+      scoped_refptr<TracedValue> data(new TracedValue());
+      data->SetString("code_start", PtrToString(event->code_start));
+      data->SetInteger("code_len", static_cast<unsigned>(event->code_len));
+      return data;
+    }
+
+    case v8::JitCodeEvent::CODE_ADD_LINE_POS_INFO:
+    case v8::JitCodeEvent::CODE_START_LINE_INFO_RECORDING:
+    case v8::JitCodeEvent::CODE_END_LINE_INFO_RECORDING:
+      return nullptr;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
+// Main thread sampler. Must be created on the main thread.
+class RenderThreadSampler : public Sampler {
+ public:
+  RenderThreadSampler()

[yurys, 2015/01/14 13:41:36]

Why do we need a subclass here? Wouldn't Sampler::createForCurrentThread() work
?

[alph, 2015/01/14 14:31:10]

Done.

+      : Sampler(base::PlatformThread::CurrentHandle(), Isolate::GetCurrent()) {
+    // Must be called on the render thread.
+    DCHECK(RenderThreadImpl::current());
+  }
+};
+
 class V8SamplingThread : public base::PlatformThread::Delegate {
  public:
-  explicit V8SamplingThread(base::WaitableEvent* event);
+  V8SamplingThread(RenderThreadSampler*, base::WaitableEvent*);
 
   // Implementation of PlatformThread::Delegate:
   void ThreadMain() override;
@@ -21,27 +161,71 @@ class V8SamplingThread : public base::PlatformThread::Delegate {
   void Stop();
 
  private:
+  void Sample();
+  void InstallSamplers();
+  void RemoveSamplers();
+  void StartSamplers();
+  void StopSamplers();
+  static void JitCodeEventHandler(const v8::JitCodeEvent* event);
+
+  RenderThreadSampler* render_thread_sampler_;
   base::CancellationFlag cancellation_flag_;
   base::WaitableEvent* waitable_event_for_testing_;
   base::PlatformThreadHandle sampling_thread_handle_;
+  std::vector<Sampler*> samplers_;
 
   DISALLOW_COPY_AND_ASSIGN(V8SamplingThread);
 };
 
-V8SamplingThread::V8SamplingThread(base::WaitableEvent* event)
-    : waitable_event_for_testing_(event) {
+V8SamplingThread::V8SamplingThread(RenderThreadSampler* render_thread_sampler,
+                                   base::WaitableEvent* event)
+    : render_thread_sampler_(render_thread_sampler),

[yurys, 2015/01/14 13:41:37]

May be pass an array of all samplers?

[alph, 2015/01/14 14:31:10]

I'm not yet sure who will be responsible for creating samplers for workers. Let
me update on that later.

+      waitable_event_for_testing_(event) {
 }
 
 void V8SamplingThread::ThreadMain() {
-  base::PlatformThread::SetName("V8 Sampling Profiler Thread");
+  base::PlatformThread::SetName("V8SamplingProfilerThread");
+  InstallSamplers();
+  StartSamplers();
   const int kSamplingFrequencyMicroseconds = 1000;
   while (!cancellation_flag_.IsSet()) {
+    Sample();
     if (waitable_event_for_testing_) {
       waitable_event_for_testing_->Signal();
     }
     base::PlatformThread::Sleep(
         base::TimeDelta::FromMicroseconds(kSamplingFrequencyMicroseconds));
   }
+  StopSamplers();
+  RemoveSamplers();
+}
+
+void V8SamplingThread::Sample() {
+  for (auto sampler : samplers_) {

[yurys, 2015/01/14 13:41:36]

auto -> Sampler* here and below

[alph, 2015/01/14 14:31:10]

Done.

+    sampler->Sample();
+  }
+}
+
+void V8SamplingThread::InstallSamplers() {
+  // Note that the list does not own samplers.
+  samplers_.push_back(render_thread_sampler_);
+  // TODO: add worker samplers.
+}
+
+void V8SamplingThread::RemoveSamplers() {
+  samplers_.clear();
+}
+
+void V8SamplingThread::StartSamplers() {
+  for (auto sampler : samplers_) {
+    sampler->Start();
+  }
+}
+
+void V8SamplingThread::StopSamplers() {
+  for (auto sampler : samplers_) {
+    sampler->Stop();
+  }
 }
 
 void V8SamplingThread::Start() {
@@ -55,7 +239,9 @@ void V8SamplingThread::Stop() {
   base::PlatformThread::Join(sampling_thread_handle_);
 }
 
-V8SamplingProfiler::V8SamplingProfiler() : sampling_thread_(nullptr) {
+V8SamplingProfiler::V8SamplingProfiler()
+    : sampling_thread_(nullptr),
+      render_thread_sampler_(new RenderThreadSampler()) {
   // Force the "v8_cpu_profile" category to show up in the trace viewer.
   base::debug::TraceLog::GetCategoryGroupEnabled(
       TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"));
@@ -74,8 +260,8 @@ void V8SamplingProfiler::OnTraceLogEnabled() {
   if (!enabled)
     return;
   DCHECK(!sampling_thread_.get());
-  sampling_thread_.reset(
-      new V8SamplingThread(waitable_event_for_testing_.get()));
+  sampling_thread_.reset(new V8SamplingThread(
+      render_thread_sampler_.get(), waitable_event_for_testing_.get()));
   sampling_thread_->Start();
 }