Use a common buffer across all instances for stack-copy.

base/profiler/native_stack_sampler.h

Index: base/profiler/native_stack_sampler.h
diff --git a/base/profiler/native_stack_sampler.h b/base/profiler/native_stack_sampler.h
index 8d7e441b69306b072ed67321a531ac76aee4e28c..c7cfbb34f2a17bdc186f56d95e0aa04286f33c82 100644
--- a/base/profiler/native_stack_sampler.h
+++ b/base/profiler/native_stack_sampler.h
@@ -21,6 +21,21 @@ class NativeStackSamplerTestDelegate;
 // given thread.
 class NativeStackSampler {
  public:
+  // This class contains data structures that can be shared across multiple
+  // instances of NativeStackSampler so long as they do not run concurrently.
+  // This supports having a single instance of large structures such as copy
+  // buffers.
+  class StackBuffer {

[Mike Wittman, 2017/05/01 20:28:03]

Following from the comment below, this can be simplified to directly hold a
void* buffer and size_t size fields, with no need to subclass.

[bcwhite, 2017/05/08 14:00:59]

If you want the generic base class header file to invoke knowledge about what is
derived from it in private .cc files and assume that it will always be the same
for future platforms, then yes.

Is that what you want?

[Mike Wittman, 2017/05/09 01:48:05]

The abstraction should cover exactly the known subclass behavior. Trying to
broaden the abstraction to account for the unknown requirements of unknown
additional platforms just introduces unnecessary complexity, making both the
code and the underlying problem being solved harder to understand. This is no
more or less than an application of the YAGNI principle.

Why do you say this assumes the behavior will always be the same for future
platforms? Internal interfaces are as mutable as any other code in Chrome.
There's nothing stopping anyone from generalizing it, if and when a specific
need is identified.

[bcwhite, 2017/05/09 18:33:33]

Done.

+   public:
+    virtual ~StackBuffer() {}
+
+   protected:
+    StackBuffer() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(StackBuffer);
+  };
+
   // The callback type used to add annotations to a sample during collection.
   // This is passed to the native sampler to be applied at the most appropriate
   // time. It is a simple function-pointer because the generated code must be
@@ -39,6 +54,9 @@ class NativeStackSampler {
       AnnotateCallback annotator,
       NativeStackSamplerTestDelegate* test_delegate);
 
+  // Creates an instance of the necessary stack buffer.
+  static std::unique_ptr<StackBuffer> CreateStackBuffer();

[Mike Wittman, 2017/05/01 20:28:03]

From
https://codereview.chromium.org/2848683006/diff/20001/base/profiler/native_st...
the only variation between platforms is the size of the buffer. So this can be
simplified to:
  static size_t GetStackBufferSize();
and the buffer allocation moved into StackSamplingProfiler.

[Mike Wittman, 2017/05/10 01:29:56]

Please address this comment also.

[bcwhite, 2017/05/10 13:50:37]

Done.

[Mike Wittman, 2017/05/10 17:17:21]

I don't see this change in patch set 10.

[bcwhite, 2017/05/11 16:56:31]

Done.

+
   // The following functions are all called on the SamplingThread (not the
   // thread being sampled).
 
@@ -48,11 +66,12 @@ class NativeStackSampler {
       std::vector<StackSamplingProfiler::Module>* modules) = 0;
 
   // Records a stack sample to |sample|.
-  virtual void RecordStackSample(StackSamplingProfiler::Sample* sample) = 0;
+  virtual void RecordStackSample(StackBuffer* stackbuffer,
+                                 StackSamplingProfiler::Sample* sample) = 0;
 
   // Notifies the sampler that we've stopped recording the current
   // profile.
-  virtual void ProfileRecordingStopped() = 0;
+  virtual void ProfileRecordingStopped(StackBuffer* stackbuffer) = 0;
 
  protected:
   NativeStackSampler();