Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2444143002:
Add process lifetime annotations to stack samples. (Closed)
|
Created:
4 years, 1 month ago by bcwhite Modified:
4 years ago CC:
Aaron Boodman, abarth-chromium, asvitkine+watch_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd process lifetime annotations to stack samples.
Annotations are passed in the proto along
with the stack frames so analysis can correlate with other things
happening on the system.
BUG=657012
Committed: https://crrev.com/30c14f27cf63a48b082efe67ae29bb6e4282cbd7
Cr-Commit-Position: refs/heads/master@{#434273}
Patch Set 1 #
Total comments: 24
Patch Set 2 : addressed review comments by wittman #
Total comments: 6
Patch Set 3 : added comments and fixed some build problems #
Total comments: 2
Patch Set 4 : addressed review comments by wittman #
Total comments: 6
Patch Set 5 : use bare function-pointer callback; added test #
Total comments: 10
Patch Set 6 : addressed comments by wittman #Patch Set 7 : updated proto to pass individual repeated enumeration values #
Total comments: 16
Patch Set 8 : addressed review comments and added test #
Total comments: 3
Patch Set 9 : restored lost changes #Patch Set 10 : convert between C++ enum and proto enum; rewrote test to be extensible and readable #Patch Set 11 : fixed non-windows constant #
Total comments: 29
Patch Set 12 : addressed comments by asvitkine #Patch Set 13 : rebased #Patch Set 14 : addressed review comments and finished converting tests #
Total comments: 32
Patch Set 15 : addressed review comments & git cl format base #
Total comments: 2
Patch Set 16 : use shorter integer for ProfilesGenerator #Patch Set 17 : rebased #Patch Set 18 : switch from #define generator array to builder class #
Total comments: 9
Patch Set 19 : abandon generator for on-the-fly Profiles creation #Messages
Total messages: 192 (146 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 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
wittman@chromium.org changed reviewers: + wittman@chromium.org
https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:84: void ChangeAtomicFlags(volatile subtle::Atomic32* flags, Same comment about volatile. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:192: sample.current_activities = subtle::NoBarrier_Load(¤t_activities_); The SuspendThread call in the native stack sampler implementation is reportedly fairly expensive, so to maximize accuracy it's probably worth saving the state off after that occurs. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:68: enum ProcessPhase : int { uint32_t (and below) https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:121: struct BASE_EXPORT Sample { We probably should a add constructor at this point to ensure the bitfields are initialized. That should reduce the verbosity of the test code as well. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:128: int32_t process_phases; uint32_t (and below) https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:264: static volatile subtle::Atomic32 process_phases_; I believe declaring subtle::Atomic32 volatile is unnecessary. It's not done elsewhere in Chrome. https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... components/metrics/proto/call_stack_profile.proto:39: optional fixed32 process_phases = 3; The individual phases and activities should be explicitly specified in the proto, so we have agreement on meaning between the Chrome side and and the server side. My understanding is that it's not practical to define bitmask constants in protos, and the standard way of representing data like this is fields of repeated enums (as mentioned by Alexei in the bug). We'll need to translate between the two representations in the metrics provider. https://codereview.chromium.org/2444143002/diff/1/components/metrics/public/i... File components/metrics/public/interfaces/call_stack_profile_collector.mojom (right): https://codereview.chromium.org/2444143002/diff/1/components/metrics/public/i... components/metrics/public/interfaces/call_stack_profile_collector.mojom:24: int32 process_phases; uint32
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...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #2 (id:20001) 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #2 (id:40001) 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...
bcwhite@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:84: void ChangeAtomicFlags(volatile subtle::Atomic32* flags, On 2016/10/24 22:28:14, Mike Wittman wrote: > Same comment about volatile. Done. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:192: sample.current_activities = subtle::NoBarrier_Load(¤t_activities_); On 2016/10/24 22:28:14, Mike Wittman wrote: > The SuspendThread call in the native stack sampler implementation is reportedly > fairly expensive, so to maximize accuracy it's probably worth saving the state > off after that occurs. Done. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:68: enum ProcessPhase : int { On 2016/10/24 22:28:14, Mike Wittman wrote: > uint32_t (and below) I'd rather not since I'm using it as a "flag number" that's only coded as "1 << flag". "int" is the natural word type on the machine. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:121: struct BASE_EXPORT Sample { On 2016/10/24 22:28:14, Mike Wittman wrote: > We probably should a add constructor at this point to ensure the bitfields are > initialized. That should reduce the verbosity of the test code as well. Done. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:128: int32_t process_phases; On 2016/10/24 22:28:14, Mike Wittman wrote: > uint32_t (and below) Atomic32 is always a signed (unfortunately) so making this signed allows conversion without risking a bunch of signed/unsigned compiler gymnastics. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:264: static volatile subtle::Atomic32 process_phases_; On 2016/10/24 22:28:14, Mike Wittman wrote: > I believe declaring subtle::Atomic32 volatile is unnecessary. It's not done > elsewhere in Chrome. True. It is done in at least one place (because I did it) but its only a requirement if its an atomic shared across different processes. https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... components/metrics/proto/call_stack_profile.proto:39: optional fixed32 process_phases = 3; As I understand it, this will require the constants to be defined in at least three places: stack_sampling_profiler.h (in Chrome) call_stack_profiles.proto (in Chrome) call_stack_profile.proto (in Google3) (another in Google3?) Wouldn't it be better to _not_ define it in the .proto at all and define it only twice: stack_sampling_profiler.h (in Chrome) stack_sampling_profiler.h (or whatever it's called -- in Google3)
https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; Do we need to use atomics here? Can't we just always do modifications to these on the main thread?
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...)
https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; On 2016/10/25 14:49:08, Alexei Svitkine (slow) wrote: > Do we need to use atomics here? > > Can't we just always do modifications to these on the main thread? I don't know if we can guarantee that or not but it wouldn't matter since they must also be read by the sampling thread to be stored alongside the collected stack frames.
https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; On 2016/10/25 15:02:22, bcwhite wrote: > On 2016/10/25 14:49:08, Alexei Svitkine (slow) wrote: > > Do we need to use atomics here? > > > > Can't we just always do modifications to these on the main thread? > > I don't know if we can guarantee that or not but it wouldn't matter since they > must also be read by the sampling thread to be stored alongside the collected > stack frames. In terms of guarantee, I was thinking the code can just do a post task if it's not on the right thread currently. If these need to be read on the sampling thread, can we just do a post task to the sampling thread and always update them there? I guess the downside of the post task approach is that they may arrive late - so maybe having these atomic is better. OK, I'm compelled with having them atomic given the above - but please expand comment above them to mention both the fact that they're needed on sampling thread and that the alternative of using posttask has the risk of the updates arriving late which would make the data not accurate.
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 #3 (id:80001) 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...
https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; On 2016/10/25 15:25:58, Alexei Svitkine (slow) wrote: > On 2016/10/25 15:02:22, bcwhite wrote: > > On 2016/10/25 14:49:08, Alexei Svitkine (slow) wrote: > > > Do we need to use atomics here? > > > > > > Can't we just always do modifications to these on the main thread? > > > > I don't know if we can guarantee that or not but it wouldn't matter since they > > must also be read by the sampling thread to be stored alongside the collected > > stack frames. > > In terms of guarantee, I was thinking the code can just do a post task if it's > not on the right thread currently. > > If these need to be read on the sampling thread, can we just do a post task to > the sampling thread and always update them there? > > I guess the downside of the post task approach is that they may arrive late - so > maybe having these atomic is better. OK, I'm compelled with having them atomic > given the above - but please expand comment above them to mention both the fact > that they're needed on sampling thread and that the alternative of using > posttask has the risk of the updates arriving late which would make the data not > accurate. Done.
https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:192: sample.current_activities = subtle::NoBarrier_Load(¤t_activities_); On 2016/10/25 14:45:23, bcwhite wrote: > On 2016/10/24 22:28:14, Mike Wittman wrote: > > The SuspendThread call in the native stack sampler implementation is > reportedly > > fairly expensive, so to maximize accuracy it's probably worth saving the state > > off after that occurs. > > Done. Sorry for not being specific: I was intending to suggest that this should happen within SuspendThreadAndRecordStack within ScopedSuspendThread. There's a fair amount of work being done both before and after that point, so this would place the recording of the state as close as possible to the actual stack capture. We could probably pass a bare function pointer callback so that the native stack sampler doesn't have to know about the details of obtaining the state. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:68: enum ProcessPhase : int { On 2016/10/25 14:45:23, bcwhite wrote: > On 2016/10/24 22:28:14, Mike Wittman wrote: > > uint32_t (and below) > > I'd rather not since I'm using it as a "flag number" that's only coded as "1 << > flag". "int" is the natural word type on the machine. Ah, right. Forgot that this was an index and not a mask. Do we even need a type specifier here? https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:128: int32_t process_phases; On 2016/10/25 14:45:23, bcwhite wrote: > On 2016/10/24 22:28:14, Mike Wittman wrote: > > uint32_t (and below) > > Atomic32 is always a signed (unfortunately) so making this signed allows > conversion without risking a bunch of signed/unsigned compiler gymnastics. The use of Atomic32 is an implementation detail of the profiler; I don't think it should influence the interfaces. The unsigned cast is guaranteed to preserve the bit pattern, and it looks like there's only two places where it would be required, so this doesn't seem like a significant burden. https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... components/metrics/proto/call_stack_profile.proto:39: optional fixed32 process_phases = 3; The interpretation of the phases/activities is part of the interface between Chrome and the UMA server, so it really should be explicitly specified. Without it there's no good mechanism to manage changes to the data being sent/processed across versions of Chrome and the server code. The duplication is unfortunately unavoidable, but as far as I know doing things this way is standard practice. If the enums are in a standalone proto -- execution_context.proto probably would be a good home -- then the constants will be defined in two places, one of which gets copied to Google3. The Google3 stack_sampling_profiler.h equivalent is a proto definition rather than anything defined in source code, so there's no additional duplication there. https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; On 2016/10/25 16:22:26, bcwhite wrote: > On 2016/10/25 15:25:58, Alexei Svitkine (slow) wrote: > > On 2016/10/25 15:02:22, bcwhite wrote: > > > On 2016/10/25 14:49:08, Alexei Svitkine (slow) wrote: > > > > Do we need to use atomics here? > > > > > > > > Can't we just always do modifications to these on the main thread? > > > > > > I don't know if we can guarantee that or not but it wouldn't matter since > they > > > must also be read by the sampling thread to be stored alongside the > collected > > > stack frames. > > > > In terms of guarantee, I was thinking the code can just do a post task if it's > > not on the right thread currently. > > > > If these need to be read on the sampling thread, can we just do a post task to > > the sampling thread and always update them there? > > > > I guess the downside of the post task approach is that they may arrive late - > so > > maybe having these atomic is better. OK, I'm compelled with having them atomic > > given the above - but please expand comment above them to mention both the > fact > > that they're needed on sampling thread and that the alternative of using > > posttask has the risk of the updates arriving late which would make the data > not > > accurate. > > Done. It's also worth mentioning that these have to be atomics rather than guarded by a lock, to avoid deadlock during stack collection.
https://codereview.chromium.org/2444143002/diff/100001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/100001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:70: FirstNonEmptyPaint, One other thing: being in //base, the profiler is intended to be a general mechanism for capturing stack profiles. These enums will be highly specific to Chrome execution, so don't belong here. They probably should go in components/metrics, with the interfaces here generalized to use basic types.
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...
https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:192: sample.current_activities = subtle::NoBarrier_Load(¤t_activities_); On 2016/10/25 17:24:22, Mike Wittman wrote: > On 2016/10/25 14:45:23, bcwhite wrote: > > On 2016/10/24 22:28:14, Mike Wittman wrote: > > > The SuspendThread call in the native stack sampler implementation is > > reportedly > > > fairly expensive, so to maximize accuracy it's probably worth saving the > state > > > off after that occurs. > > > > Done. > > Sorry for not being specific: I was intending to suggest that this should happen > within SuspendThreadAndRecordStack within ScopedSuspendThread. There's a fair > amount of work being done both before and after that point, so this would place > the recording of the state as close as possible to the actual stack capture. > > We could probably pass a bare function pointer callback so that the native stack > sampler doesn't have to know about the details of obtaining the state. Done. Ya know... Callbacks only _look_ simple... :-) https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:68: enum ProcessPhase : int { On 2016/10/25 17:24:22, Mike Wittman wrote: > On 2016/10/25 14:45:23, bcwhite wrote: > > On 2016/10/24 22:28:14, Mike Wittman wrote: > > > uint32_t (and below) > > > > I'd rather not since I'm using it as a "flag number" that's only coded as "1 > << > > flag". "int" is the natural word type on the machine. > > Ah, right. Forgot that this was an index and not a mask. > > Do we even need a type specifier here? I assumed it was necessary in order to use it as an integer with the "<<" operator... but it turns out not. Removed. https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.h:128: int32_t process_phases; On 2016/10/25 17:24:22, Mike Wittman wrote: > On 2016/10/25 14:45:23, bcwhite wrote: > > On 2016/10/24 22:28:14, Mike Wittman wrote: > > > uint32_t (and below) > > > > Atomic32 is always a signed (unfortunately) so making this signed allows > > conversion without risking a bunch of signed/unsigned compiler gymnastics. > > The use of Atomic32 is an implementation detail of the profiler; I don't think > it should influence the interfaces. > > The unsigned cast is guaranteed to preserve the bit pattern, and it looks like > there's only two places where it would be required, so this doesn't seem like a > significant burden. Good enough for me. Perhaps the compilers will be generous given that it's only assignments. https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/1/components/metrics/proto/ca... components/metrics/proto/call_stack_profile.proto:39: optional fixed32 process_phases = 3; On 2016/10/25 17:24:22, Mike Wittman wrote: > The interpretation of the phases/activities is part of the interface between > Chrome and the UMA server, so it really should be explicitly specified. Without > it there's no good mechanism to manage changes to the data being sent/processed > across versions of Chrome and the server code. The duplication is unfortunately > unavoidable, but as far as I know doing things this way is standard practice. > > If the enums are in a standalone proto -- execution_context.proto probably would > be a good home -- then the constants will be defined in two places, one of which > gets copied to Google3. The Google3 stack_sampling_profiler.h equivalent is a > proto definition rather than anything defined in source code, so there's no > additional duplication there. I'll work on it. https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/60001/base/profiler/stack_sam... base/profiler/stack_sampling_profiler.h:272: static subtle::Atomic32 current_activities_; On 2016/10/25 17:24:22, Mike Wittman wrote: > On 2016/10/25 16:22:26, bcwhite wrote: > > On 2016/10/25 15:25:58, Alexei Svitkine (slow) wrote: > > > On 2016/10/25 15:02:22, bcwhite wrote: > > > > On 2016/10/25 14:49:08, Alexei Svitkine (slow) wrote: > > > > > Do we need to use atomics here? > > > > > > > > > > Can't we just always do modifications to these on the main thread? > > > > > > > > I don't know if we can guarantee that or not but it wouldn't matter since > > they > > > > must also be read by the sampling thread to be stored alongside the > > collected > > > > stack frames. > > > > > > In terms of guarantee, I was thinking the code can just do a post task if > it's > > > not on the right thread currently. > > > > > > If these need to be read on the sampling thread, can we just do a post task > to > > > the sampling thread and always update them there? > > > > > > I guess the downside of the post task approach is that they may arrive late > - > > so > > > maybe having these atomic is better. OK, I'm compelled with having them > atomic > > > given the above - but please expand comment above them to mention both the > > fact > > > that they're needed on sampling thread and that the alternative of using > > > posttask has the risk of the updates arriving late which would make the data > > not > > > accurate. > > > > Done. > > It's also worth mentioning that these have to be atomics rather than guarded by > a lock, to avoid deadlock during stack collection. Done. https://codereview.chromium.org/2444143002/diff/100001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/100001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:70: FirstNonEmptyPaint, On 2016/10/25 17:47:45, Mike Wittman wrote: > One other thing: being in //base, the profiler is intended to be a general > mechanism for capturing stack profiles. These enums will be highly specific to > Chrome execution, so don't belong here. They probably should go in > components/metrics, with the interfaces here generalized to use basic types. I hate to lose the type-enforcement, but I guess that makes sense. Will do...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/1/base/profiler/stack_samplin... base/profiler/stack_sampling_profiler.cc:192: sample.current_activities = subtle::NoBarrier_Load(¤t_activities_); On 2016/10/25 21:10:42, bcwhite wrote: > On 2016/10/25 17:24:22, Mike Wittman wrote: > > On 2016/10/25 14:45:23, bcwhite wrote: > > > On 2016/10/24 22:28:14, Mike Wittman wrote: > > > > The SuspendThread call in the native stack sampler implementation is > > > reportedly > > > > fairly expensive, so to maximize accuracy it's probably worth saving the > > state > > > > off after that occurs. > > > > > > Done. > > > > Sorry for not being specific: I was intending to suggest that this should > happen > > within SuspendThreadAndRecordStack within ScopedSuspendThread. There's a fair > > amount of work being done both before and after that point, so this would > place > > the recording of the state as close as possible to the actual stack capture. > > > > We could probably pass a bare function pointer callback so that the native > stack > > sampler doesn't have to know about the details of obtaining the state. > > Done. Ya know... Callbacks only _look_ simple... :-) Thanks. But it gets even more complicated. :) (See next comment.) https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); 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); https://codereview.chromium.org/2444143002/diff/120001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:197: using AnnotateCallback = Callback<void(Sample*)>; I think this type would be better on NativeStackSampler, to minimize the circular dependencies between NativeStackSampler and StackSamplingProfiler. (CallStackProfile and its supporting types probably belong in their own file to avoid the backwards dependency entirely, but that's an entirely separate issue.)
https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); On 2016/10/26 16:28:17, Mike Wittman wrote: > 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. 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);
https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); On 2016/10/26 18:30:43, bcwhite wrote: > On 2016/10/26 16:28:17, Mike Wittman wrote: > > 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. > > 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); 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.
https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); > 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. 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. > 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. 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.
https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); On 2016/10/26 20:02:10, bcwhite wrote: > > 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. > > 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. 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.
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 #5 (id:140001) 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...
Patchset #5 (id:160001) has been deleted
On 2016/10/26 20:20:37, Mike Wittman wrote: > https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... > File base/profiler/native_stack_sampler_win.cc (right): > > https://codereview.chromium.org/2444143002/diff/120001/base/profiler/native_s... > base/profiler/native_stack_sampler_win.cc:358: annotator.Run(); > On 2016/10/26 20:02:10, bcwhite wrote: > > > 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. > > > > 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. > > 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. Now using bare funcion-pointer. I chose not to do any kind of multi-return or opaque-mangling because I don't want the "native sampler" code to have to change if the StackSamplingProfiler code starts doing annotations a different way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2444143002/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:417: AnnotateCallback annotator_; const https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:328: void StackSamplingProfiler::SetProcessPhase(ProcessPhase phase) { DCHECK_GE(phase, 0); DCHECK_LT(phase, sizeof(process_phases_)*8); and similarly below https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:72: using ProcessActivity = int; Can we avoid defining these typedefs and just use "int" in the interfaces? I feel like these are a net negative for readability in that they don't add information to the interfaces below, and they hide the type in use, forcing the reader to refer back here to be able to fully understand the interfaces. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:232: // frame. These are all thread-safe so can be called from anywhere. Document that the valid values are between 0 and 31. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:594: StackSamplingProfiler::SetProcessPhase(1); I think it's worth splitting the annotation checks into a separate test, to keep things focused in each test case. Also, we should check that the annotations are 0 if unset.
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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 #6 (id:200001) has been deleted
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 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_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 #7 (id:240001) 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...
I've updated the .proto to define the enumeration and changed the field to repeated enumerations. The enum values from the proto can now be passed to the Set/Begin/End methods directly. Still have to actually set them from an appropriate place in the code. That can be done in a later CL. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:417: AnnotateCallback annotator_; On 2016/11/02 17:14:38, Mike Wittman wrote: > const Done. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:328: void StackSamplingProfiler::SetProcessPhase(ProcessPhase phase) { On 2016/11/02 17:14:38, Mike Wittman wrote: > DCHECK_GE(phase, 0); > DCHECK_LT(phase, sizeof(process_phases_)*8); > > and similarly below Done. Though this was checked implicitly in ChangeAtomicFlags. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:72: using ProcessActivity = int; On 2016/11/02 17:14:39, Mike Wittman wrote: > Can we avoid defining these typedefs and just use "int" in the interfaces? I > feel like these are a net negative for readability in that they don't add > information to the interfaces below, and they hide the type in use, forcing the > reader to refer back here to be able to fully understand the interfaces. I had wanted to clearly define what is being passed. But now that I've got the constants generated in the .proto file, it has to be an "int" anyway. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:232: // frame. These are all thread-safe so can be called from anywhere. On 2016/11/02 17:14:39, Mike Wittman wrote: > Document that the valid values are between 0 and 31. Done. https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/180001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:594: StackSamplingProfiler::SetProcessPhase(1); On 2016/11/02 17:14:39, Mike Wittman wrote: > I think it's worth splitting the annotation checks into a separate test, to keep > things focused in each test case. > > Also, we should check that the annotations are 0 if unset. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/02 20:10:45, bcwhite wrote: > I've updated the .proto to define the enumeration and changed the field to > repeated enumerations. > > The enum values from the proto can now be passed to the Set/Begin/End methods > directly. Using proto-generated types in public interfaces is a no-no in Chrome because it forces serialization of the build process. So we'll still need a C++ enum corresponding to these proto types. https://codereview.chromium.org/2444143002/diff/260001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:18: #include "base/bind.h" This can be removed. https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:338: DCHECK_GT(static_cast<int>(sizeof(process_phases_) * 8), activity); process_phases_ => current_activities_ (and below) https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:227: // parameter values should be from an enumeration of the appropriate type We should also specify that these functions set/clear the corresponding bits in the Sample fields, and that the meaning of the bits is user-defined. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:267: // Phases are "set only" so current is always superset of last-seen. The process phase/activities encoding should have tests. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:267: // Phases are "set only" so current is always superset of last-seen. Also, this logic could stand to be extracted into its own function. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; What's the rationale beyond recording metrics collection as a phase?
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...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #8 (id:280001) 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...
> Using proto-generated types in public interfaces is a > no-no in Chrome because it forces serialization of the > build process. So we'll still need a C++ enum > corresponding to these proto types. Because that file has to be generated and so nothing that depends on it can be compiled until that happens? There's no need to include the generated .pb.h file from within any .h file. Only the various .cc files that make the method calls would need to include it. But if that's still a problem, it won't be difficult to create a parallel .h file -- just somewhat annoying to ensure the two stay in sync. Either way, it's for the follow-on CL that will actually make these calls. https://codereview.chromium.org/2444143002/diff/260001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:18: #include "base/bind.h" On 2016/11/02 21:44:04, Mike Wittman wrote: > This can be removed. Done. https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:338: DCHECK_GT(static_cast<int>(sizeof(process_phases_) * 8), activity); On 2016/11/02 21:44:04, Mike Wittman wrote: > process_phases_ => current_activities_ (and below) Oops. Done. https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/260001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:227: // parameter values should be from an enumeration of the appropriate type On 2016/11/02 21:44:04, Mike Wittman wrote: > We should also specify that these functions set/clear the corresponding bits in > the Sample fields, and that the meaning of the bits is user-defined. Done. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:267: // Phases are "set only" so current is always superset of last-seen. On 2016/11/02 21:44:04, Mike Wittman wrote: > Also, this logic could stand to be extracted into its own function. Done. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:267: // Phases are "set only" so current is always superset of last-seen. On 2016/11/02 21:44:04, Mike Wittman wrote: > The process phase/activities encoding should have tests. OMG the existing tests are ugly! <shudder> Done. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; On 2016/11/02 21:44:04, Mike Wittman wrote: > What's the rationale beyond recording metrics collection as a phase? It's not a phase, just a periodic activity that occurs (begins and ends) during normal operation. It's not yet used and so could change if it's not useful but there has to be at least one value defined for tests not to break.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It looks like the changes other than the metrics provider did not make it into patch set 8. On 2016/11/03 18:29:50, bcwhite wrote: > > Using proto-generated types in public interfaces is a > > no-no in Chrome because it forces serialization of the > > build process. So we'll still need a C++ enum > > corresponding to these proto types. > > Because that file has to be generated and so nothing that depends on it can be > compiled until that happens? > > There's no need to include the generated .pb.h file from within any .h file. > Only the various .cc files that make the method calls would need to include it. > > But if that's still a problem, it won't be difficult to create a parallel .h > file -- just somewhat annoying to ensure the two stay in sync. > > Either way, it's for the follow-on CL that will actually make these calls. Yes, as I understand it the issue is that this introduces inter-target build sequence dependencies. If you look at the .pb.h includes in Chrome, they're all included internal to their own component. The other reason for the having the C++ enum is to decouple the bit indices in use by the profiler from the enum values in the protobuf. Otherwise, future deprecation of enum values will prevent the use of bit positions in the annotation words. This will change the way the protobuf encoding is done in the metrics provider, so it can't reasonably be deferred until the follow-on CL. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; On 2016/11/03 18:29:50, bcwhite wrote: > On 2016/11/02 21:44:04, Mike Wittman wrote: > > What's the rationale beyond recording metrics collection as a phase? > > It's not a phase, just a periodic activity that occurs (begins and ends) during > normal operation. It's not yet used and so could change if it's not useful but > there has to be at least one value defined for tests not to break. Sorry, "phase" was a typo. So, three things: 1. Proto changes have to be reviewed in google3 by internal logging and privacy teams, and committed before they can be approved for commit in the Chrome repository. 2. There's a substantial cost to changing these enums, both in terms of re-review and backwards compatibility. Once sent over the wire the enum values essentially have to live forever so the data can continue to be interpreted server-side. 3. As a result, we should settle on exactly what these enums need to contain to support recording the desired startup information, and get that reviewed in google3 before committing here. If we don't have a need for recording process activity yet then it should be removed for the time being. https://codereview.chromium.org/2444143002/diff/300001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/300001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:311: const Frame sample_frames[][1] = { This test is getting kind of hard to follow... I think it would be easier to understand if we represent the data at the Sample level. e.g. Sample sample1({Frame(module_base_address + 0x10, 0)}, 1, 0); Sample sample2({Frame(module_base_address + 0x20, 0)}, 1, 0); Sample sample3({Frame(module_base_address + 0x10, 0)}, 3, 0); Sample sample4({Frame(module_base_address + 0x20, 0)}, 3, 0); Sample sample5({Frame(module_base_address + 0x20, 0)}, 3, 1); const Sample samples[] = { sample1, sample2, sample3, sample4, sample5, sample1, sample1, sample3 }; struct Expected { Sample sample; int count; }; Expected expected[] = { {sample1, 3}, {sample2, 1}, {sample3, 2}, {sample4, 1}, {sample5, 1} }; https://codereview.chromium.org/2444143002/diff/300001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:401: for (int x = 0; x < proto_sample.activities_ended_size(); ++x) { EXPECT_EQ(0u, proto_sample.activities_ended_size()) ? https://codereview.chromium.org/2444143002/diff/300001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:437: const Frame sample_frames[][1] = { Same comment here about representing with Sample.
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...
Odd about the missing changes. They're in my editor buffers (saved) but not in git. Musta messed something up. Fixed. https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; On 2016/11/03 21:15:39, Mike Wittman wrote: > On 2016/11/03 18:29:50, bcwhite wrote: > > On 2016/11/02 21:44:04, Mike Wittman wrote: > > > What's the rationale beyond recording metrics collection as a phase? > > > > It's not a phase, just a periodic activity that occurs (begins and ends) > during > > normal operation. It's not yet used and so could change if it's not useful > but > > there has to be at least one value defined for tests not to break. > > Sorry, "phase" was a typo. So, three things: > > 1. Proto changes have to be reviewed in google3 by internal logging and privacy > teams, and committed before they can be approved for commit in the Chrome > repository. > > 2. There's a substantial cost to changing these enums, both in terms of > re-review and backwards compatibility. Once sent over the wire the enum values > essentially have to live forever so the data can continue to be interpreted > server-side. > > 3. As a result, we should settle on exactly what these enums need to contain to > support recording the desired startup information, and get that reviewed in > google3 before committing here. If we don't have a need for recording process > activity yet then it should be removed for the time being. Ah, well then. I had planned to get this in, then the follow-on CL, all before going the google3 version. Since nothing yet sets those bits, they could be changed to whatever with no backward compatibility problems. I guess there will have to be some discussion next week.
https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; On 2016/11/03 21:51:10, bcwhite wrote: > On 2016/11/03 21:15:39, Mike Wittman wrote: > > On 2016/11/03 18:29:50, bcwhite wrote: > > > On 2016/11/02 21:44:04, Mike Wittman wrote: > > > > What's the rationale beyond recording metrics collection as a phase? > > > > > > It's not a phase, just a periodic activity that occurs (begins and ends) > > during > > > normal operation. It's not yet used and so could change if it's not useful > > but > > > there has to be at least one value defined for tests not to break. > > > > Sorry, "phase" was a typo. So, three things: > > > > 1. Proto changes have to be reviewed in google3 by internal logging and > privacy > > teams, and committed before they can be approved for commit in the Chrome > > repository. > > > > 2. There's a substantial cost to changing these enums, both in terms of > > re-review and backwards compatibility. Once sent over the wire the enum values > > essentially have to live forever so the data can continue to be interpreted > > server-side. > > > > 3. As a result, we should settle on exactly what these enums need to contain > to > > support recording the desired startup information, and get that reviewed in > > google3 before committing here. If we don't have a need for recording process > > activity yet then it should be removed for the time being. > > Ah, well then. I had planned to get this in, then the follow-on CL, all before > going the google3 version. Since nothing yet sets those bits, they could be > changed to whatever with no backward compatibility problems. > > I guess there will have to be some discussion next week. In general we shouldn't be committing Chrome functionality to support uploading new state -- even if incomplete or disabled -- until we have policy approval for saving the state. There are too many things that could go wrong (e.g. random bit flips) that could cause state to be uploaded anyway and put us in questionable territory. Google3 approval for this stuff, once settled, shouldn't be difficult or take very long. Let's move discussions of which startup phases should be recorded into the bug. I won't be there for discussions next week, but I'm sure Alexei can provide guidance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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/2444143002/diff/260001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/260001/components/metrics/pro... components/metrics/proto/execution_context.proto:63: METRICS_COLLECTION = 0; On 2016/11/03 23:11:03, Mike Wittman - OOO to Nov.15 wrote: > On 2016/11/03 21:51:10, bcwhite wrote: > > On 2016/11/03 21:15:39, Mike Wittman wrote: > > > On 2016/11/03 18:29:50, bcwhite wrote: > > > > On 2016/11/02 21:44:04, Mike Wittman wrote: > > > > > What's the rationale beyond recording metrics collection as a phase? > > > > > > > > It's not a phase, just a periodic activity that occurs (begins and ends) > > > during > > > > normal operation. It's not yet used and so could change if it's not > useful > > > but > > > > there has to be at least one value defined for tests not to break. > > > > > > Sorry, "phase" was a typo. So, three things: > > > > > > 1. Proto changes have to be reviewed in google3 by internal logging and > > privacy > > > teams, and committed before they can be approved for commit in the Chrome > > > repository. > > > > > > 2. There's a substantial cost to changing these enums, both in terms of > > > re-review and backwards compatibility. Once sent over the wire the enum > values > > > essentially have to live forever so the data can continue to be interpreted > > > server-side. > > > > > > 3. As a result, we should settle on exactly what these enums need to contain > > to > > > support recording the desired startup information, and get that reviewed in > > > google3 before committing here. If we don't have a need for recording > process > > > activity yet then it should be removed for the time being. > > > > Ah, well then. I had planned to get this in, then the follow-on CL, all > before > > going the google3 version. Since nothing yet sets those bits, they could be > > changed to whatever with no backward compatibility problems. > > > > I guess there will have to be some discussion next week. > > In general we shouldn't be committing Chrome functionality to support uploading > new state -- even if incomplete or disabled -- until we have policy approval for > saving the state. There are too many things that could go wrong (e.g. random bit > flips) that could cause state to be uploaded anyway and put us in questionable > territory. Google3 approval for this stuff, once settled, shouldn't be difficult > or take very long. Makes perfect sense. I just didn't know about it.
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...
I've created a local enum of phases/activities updated the metrics provider to support conversion between those and similar ones defined in the .proto file. It'll have to be refined once there are official value. I've also rewritten one of the tests to be much simpler with reusable validation code but wanted to get comments on it before I changed the remaining tests to operate the same.
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...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #10 (id:340001) 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...)
Seems there's some red bots. Let me know when you've resolved those and I can take a look.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/11/07 17:33:29, Alexei Svitkine (very slow) wrote: > Seems there's some red bots. Let me know when you've resolved those and I can > take a look. Looks like it was just a paste-o of one of the non-windows constants in the test. Fixed.
https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: (*annotator)(sample); This requires the annotator to not be null. Can you add a DCHECK somewhere to ensure that? e.g. maybe to NativeStackSamplerWin() body? https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:99: } We have other functions elsewhere: https://cs.chromium.org/chromium/src/base/memory/singleton.cc?sq=package:chro... https://cs.chromium.org/chromium/src/base/lazy_instance.cc?q=base/lazy_instan... Besides the comments about sharing in those places, should we call PlatformThread::YieldCurrentThread() in the body of the loop? https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:126: // ProcessPhase, above. This comment is not correct anymore since there's no ProcessPhase above now, right? Same for the one below. Please rephrase to mention what these are now. (Even if the answer that these are defined elsewhere and pass through function x). https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:130: // captured. See ProcessActivity, above. I think there needs to be a more in depth comment somewhere discussing the distinction between phases and activities because it's not obvious to me. https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:634: #define MAYBE_Annotations DISABLED_Annotations How about just having the full TEST() {} block inside the ifdef? I'm not sure what the current structure buys over that. I think DISABLED_ is usually used when a test that's flaky gets disabled but in this case we just don't want to ever run it if the ifdef is false. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:36: const ProcessPhase Please document these mappings with a comment above. I guess this is to map specific bits to protobuf constants. Which seems fine, but should be made more clear. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:264: // Transcode Sample annotations into protobuf fields. Maybe expand the comment for the motivation - that internally in Chrome we use bit fields to set these since there shouldn't be more than 32 at any time, but the protobuf uses repeated fields for future proof-ness and because the set of 32 may evolve over time. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:270: ++bit, new_phases >>= 1) { Instead of new_phases >>= 1 logic, I'd actually just unset the bit in the if - I think that would be a bit clearer. (Interestingly, x86 has instructions to find the next set bit which are exposed as e.g. __builtin_clz on GCC. However, I didn't find a base:: abstraction for these, so I think current approach is fine.) https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:76: #define GEN_ADD_PHASE(phase) kAddPhase, phase What is this for? I don't see it being used. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/call_stack_profile.proto:42: repeated ProcessPhase process_phase = 3; This looks good to me! I'm not sure about the activities part - so we can chat offline or you can remove it from the CL for now. Whichever we do, we'll need to land the server-side version of this proto change before submitting this CL. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/execution_context.proto:69: } I'm not super convinced of the utility of these, but happy to hear the use case. Either we can chat about these offline or on this CL and continue the review from there - or alternatively how about removing the activity stuff from this initial CL and it can be landed as a follow-up CL later?
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/2444143002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: (*annotator)(sample); On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > This requires the annotator to not be null. Can you add a DCHECK somewhere to > ensure that? > > e.g. maybe to NativeStackSamplerWin() body? Done. https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:99: } > We have other functions elsewhere: > > https://cs.chromium.org/chromium/src/base/memory/singleton.cc?sq=package:chro... > > https://cs.chromium.org/chromium/src/base/lazy_instance.cc?q=base/lazy_instan... Those are different beasts. They're effectively a spin-lock while other work is going on. > Besides the comments about sharing in those places, should we call > PlatformThread::YieldCurrentThread() in the body of the loop? There's no benefit here. Those other cases deal with outside (i.e. non-atomic) work that takes some arbitrary amount of time. This function has no "other" work, just the single CAS. Any other thread (which would, by necessity of it being a single instruction, be on a parallel core) that has caused the conflict has already completed so there's no point in waiting. With std::atomic, this would be just bits = flags->Load(std::memory_order_relaxed); while (!flags->compare_exchange_weak(bits, ..., std::memory_order_relaxed)) ; https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.h (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:126: // ProcessPhase, above. On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > This comment is not correct anymore since there's no ProcessPhase above now, > right? > > Same for the one below. > > Please rephrase to mention what these are now. (Even if the answer that these > are defined elsewhere and pass through function x). Done. https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.h:130: // captured. See ProcessActivity, above. On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > I think there needs to be a more in depth comment somewhere discussing the > distinction between phases and activities because it's not obvious to me. Done. https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:634: #define MAYBE_Annotations DISABLED_Annotations > How about just having the full TEST() {} block inside the ifdef? I'm not sure > what the current structure buys over that. I've wondered about that but this seems to be the standard way of defining "conditional" tests throughout the codebase. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:36: const ProcessPhase On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > Please document these mappings with a comment above. > > I guess this is to map specific bits to protobuf constants. Which seems fine, > but should be made more clear. Done. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:264: // Transcode Sample annotations into protobuf fields. On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > Maybe expand the comment for the motivation - that internally in Chrome we use > bit fields to set these since there shouldn't be more than 32 at any time, but > the protobuf uses repeated fields for future proof-ness and because the set of > 32 may evolve over time. Done. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:270: ++bit, new_phases >>= 1) { > Instead of new_phases >>= 1 logic, I'd actually just unset the bit in the if - I > think that would be a bit clearer. That will be two extra instructions (complement and mask) per loop iteration. You sure? for (size_t bit = 0; new_phases != 0 && bit < sizeof(new_phases) * 8; ++bit) { const uint32_t flag = 1U << bit; if (new_phases & flag) { if (bit >= arraysize(kProtoPhases)) { NOTREACHED(); continue; } sample_proto->add_process_phase(kProtoPhases[bit]); new_phases &= ~flag; } } https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:76: #define GEN_ADD_PHASE(phase) kAddPhase, phase On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > What is this for? I don't see it being used. It's not... yet. It'll be needed when the remaining tests are converted. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/call_stack_profile.proto:42: repeated ProcessPhase process_phase = 3; On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > This looks good to me! > > I'm not sure about the activities part - so we can chat offline or you can > remove it from the CL for now. Whichever we do, we'll need to land the > server-side version of this proto change before submitting this CL. Acknowledged. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/execution_context.proto:69: } On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > I'm not super convinced of the utility of these, but happy to hear the use case. > > Either we can chat about these offline or on this CL and continue the review > from there - or alternatively how about removing the activity stuff from this > initial CL and it can be landed as a follow-up CL later? Yeah, we should talk face-to-face quickly. It's basically there to record big system activities that, if happen to be running concurrently to something under test, could skew the samples. If, to completely make up an example, you notice your code-under-test occasionally spending a lot of time in utility method ABC, it might be nice to know that activity XYZ (which also calls ABC) happened to also be running on those occasions.
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 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/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:270: ++bit, new_phases >>= 1) { On 2016/11/08 01:02:36, bcwhite wrote: > > Instead of new_phases >>= 1 logic, I'd actually just unset the bit in the if - > I > > think that would be a bit clearer. > > That will be two extra instructions (complement and mask) per loop iteration. > You sure? > > for (size_t bit = 0; new_phases != 0 && bit < sizeof(new_phases) * 8; > ++bit) { > const uint32_t flag = 1U << bit; > if (new_phases & flag) { > if (bit >= arraysize(kProtoPhases)) { > NOTREACHED(); > continue; > } > sample_proto->add_process_phase(kProtoPhases[bit]); > new_phases &= ~flag; > } > } It will only do those instructions when the bit is set, which should be true for not too many bits. So I do prefer that version - since it's much clearer to me. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/execution_context.proto:69: } On 2016/11/08 01:02:36, bcwhite wrote: > On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > > I'm not super convinced of the utility of these, but happy to hear the use > case. > > > > Either we can chat about these offline or on this CL and continue the review > > from there - or alternatively how about removing the activity stuff from this > > initial CL and it can be landed as a follow-up CL later? > > Yeah, we should talk face-to-face quickly. It's basically there to record big > system activities that, if happen to be running concurrently to something under > test, could skew the samples. > > If, to completely make up an example, you notice your code-under-test > occasionally spending a lot of time in utility method ABC, it might be nice to > know that activity XYZ (which also calls ABC) happened to also be running on > those occasions. Per offline discussion, we agreed to land the phases first and the activity can be landed later if desired.
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #13 (id:420001) 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...
Patchset #14 (id:460001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:270: ++bit, new_phases >>= 1) { On 2016/11/14 18:26:39, Alexei Svitkine (very slow) wrote: > On 2016/11/08 01:02:36, bcwhite wrote: > > > Instead of new_phases >>= 1 logic, I'd actually just unset the bit in the if > - > > I > > > think that would be a bit clearer. > > > > That will be two extra instructions (complement and mask) per loop iteration. > > You sure? > > > > for (size_t bit = 0; new_phases != 0 && bit < sizeof(new_phases) * 8; > > ++bit) { > > const uint32_t flag = 1U << bit; > > if (new_phases & flag) { > > if (bit >= arraysize(kProtoPhases)) { > > NOTREACHED(); > > continue; > > } > > sample_proto->add_process_phase(kProtoPhases[bit]); > > new_phases &= ~flag; > > } > > } > > It will only do those instructions when the bit is set, which should be true for > not too many bits. So I do prefer that version - since it's much clearer to me. I think it's perfectly clear this way. But done. https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/380001/components/metrics/pro... components/metrics/proto/execution_context.proto:69: } On 2016/11/14 18:26:39, Alexei Svitkine (very slow) wrote: > On 2016/11/08 01:02:36, bcwhite wrote: > > On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > > > I'm not super convinced of the utility of these, but happy to hear the use > > case. > > > > > > Either we can chat about these offline or on this CL and continue the review > > > from there - or alternatively how about removing the activity stuff from > this > > > initial CL and it can be landed as a follow-up CL later? > > > > Yeah, we should talk face-to-face quickly. It's basically there to record big > > system activities that, if happen to be running concurrently to something > under > > test, could skew the samples. > > > > If, to completely make up an example, you notice your code-under-test > > occasionally spending a lot of time in utility method ABC, it might be nice to > > know that activity XYZ (which also calls ABC) happened to also be running on > > those occasions. > > Per offline discussion, we agreed to land the phases first and the activity can > be landed later if desired. Done.
lgtm % comment https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:219: native_sampler_->RecordStackSample(&sample); Nit: Remove this change since it doesn't seem necessary. https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:339: // The code inside this method must not do anything that could acquire a mutex, Nit: Move this comment inside the method. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:305: auto location = sample_index.find(*it); This doesn't need any extra logic to split samples based on process_phases because that logic lives in the iterator == functions, is that right? Please add a comment to that extent both here on above line 290. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.h:40: enum Activities : int { Nit: Remove from this CL. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:153: } break; Nit: I'm not sure this is correct style. I think correct style is: case foo: { break; }
FYI: I'm back now and taking another look... please hold off committing until I've had the chance to review.
https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: (*annotator)(sample); On 2016/11/08 01:02:36, bcwhite wrote: > On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > > This requires the annotator to not be null. Can you add a DCHECK somewhere to > > ensure that? > > > > e.g. maybe to NativeStackSamplerWin() body? > > Done. DCHECK shouldn't be used here because it can allocate memory and deadlock. The use in NativeStackSamplerWin::NativeStackSamplerWin() is sufficient. https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:634: #define MAYBE_Annotations DISABLED_Annotations On 2016/11/08 01:02:36, bcwhite wrote: > > How about just having the full TEST() {} block inside the ifdef? I'm not sure > > what the current structure buys over that. > > I've wondered about that but this seems to be the standard way of defining > "conditional" tests throughout the codebase. This structure keeps the tests compiling on non-Windows platforms. https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:649: EXPECT_EQ(params.sampling_interval, profile1.sampling_period); nit: this check can be removed https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:258: // field with each bit correspondirg to an entry in an enumeration while the nit: corresponding https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:305: auto location = sample_index.find(*it); On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > This doesn't need any extra logic to split samples based on process_phases > because that logic lives in the iterator == functions, is that right? Please add > a comment to that extent both here on above line 290. I believe this does need to split samples based on process_phases. The server-side decoding of the process phases must be the same for both MAY_SHUFFLE and PRESERVE_ORDER, since the ordering spec is not encoded in the protobuf. I don't think a single decoding algorithm is feasible unless the MAY_SHUFFLE stacks are encoded in the same manner. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:124: void CallStackProfileMetricsProviderTest::GenerateProfiles( Can GenerateProfiles and VerifyProfileProto be moved to the anonymous namespace? https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:125: Profiles* profiles, nit: output parameters come after input parameters https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:170: const ExpectedProtoProfile& expected) { nit: swap argument order to match typical (expected, actual) ordering of other test utility functions https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), What's the advantage of this form of input data initialization vs. the previous style? https://codereview.chromium.org/2444143002/diff/480001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/pro... components/metrics/proto/execution_context.proto:57: // Based on histogram Startup.BrowserMainRunnerImplInitializeStep2Time nit: period at end of comment (and other cases below)
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/2444143002/diff/380001/base/profiler/native_s... File base/profiler/native_stack_sampler_win.cc (right): https://codereview.chromium.org/2444143002/diff/380001/base/profiler/native_s... base/profiler/native_stack_sampler_win.cc:358: (*annotator)(sample); On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > On 2016/11/08 01:02:36, bcwhite wrote: > > On 2016/11/07 23:22:07, Alexei Svitkine (very slow) wrote: > > > This requires the annotator to not be null. Can you add a DCHECK somewhere > to > > > ensure that? > > > > > > e.g. maybe to NativeStackSamplerWin() body? > > > > Done. > > DCHECK shouldn't be used here because it can allocate memory and deadlock. The > use in NativeStackSamplerWin::NativeStackSamplerWin() is sufficient. Done. https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:219: native_sampler_->RecordStackSample(&sample); On 2016/11/16 19:10:23, Alexei Svitkine (very slow) wrote: > Nit: Remove this change since it doesn't seem necessary. Done. https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler.cc:339: // The code inside this method must not do anything that could acquire a mutex, On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > Nit: Move this comment inside the method. Done. https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/base/profiler/stack_sa... base/profiler/stack_sampling_profiler_unittest.cc:649: EXPECT_EQ(params.sampling_interval, profile1.sampling_period); On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > nit: this check can be removed Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:258: // field with each bit correspondirg to an entry in an enumeration while the On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > nit: corresponding Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:305: auto location = sample_index.find(*it); On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > This doesn't need any extra logic to split samples based on process_phases > because that logic lives in the iterator == functions, is that right? Please add > a comment to that extent both here on above line 290. Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.cc:305: auto location = sample_index.find(*it); On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > > This doesn't need any extra logic to split samples based on process_phases > > because that logic lives in the iterator == functions, is that right? Please > add > > a comment to that extent both here on above line 290. > > I believe this does need to split samples based on process_phases. The > server-side decoding of the process phases must be the same for both MAY_SHUFFLE > and PRESERVE_ORDER, since the ordering spec is not encoded in the protobuf. I > don't think a single decoding algorithm is feasible unless the MAY_SHUFFLE > stacks are encoded in the same manner. Splitting on phase is already done because, as Alexei said, the comparator for Sample checks it. However, the decoding algorithm for phases does need to be consistent so I'll do that. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.h:40: enum Activities : int { On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > Nit: Remove from this CL. Doh! I thought I got them all. <sigh> Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:124: void CallStackProfileMetricsProviderTest::GenerateProfiles( On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > Can GenerateProfiles and VerifyProfileProto be moved to the anonymous namespace? I think it could be it's paired with Verify just below it so I think it fits better here. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:125: Profiles* profiles, On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > nit: output parameters come after input parameters Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:153: } break; On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > Nit: I'm not sure this is correct style. > > I think correct style is: > > case foo: { > break; > } That's the way git cl format put it. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:170: const ExpectedProtoProfile& expected) { On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > nit: swap argument order to match typical (expected, actual) ordering of other > test utility functions Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > What's the advantage of this form of input data initialization vs. the previous > style? This is far more flexible because you're not limited to having everything the same size as with arrays of arrays. It's much more readable, at least in my opinion. It matches the order in which things happen during a real collection so it's easier to correlate. It's shorter and self-documenting. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/pro... File components/metrics/proto/execution_context.proto (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/pro... components/metrics/proto/execution_context.proto:57: // Based on histogram Startup.BrowserMainRunnerImplInitializeStep2Time On 2016/11/16 22:36:10, Mike Wittman - OOO to Nov.15 wrote: > nit: period at end of comment (and other cases below) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.h:40: enum Activities : int { On 2016/11/16 23:10:49, bcwhite wrote: > On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > > Nit: Remove from this CL. > > Doh! I thought I got them all. <sigh> > Done. Also update the CL description. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), On 2016/11/16 23:10:49, bcwhite wrote: > On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > > What's the advantage of this form of input data initialization vs. the > previous > > style? > > This is far more flexible because you're not limited to having everything the > same size as with arrays of arrays. > > It's much more readable, at least in my opinion. > > It matches the order in which things happen during a real collection so it's > easier to correlate. > > It's shorter and self-documenting. Sounds reasonable. Macros are discouraged by C++ style though, and generally avoided in Chrome when a pure C++ implementation is viable. Can you translate this to use a builder pattern? https://codereview.chromium.org/2444143002/diff/500001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/500001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:32: using ProfilesGenerator = uint64_t; This probably should be uintptr_t given the way the type is used.
Description was changed from ========== Add phase/activity annotations to stack samples. A bit-field of "phases" and "activities" are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 ========== to ========== Add process lifetime annotations to stack samples. Annotations are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 ==========
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/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider.h:40: enum Activities : int { On 2016/11/17 17:23:21, Mike Wittman wrote: > On 2016/11/16 23:10:49, bcwhite wrote: > > On 2016/11/16 19:10:24, Alexei Svitkine (very slow) wrote: > > > Nit: Remove from this CL. > > > > Doh! I thought I got them all. <sigh> > > Done. > > Also update the CL description. Done. https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), On 2016/11/17 17:23:21, Mike Wittman wrote: > On 2016/11/16 23:10:49, bcwhite wrote: > > On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > > > What's the advantage of this form of input data initialization vs. the > > previous > > > style? > > > > This is far more flexible because you're not limited to having everything the > > same size as with arrays of arrays. > > > > It's much more readable, at least in my opinion. > > > > It matches the order in which things happen during a real collection so it's > > easier to correlate. > > > > It's shorter and self-documenting. > > Sounds reasonable. Macros are discouraged by C++ style though, and generally > avoided in Chrome when a pure C++ implementation is viable. Can you translate > this to use a builder pattern? I wouldn't have used them if it wasn't just a test. Switching to the "builder" pattern is possible; I chose this because it creates compile-time constants rather than creating them at run-time. Not too difficult to switch if you feel its important. https://codereview.chromium.org/2444143002/diff/500001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/500001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:32: using ProfilesGenerator = uint64_t; On 2016/11/17 17:23:21, Mike Wittman wrote: > This probably should be uintptr_t given the way the type is used. I guess it could. I originally thought I'd be storing the 64-bit MD5 prefix but that's only for verification. Done.
https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), On 2016/11/17 17:43:19, bcwhite wrote: > On 2016/11/17 17:23:21, Mike Wittman wrote: > > On 2016/11/16 23:10:49, bcwhite wrote: > > > On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > > > > What's the advantage of this form of input data initialization vs. the > > > previous > > > > style? > > > > > > This is far more flexible because you're not limited to having everything > the > > > same size as with arrays of arrays. > > > > > > It's much more readable, at least in my opinion. > > > > > > It matches the order in which things happen during a real collection so it's > > > easier to correlate. > > > > > > It's shorter and self-documenting. > > > > Sounds reasonable. Macros are discouraged by C++ style though, and generally > > avoided in Chrome when a pure C++ implementation is viable. Can you translate > > this to use a builder pattern? > > I wouldn't have used them if it wasn't just a test. Switching to the "builder" > pattern is possible; I chose this because it creates compile-time constants > rather than creating them at run-time. Not too difficult to switch if you feel > its important. I think it's worth it to eliminate the macros and to encapsulate the type punning, which is not immediately easy to grok.
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: 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...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #18 (id:560001) 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...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #18 (id:580001) 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/2444143002/diff/480001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/480001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:264: GEN_NEW_PROFILE(100, 10), On 2016/11/17 17:51:27, Mike Wittman wrote: > On 2016/11/17 17:43:19, bcwhite wrote: > > On 2016/11/17 17:23:21, Mike Wittman wrote: > > > On 2016/11/16 23:10:49, bcwhite wrote: > > > > On 2016/11/16 22:36:09, Mike Wittman - OOO to Nov.15 wrote: > > > > > What's the advantage of this form of input data initialization vs. the > > > > previous > > > > > style? > > > > > > > > This is far more flexible because you're not limited to having everything > > the > > > > same size as with arrays of arrays. > > > > > > > > It's much more readable, at least in my opinion. > > > > > > > > It matches the order in which things happen during a real collection so > it's > > > > easier to correlate. > > > > > > > > It's shorter and self-documenting. > > > > > > Sounds reasonable. Macros are discouraged by C++ style though, and generally > > > avoided in Chrome when a pure C++ implementation is viable. Can you > translate > > > this to use a builder pattern? > > > > I wouldn't have used them if it wasn't just a test. Switching to the > "builder" > > pattern is possible; I chose this because it creates compile-time constants > > rather than creating them at run-time. Not too difficult to switch if you > feel > > its important. > > I think it's worth it to eliminate the macros and to encapsulate the type > punning, which is not immediately easy to grok. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the test changes. This looks good, with the following suggestions. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:61: using Generator = uintptr_t; This and the following enum can be made private. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:105: ProfilesGenerator& End() { I think this function can be removed (based on comment below). https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:110: void GenerateProfiles(Profiles* profiles); If we change this to Profiles Build() const; then the callers can be simplified: Profiles profiles = ProfilesGenerator() .NewProfile(100, 10) /* ... */ .Build(); This should be the same cost due to RVO. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:123: while (true) { We should be able to eliminate kEndOfGenerator now, since we know the size of the array in this function.
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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:61: using Generator = uintptr_t; On 2016/11/23 00:28:40, Mike Wittman wrote: > This and the following enum can be made private. Done. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:105: ProfilesGenerator& End() { On 2016/11/23 00:28:40, Mike Wittman wrote: > I think this function can be removed (based on comment below). Done. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:110: void GenerateProfiles(Profiles* profiles); On 2016/11/23 00:28:40, Mike Wittman wrote: > If we change this to > Profiles Build() const; > > then the callers can be simplified: > Profiles profiles = ProfilesGenerator() > .NewProfile(100, 10) > /* ... */ > .Build(); > > This should be the same cost due to RVO. I like it! Except... oh. <sigh> I guess I should do away with the generator_ vector completely and just build an internal Profiles object on the fly. https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:123: while (true) { On 2016/11/23 00:28:40, Mike Wittman wrote: > We should be able to eliminate kEndOfGenerator now, since we know the size of > the array in this function. Done.
lgtm Thanks! https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/2444143002/diff/600001/components/metrics/cal... components/metrics/call_stack_profile_metrics_provider_unittest.cc:110: void GenerateProfiles(Profiles* profiles); On 2016/11/23 14:45:53, bcwhite wrote: > On 2016/11/23 00:28:40, Mike Wittman wrote: > > If we change this to > > Profiles Build() const; > > > > then the callers can be simplified: > > Profiles profiles = ProfilesGenerator() > > .NewProfile(100, 10) > > /* ... */ > > .Build(); > > > > This should be the same cost due to RVO. > > I like it! > > Except... oh. <sigh> I guess I should do away with the generator_ vector > completely and just build an internal Profiles object on the fly. Nice, this is much cleaner.
bcwhite@chromium.org changed reviewers: + palmer@chromium.org
+palmer (SECURITY_OWNERS) for: components/metrics/public/... The Sample type was changed from an array of frames to a structure containing an array of frames plus the "process_phases" annotation.
lgtm
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2444143002/#ps620001 (title: "abandon generator for on-the-fly Profiles creation")
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": 620001, "attempt_start_ts": 1479935097933350,
"parent_rev": "b6a935e8e3bb8f4d3c0a626844079b4c22fd830e", "commit_rev":
"5f9c035e32b8376afc0673259d8f318176191069"}
Message was sent while issue was closed.
Description was changed from ========== Add process lifetime annotations to stack samples. Annotations are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 ========== to ========== Add process lifetime annotations to stack samples. Annotations are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Add process lifetime annotations to stack samples. Annotations are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 ========== to ========== Add process lifetime annotations to stack samples. Annotations are passed in the proto along with the stack frames so analysis can correlate with other things happening on the system. BUG=657012 Committed: https://crrev.com/30c14f27cf63a48b082efe67ae29bb6e4282cbd7 Cr-Commit-Position: refs/heads/master@{#434273} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/30c14f27cf63a48b082efe67ae29bb6e4282cbd7 Cr-Commit-Position: refs/heads/master@{#434273} |
