Add process lifetime annotations to stack samples.

base/profiler/native_stack_sampler_win.cc

Index: base/profiler/native_stack_sampler_win.cc
diff --git a/base/profiler/native_stack_sampler_win.cc b/base/profiler/native_stack_sampler_win.cc
index 063374f19d5b1edfd7d758d98e4047a1b8d67bd6..1a3f2520d6be1d9986d932b923cca9ecf211f5de 100644
--- a/base/profiler/native_stack_sampler_win.cc
+++ b/base/profiler/native_stack_sampler_win.cc
@@ -15,6 +15,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ -319,6 +320,7 @@ void SuspendThreadAndRecordStack(
     void* stack_copy_buffer,
     size_t stack_copy_buffer_size,
     std::vector<RecordedFrame>* stack,
+    Closure annotator,
     NativeStackSamplerTestDelegate* test_delegate) {
   DCHECK(stack->empty());
 
@@ -353,6 +355,8 @@ void SuspendThreadAndRecordStack(
     if (PointsToGuardPage(bottom))
       return;
 
+    annotator.Run();

[Mike Wittman, 2016/10/26 16:28:17]

Invoking Callback::Run while the thread is suspended is risky. If the
implementation of Run ever allocates memory, including indirectly via things
like LOG/CHECK/DCHECK, it risks deadlock. I don't know if this is the case now,
but even if not, we shouldn't depend on it for correctness.

I suggest passing a raw function pointer instead. Something like:

using GetAnnotationCallback = void(*)(uint32_t*, uint32_t*);

...

uint32_t phases = 0
uint32_t activities = 0;
get_annotation(&phases, &activities);

...

RecordStack(&thread_context, stack, phases, activities);

[bcwhite, 2016/10/26 18:30:43]

Good point.  We can document this code to say what isn't allowed but we can't
control what the intermediate code may do.

This section of the code doesn't know anything about the Sample instance which
is where those annotations are recorded.  I could pass it and use that as the
parameter to the function-pointer so the called function knows where to write
the fields.  But...

Right now the context is global so if we're not using official "callback"
objects and have to pass in an opaque "sample" pointer just for a parameter to
the function pointer, then we might as well skip the function pointer altogether
and just call the static method.

  StackSamplingProfiler::RecordAnnotations(sample);

[Mike Wittman, 2016/10/26 19:41:14]

That would work, but it would couple the implementation of this class and
StackSamplingProfiler, which I'd rather not do. That would make it difficult to
test this class in isolation, and we may want to do that as we add more
complexity to StackSamplingProfiler.

If the callback is solely responsible for returning the current phases and
activities, rather than copying them to the Sample, then two values could be
recorded here and threaded through to CopyToSample, which could assign them to
the Sample. In that case there would be no need to pass a Sample pointer.

[bcwhite, 2016/10/26 20:02:10]

It might be possible to just make it testable via static methods on the
StackSamplingProfiler... but global state could be a problem if tests were to
run in parallel.  <sigh>  Okay, scratch that idea.
Yes, but I don't think that's a good idea.  I think annotations should be solely
the domain of the StackSamplingProfiler; we should avoid having the Native part
know anything about them.  We don't want future changes to in the former to
require changes to the latter.  Thus, an opaque sample* seems the best way to
me.

[Mike Wittman, 2016/10/26 20:20:36]

That's reasonable, although I feel like this may be less of a concern once the
details of the phases and activity state are moved to components/metrics and the
StackSamplingProfiler is no longer dealing with them. Perhaps it's worth leaving
this question until then.

I guess another option would be to treat the entire annotation state as an
opaque blob of bits (e.g. std::bitset<64>) at the Native level, where it just
knows how to copy the bits around, and the interpretation of the bits is done at
a higher level.

+
     std::memcpy(stack_copy_buffer, reinterpret_cast<const void*>(bottom),
                 top - bottom);
   }
@@ -370,6 +374,7 @@ void SuspendThreadAndRecordStack(
 class NativeStackSamplerWin : public NativeStackSampler {
  public:
   NativeStackSamplerWin(win::ScopedHandle thread_handle,
+                        StackSamplingProfiler::AnnotateCallback annotator,
                         NativeStackSamplerTestDelegate* test_delegate);
   ~NativeStackSamplerWin() override;
 
@@ -408,6 +413,8 @@ class NativeStackSamplerWin : public NativeStackSampler {
 
   win::ScopedHandle thread_handle_;
 
+  StackSamplingProfiler::AnnotateCallback annotator_;
+
   NativeStackSamplerTestDelegate* const test_delegate_;
 
   // The stack base address corresponding to |thread_handle_|.
@@ -430,8 +437,11 @@ class NativeStackSamplerWin : public NativeStackSampler {
 
 NativeStackSamplerWin::NativeStackSamplerWin(
     win::ScopedHandle thread_handle,
+    StackSamplingProfiler::AnnotateCallback annotator,
     NativeStackSamplerTestDelegate* test_delegate)
-    : thread_handle_(thread_handle.Take()), test_delegate_(test_delegate),
+    : thread_handle_(thread_handle.Take()),
+      annotator_(annotator),
+      test_delegate_(test_delegate),
       thread_stack_base_address_(
           GetThreadEnvironmentBlock(thread_handle_.Get())->Tib.StackBase),
       stack_copy_buffer_(new unsigned char[kStackCopyBufferSize]) {
@@ -456,7 +466,7 @@ void NativeStackSamplerWin::RecordStackSample(
   std::vector<RecordedFrame> stack;
   SuspendThreadAndRecordStack(thread_handle_.Get(), thread_stack_base_address_,
                               stack_copy_buffer_.get(), kStackCopyBufferSize,
-                              &stack, test_delegate_);
+                              &stack, Bind(annotator_, sample), test_delegate_);
   CopyToSample(stack, sample, current_modules_);
 }
 
@@ -508,11 +518,11 @@ void NativeStackSamplerWin::CopyToSample(
     const std::vector<RecordedFrame>& stack,
     StackSamplingProfiler::Sample* sample,
     std::vector<StackSamplingProfiler::Module>* modules) {
-  sample->clear();
-  sample->reserve(stack.size());
+  sample->frames.clear();
+  sample->frames.reserve(stack.size());
 
   for (const RecordedFrame& frame : stack) {
-    sample->push_back(StackSamplingProfiler::Frame(
+    sample->frames.push_back(StackSamplingProfiler::Frame(
         reinterpret_cast<uintptr_t>(frame.instruction_pointer),
         GetModuleIndex(frame.module.Get(), modules)));
   }
@@ -522,6 +532,7 @@ void NativeStackSamplerWin::CopyToSample(
 
 std::unique_ptr<NativeStackSampler> NativeStackSampler::Create(
     PlatformThreadId thread_id,
+    StackSamplingProfiler::AnnotateCallback annotator,
     NativeStackSamplerTestDelegate* test_delegate) {
 #if _WIN64
   // Get the thread's handle.
@@ -532,7 +543,7 @@ std::unique_ptr<NativeStackSampler> NativeStackSampler::Create(
 
   if (thread_handle) {
     return std::unique_ptr<NativeStackSampler>(new NativeStackSamplerWin(
-        win::ScopedHandle(thread_handle), test_delegate));
+        win::ScopedHandle(thread_handle), annotator, test_delegate));
   }
 #endif
   return std::unique_ptr<NativeStackSampler>();