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

base/profiler/native_stack_sampler.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_PROFILER_NATIVE_STACK_SAMPLER_H_
 #define BASE_PROFILER_NATIVE_STACK_SAMPLER_H_
 
 #include <memory>
 
 #include "base/base_export.h"
 #include "base/macros.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "base/threading/platform_thread.h"
 
 namespace base {
 
 class NativeStackSamplerTestDelegate;
 
 // NativeStackSampler is an implementation detail of StackSamplingProfiler. It
 // abstracts the native implementation required to record a stack sample for a
 // 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
   // completely predictable and do nothing that could acquire a mutex; a
   // Callback object is code outside the control of this object and could,
   // for example, acquire a mutex as part of allocating memory for a LOG
   // message.
   using AnnotateCallback = void (*)(StackSamplingProfiler::Sample*);
 
   virtual ~NativeStackSampler();
 
   // Creates a stack sampler that records samples for |thread_handle|. Returns
   // null if this platform does not support stack sampling.
   static std::unique_ptr<NativeStackSampler> Create(
       PlatformThreadId thread_id,
       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).
 
   // Notifies the sampler that we're starting to record a new profile. Modules
   // shared across samples in the profile should be recorded in |modules|.
   virtual void ProfileRecordingStarting(
       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();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NativeStackSampler);
 };
 
 // NativeStackSamplerTestDelegate provides seams for test code to execute during
 // stack collection.
 class BASE_EXPORT NativeStackSamplerTestDelegate {
  public:
   virtual ~NativeStackSamplerTestDelegate();
 
   // Called after copying the stack and resuming the target thread, but prior to
   // walking the stack. Invoked on the SamplingThread.
   virtual void OnPreStackWalk() = 0;
 
  protected:
   NativeStackSamplerTestDelegate();
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NativeStackSamplerTestDelegate);
 };
 
 }  // namespace base
 
 #endif  // BASE_PROFILER_NATIVE_STACK_SAMPLER_H_