Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601633002:
Use a common buffer across all instances for stack-copy. (Closed)
|
Created:
3 years, 12 months ago by bcwhite Modified:
3 years, 7 months ago Reviewers:
Mike Wittman CC:
chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse a common buffer across all instances for stack-copy.
BUG=671716
Review-Url: https://codereview.chromium.org/2601633002
Cr-Commit-Position: refs/heads/master@{#471294}
Committed: https://chromium.googlesource.com/chromium/src/+/1a4d8f0a44795c09c519f9c96fec431793eb5837
Patch Set 1 #Patch Set 2 : be tolerant of in-use on a production build #Patch Set 3 : change from shared static buffer to caller-owned 'common' buffer #
Total comments: 20
Patch Set 4 : common -> stackbuffer and other review changes #Patch Set 5 : attached to new SamplingThread #Patch Set 6 : rebased #
Total comments: 13
Patch Set 7 : addressed review comments by wittman #Patch Set 8 : rebased #Patch Set 9 : use common StackBuffer; update Mac native sampler #
Total comments: 10
Patch Set 10 : make StackBuffer word aligned #
Total comments: 8
Patch Set 11 : addressed review comments by wittman #
Total comments: 2
Patch Set 12 : addressed review comments by wittman #
Messages
Total messages: 128 (107 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
bcwhite@chromium.org changed reviewers: + wittman@chromium.org
Keeping the creating of the buffer here is reasonable since the sizing is potentially platform-specific. But it seems like having the SamplingThread own the buffer (and pass to RecordStackSample) would avoid the need for reference counting and in-use checking. Can we do that?
On 2017/01/18 17:02:53, Mike Wittman wrote: > Keeping the creating of the buffer here is reasonable since the sizing is > potentially platform-specific. But it seems like having the SamplingThread own > the buffer (and pass to RecordStackSample) would avoid the need for reference > counting and in-use checking. Can we do that? I think it best to keep the working of the Native sampler inside that class. It's cleaner and doesn't require assumptions or extra knowledge by the generic sampler that may not be applicable should this ever extend to other operating systems. The in-use checking isn't necessary. I think it can be removed later on but for now seems a good secondary check.
On 2017/01/18 18:53:06, bcwhite wrote: > On 2017/01/18 17:02:53, Mike Wittman wrote: > > Keeping the creating of the buffer here is reasonable since the sizing is > > potentially platform-specific. But it seems like having the SamplingThread own > > the buffer (and pass to RecordStackSample) would avoid the need for reference > > counting and in-use checking. Can we do that? > > I think it best to keep the working of the Native sampler inside that class. > It's cleaner and doesn't require assumptions or extra knowledge by the generic > sampler that may not be applicable should this ever extend to other operating > systems. This does require extra assumptions/knowledge at the generic sampler level though -- it adds the implicit requirement that RecordStackSample may not be invoked on multiple NativeStackSampler instances at the same time. If the user must observe this non-intuitive constraint, then it's better to encode it formally in the interface, placing the responsibility for managing the shared resource on the user. This will work better for supporting other operating systems too. Mac will probably use the same strategy as Windows. But Linux/Android will likely require significant refactoring to the interface, which is easier done with clear, explicit ownership relationships and constraints expressed explicitly in the interface. Devolving explicit interface constraints to a lower level later is easier than reconstructing implicit constraints from implementation. Generally, reference counting should be used as a solution of last resort because it makes reasoning about object lifetimes much more difficult. > The in-use checking isn't necessary. I think it can be removed later on but for > now seems a good secondary check. I'd prefer to construct the code to make it obvious this situation will not occur, so the check can be avoided entirely.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
On 2017/01/19 16:15:38, Mike Wittman wrote: > On 2017/01/18 18:53:06, bcwhite wrote: > > On 2017/01/18 17:02:53, Mike Wittman wrote: > > > Keeping the creating of the buffer here is reasonable since the sizing is > > > potentially platform-specific. But it seems like having the SamplingThread > own > > > the buffer (and pass to RecordStackSample) would avoid the need for > reference > > > counting and in-use checking. Can we do that? > > > > I think it best to keep the working of the Native sampler inside that class. > > It's cleaner and doesn't require assumptions or extra knowledge by the generic > > sampler that may not be applicable should this ever extend to other operating > > systems. > > This does require extra assumptions/knowledge at the generic sampler level > though -- it adds the implicit requirement that RecordStackSample may not be > invoked on multiple NativeStackSampler instances at the same time. If the user > must observe this non-intuitive constraint, then it's better to encode it > formally in the interface, placing the responsibility for managing the shared > resource on the user. > > This will work better for supporting other operating systems too. Mac will > probably use the same strategy as Windows. But Linux/Android will likely require > significant refactoring to the interface, which is easier done with clear, > explicit ownership relationships and constraints expressed explicitly in the > interface. Devolving explicit interface constraints to a lower level later is > easier than reconstructing implicit constraints from implementation. > > Generally, reference counting should be used as a solution of last resort > because it makes reasoning about object lifetimes much more difficult. > > > The in-use checking isn't necessary. I think it can be removed later on but > for > > now seems a good secondary check. > > I'd prefer to construct the code to make it obvious this situation will not > occur, so the check can be avoided entirely. How's this?
On 2017/01/24 15:29:12, bcwhite wrote: > How's this? Looking good, thanks. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class Common { We should just call this StackBuffer. If the concept needs to be generalized in the future the name can be updated then. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:34: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: virtual std::unique_ptr<Common> CreateCommon(); This function doesn't depend on the instance so should be static. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:63: Common* common, No need to pass this to ProfileRecordingStarting or ProfileRecordingStopped. If someone wants this at some later point it can be added then. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:67: virtual void RecordStackSample(Common* common, Doesn't the calling code need to be updated? https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:18: #include "base/atomicops.h" No longer needed. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:27: #include "base/synchronization/lock.h" No longer needed. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:416: class WinCommon : public Common { WinCommon => CommonWin (or actually StackBufferWin per previous comment) Convention in Chrome is to use the platform specific name as suffix. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:462: static subtle::AtomicWord stack_copy_buffer_in_use_; No longer needed. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:503: reinterpret_cast<WinCommon*>(common)->stack_copy_buffer_, static_cast for downcast
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #6 (id:200001) has been deleted
https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class Common { On 2017/01/24 17:20:56, Mike Wittman wrote: > We should just call this StackBuffer. If the concept needs to be generalized in > the future the name can be updated then. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:34: }; On 2017/01/24 17:20:56, Mike Wittman wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: virtual std::unique_ptr<Common> CreateCommon(); On 2017/01/24 17:20:56, Mike Wittman wrote: > This function doesn't depend on the instance so should be static. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:63: Common* common, On 2017/01/24 17:20:57, Mike Wittman wrote: > No need to pass this to ProfileRecordingStarting or ProfileRecordingStopped. If > someone wants this at some later point it can be added then. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler.h:67: virtual void RecordStackSample(Common* common, On 2017/01/24 17:20:57, Mike Wittman wrote: > Doesn't the calling code need to be updated? Yes but since the calling code is in the middle of a rewrite to support multi-sampling, I'm waiting to do that until after that CL lands. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:18: #include "base/atomicops.h" On 2017/01/24 17:20:57, Mike Wittman wrote: > No longer needed. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:27: #include "base/synchronization/lock.h" On 2017/01/24 17:20:57, Mike Wittman wrote: > No longer needed. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:416: class WinCommon : public Common { On 2017/01/24 17:20:57, Mike Wittman wrote: > WinCommon => CommonWin (or actually StackBufferWin per previous comment) > > Convention in Chrome is to use the platform specific name as suffix. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:462: static subtle::AtomicWord stack_copy_buffer_in_use_; On 2017/01/24 17:20:57, Mike Wittman wrote: > No longer needed. Done. https://codereview.chromium.org/2601633002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:503: reinterpret_cast<WinCommon*>(common)->stack_copy_buffer_, On 2017/01/24 17:20:57, Mike Wittman wrote: > static_cast for downcast Done.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #7 (id:240001) has been deleted
Patchset #5 (id:180001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #5 (id:260001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It looks like the multi-sample is going to stick so shall be finish this one?
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/01 14:52:08, bcwhite wrote: > It looks like the multi-sample is going to stick so shall be finish this one? Yes, sgtm. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class StackBuffer { 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. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: static std::unique_ptr<StackBuffer> CreateStackBuffer(); 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. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:253: // A stack-buffer used by the native sampler for its work. Mention that this is shared across all active samplers. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:254: std::unique_ptr<NativeStackSampler::StackBuffer> native_buffer_; stack_buffer_ is probably a better name.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class StackBuffer { On 2017/05/01 20:28:03, Mike Wittman wrote: > 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. 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? https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:253: // A stack-buffer used by the native sampler for its work. On 2017/05/01 20:28:04, Mike Wittman wrote: > Mention that this is shared across all active samplers. Done. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:254: std::unique_ptr<NativeStackSampler::StackBuffer> native_buffer_; On 2017/05/01 20:28:03, Mike Wittman wrote: > stack_buffer_ is probably a better name. Done.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The Mac profiler CL was relanded last week and looks to be sticking, so this change needs to handle Mac too. https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class StackBuffer { On 2017/05/08 14:00:59, bcwhite wrote: > On 2017/05/01 20:28:03, Mike Wittman wrote: > > 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. > > 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? 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.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:360001) has been deleted
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:28: class StackBuffer { On 2017/05/09 01:48:05, Mike Wittman wrote: > On 2017/05/08 14:00:59, bcwhite wrote: > > On 2017/05/01 20:28:03, Mike Wittman wrote: > > > 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. > > > > 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? > > 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. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: static std::unique_ptr<StackBuffer> CreateStackBuffer(); On 2017/05/01 20:28:03, Mike Wittman wrote: > 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. Please address this comment also. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.cc (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.cc:10: : buffer_(new unsigned char[buffer_size]), size_(buffer_size) {} We should fix this for alignment while we're here, since the pointer returned by "new unsigned char[buffer_size]" is only guaranteed to be one byte aligned. The value returned by buffer() should be word aligned to avoid the performance penalty of unaligned reads/writes on x64. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:27: // buffers. For this comment I think it's sufficient to say just: Holds a memory buffer for stack copies that may be shared across multiple NativeStackSamplers. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:33: unsigned char* buffer() { return buffer_.get(); } void* for return type. There's no natural size for the contents of this buffer, and we shouldn't impose one arbitrarily. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:73: virtual void RecordStackSample(StackBuffer* stackbuffer, nit: stack_buffer (applies throughout)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: static std::unique_ptr<StackBuffer> CreateStackBuffer(); On 2017/05/10 01:29:56, Mike Wittman wrote: > On 2017/05/01 20:28:03, Mike Wittman wrote: > > 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. > > Please address this comment also. Done. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.cc (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.cc:10: : buffer_(new unsigned char[buffer_size]), size_(buffer_size) {} On 2017/05/10 01:29:56, Mike Wittman wrote: > We should fix this for alignment while we're here, since the pointer returned by > "new unsigned char[buffer_size]" is only guaranteed to be one byte aligned. The > value returned by buffer() should be word aligned to avoid the performance > penalty of unaligned reads/writes on x64. Done. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:27: // buffers. On 2017/05/10 01:29:56, Mike Wittman wrote: > For this comment I think it's sufficient to say just: > > Holds a memory buffer for stack copies that may be shared across multiple > NativeStackSamplers. The "no concurrent" clause is important. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:33: unsigned char* buffer() { return buffer_.get(); } On 2017/05/10 01:29:57, Mike Wittman wrote: > void* for return type. There's no natural size for the contents of this buffer, > and we shouldn't impose one arbitrarily. Done. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:73: virtual void RecordStackSample(StackBuffer* stackbuffer, On 2017/05/10 01:29:57, Mike Wittman wrote: > nit: stack_buffer (applies throughout) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: static std::unique_ptr<StackBuffer> CreateStackBuffer(); On 2017/05/10 13:50:37, bcwhite wrote: > On 2017/05/10 01:29:56, Mike Wittman wrote: > > On 2017/05/01 20:28:03, Mike Wittman wrote: > > > 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. > > > > Please address this comment also. > > Done. I don't see this change in patch set 10. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:27: // buffers. On 2017/05/10 13:50:37, bcwhite wrote: > On 2017/05/10 01:29:56, Mike Wittman wrote: > > For this comment I think it's sufficient to say just: > > > > Holds a memory buffer for stack copies that may be shared across multiple > > NativeStackSamplers. > > The "no concurrent" clause is important. "It's not safe to share a writable memory buffer across threads without synchronization" is something we can expect a competent developer to know and not worth restating in a comment. This is more likely to confuse than be helpful because readers will either wonder why the comment is even there, or assume it's giving something other than basic programming advice and seek to interpret "concurrently" in some incorrect manner (for example: referring to the existing interleaving of sample collection across multiple target threads). It's also unlikely that concurrent profiling will ever be needed, so whether it's safe to use the buffer concurrently is not something readers need to care about. (Applies to all comments warning about concurrency.) https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler.h:31: void* buffer() { return buffer_.get(); } nit: make both accessors const https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler.h:36: const std::unique_ptr<int[]> buffer_; uintptr_t for 8 byte words on x86_64 https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:395: static constexpr size_t kStackCopyBufferSize = 2 * 1024 * 1024; This can be moved back into the private section.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #11 (id:420001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:440001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #11 (id:460001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/300001/base/profiler/native_s... base/profiler/native_stack_sampler.h:58: static std::unique_ptr<StackBuffer> CreateStackBuffer(); On 2017/05/10 17:17:21, Mike Wittman wrote: > On 2017/05/10 13:50:37, bcwhite wrote: > > On 2017/05/10 01:29:56, Mike Wittman wrote: > > > On 2017/05/01 20:28:03, Mike Wittman wrote: > > > > 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. > > > > > > Please address this comment also. > > > > Done. > > I don't see this change in patch set 10. Done. https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler.h:27: // buffers. On 2017/05/10 17:17:21, Mike Wittman wrote: > On 2017/05/10 13:50:37, bcwhite wrote: > > On 2017/05/10 01:29:56, Mike Wittman wrote: > > > For this comment I think it's sufficient to say just: > > > > > > Holds a memory buffer for stack copies that may be shared across multiple > > > NativeStackSamplers. > > > > The "no concurrent" clause is important. > > "It's not safe to share a writable memory buffer across threads without > synchronization" is something we can expect a competent developer to know and > not worth restating in a comment. This is more likely to confuse than be helpful > because readers will either wonder why the comment is even there, or assume it's > giving something other than basic programming advice and seek to interpret > "concurrently" in some incorrect manner (for example: referring to the existing > interleaving of sample collection across multiple target threads). > > It's also unlikely that concurrent profiling will ever be needed, so whether > it's safe to use the buffer concurrently is not something readers need to care > about. > > (Applies to all comments warning about concurrency.) Done. https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler.h:31: void* buffer() { return buffer_.get(); } On 2017/05/10 17:17:21, Mike Wittman wrote: > nit: make both accessors const Done. https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler.h:36: const std::unique_ptr<int[]> buffer_; On 2017/05/10 17:17:21, Mike Wittman wrote: > uintptr_t for 8 byte words on x86_64 Done. https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:395: static constexpr size_t kStackCopyBufferSize = 2 * 1024 * 1024; On 2017/05/10 17:17:21, Mike Wittman wrote: > This can be moved back into the private section. It has to be public to be accessible to NativeStackSampler::GetStackBufferSize().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with mentioned changes https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:395: static constexpr size_t kStackCopyBufferSize = 2 * 1024 * 1024; On 2017/05/11 16:56:31, bcwhite wrote: > On 2017/05/10 17:17:21, Mike Wittman wrote: > > This can be moved back into the private section. > > It has to be public to be accessible to > NativeStackSampler::GetStackBufferSize(). Ah, right. Since it's only used by NativeStackSampler::GetStackBufferSize() we probably should just move it there. That will also make the two platform implementations more consistent. https://codereview.chromium.org/2601633002/diff/480001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/480001/base/profiler/native_s... base/profiler/native_stack_sampler.h:24: // This class contains data structures that can be shared across multiple nit: data structures => a buffer for stack copies "data structures" is a more opaque descriptor than necessary.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2601633002/diff/400001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:395: static constexpr size_t kStackCopyBufferSize = 2 * 1024 * 1024; On 2017/05/11 21:07:02, Mike Wittman wrote: > On 2017/05/11 16:56:31, bcwhite wrote: > > On 2017/05/10 17:17:21, Mike Wittman wrote: > > > This can be moved back into the private section. > > > > It has to be public to be accessible to > > NativeStackSampler::GetStackBufferSize(). > > Ah, right. > > Since it's only used by NativeStackSampler::GetStackBufferSize() we probably > should just move it there. That will also make the two platform implementations > more consistent. Done. https://codereview.chromium.org/2601633002/diff/480001/base/profiler/native_s... File base/profiler/native_stack_sampler.h (right): https://codereview.chromium.org/2601633002/diff/480001/base/profiler/native_s... base/profiler/native_stack_sampler.h:24: // This class contains data structures that can be shared across multiple On 2017/05/11 21:07:02, Mike Wittman wrote: > nit: data structures => a buffer for stack copies > > "data structures" is a more opaque descriptor than necessary. Done.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wittman@chromium.org Link to the patchset: https://codereview.chromium.org/2601633002/#ps500001 (title: "addressed review comments by wittman")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1494595369033780,
"parent_rev": "06b970c72e508b32a122679963a632bfd4d357a4", "commit_rev":
"1a4d8f0a44795c09c519f9c96fec431793eb5837"}
Message was sent while issue was closed.
Description was changed from ========== Use a common buffer across all instances for stack-copy. BUG=671716 ========== to ========== Use a common buffer across all instances for stack-copy. BUG=671716 Review-Url: https://codereview.chromium.org/2601633002 Cr-Commit-Position: refs/heads/master@{#471294} Committed: https://chromium.googlesource.com/chromium/src/+/1a4d8f0a44795c09c519f9c96fec... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:500001) as https://chromium.googlesource.com/chromium/src/+/1a4d8f0a44795c09c519f9c96fec... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
