Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
981143006:
Metrics provider for statistical stack profiler (Closed)
|
Created:
5 years, 9 months ago by Mike Wittman Modified:
5 years, 9 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, danduong, Alexei Svitkine (slow), cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMetrics provider for statistical stack profiler
Provides a metrics provider that uploads statistical stack profiling
state via UMA.
This CL builds on top of the statistical profiler implementation in
https://crrev.com/1016563004, which is under review.
BUG=464929
Committed: https://crrev.com/d19f5200062542283ffc6ab281ea961e481b5846
Cr-Commit-Position: refs/heads/master@{#321928}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : fix clang compilation #
Total comments: 22
Patch Set 4 : restrict changes to metrics provider #Patch Set 5 : address comments #
Total comments: 53
Patch Set 6 : address isherman comments #Patch Set 7 : address asvitkine comments #Patch Set 8 : handle unknown module (per other review) #
Total comments: 12
Patch Set 9 : address comments #Patch Set 10 : address comments #Patch Set 11 : update per latest StackSamplingProfiler changes #Patch Set 12 : rebase #Patch Set 13 : fix compilation error #Patch Set 14 : fix multiline comment #Messages
Total messages: 43 (15 generated)
wittman@chromium.org changed reviewers: + cpu@chromium.org
wittman@chromium.org changed reviewers: + isherman@chromium.org
PTAL cpu: base isherman: chrome/browser/metrics, components/metrics The code you both reviewed in https://codereview.chromium.org/888923004 is in patch set 1, for what it's worth. I've substantially reworked most of the implementation since then.
Also, Ilya: what's the best way to test the CallStackProfileMetricsProvider code? ProvideGeneralMetrics doesn't appear to be invoked, even with --metrics-recording-only.
Can you add alexei svitkine too? Sent from my iPhone > On Mar 6, 2015, at 5:29 PM, "wittman@chromium.org" <wittman@chromium.org> wrote: > > Also, Ilya: what's the best way to test the CallStackProfileMetricsProvider > code? ProvideGeneralMetrics doesn't appear to be invoked, even with > --metrics-recording-only. > > https://codereview.chromium.org/981143006/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+asvitkine
can you please split this into at least two CLs, 1. Just the things that go in base and their tests. That is, if you want to have this permanently in base it needs tests. 2. The rest which could be one or two CLs, the components/ and the chrome/ part.
On 2015/03/07 01:29:44, Mike Wittman wrote: > Also, Ilya: what's the best way to test the CallStackProfileMetricsProvider > code? ProvideGeneralMetrics doesn't appear to be invoked, even with > --metrics-recording-only. In unit tests, you should call ProvideGeneralMetrics directly. Please do add unit tests. If you mean manual testing, then you'll need to wait for an UMA log to be cut, which takes about 30 seconds. In a local build, you can find the appropriate constant and set it to something lower. I think it might be this constant: https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:13: using metrics::CallStackEntry; nit: Please alphabetize https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:18: namespace { nit: Please include a blank line after this one. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:29: CallStackProfile::Sample* proto_sample) { nit: Please document all functions, including local ones. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:45: if (!current_sample_proto || *it != *(it - 1)) { Hmm, this seems like a surprising design. Why doesn't the Profile use a map representation to count the number of samples for each function? https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:297: call_stack_profile_metrics_provider_ = new CallStackProfileMetricsProvider; I don't see any reason to cache this pointer (nor the one above, honestly). Do you? Note: If it *is* important to cache it, then please make sure to initialize the variable in the constructor. And, yeah, we should probably initialize the other profiler var as well, if we keep it around. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:19: repeated CallStackEntry entries = 1; I'm pretty sure that the protobuf convention is to name repeated fields as singular, i.e. "entry" rather than "entries". This makes things like "add_entry()" look sensible, whereas "add_entries()" looks odd. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:55: } Has this protocol buffer already been reviewed and landed in google3? That is a prerequisite for checking it in to the Chromium repository. I can initially review it here, if you prefer, but keep in mind that the google3 CL absolutely needs to be reviewed by the internal logging and privacy teams prior to landing this Chromium CL. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... File components/metrics/proto/sampled_profile.proto (right): https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:12: import "call_stack_profile.proto"; nit: Please alphabetize. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:63: // Sample profile data collected from Linux perf tool. nit: "Sample" -> "Sampled" https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:66: // Sample profile data collected by means other than Linux perf. nit: This is too vague. Please be more specific. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:67: optional CallStackProfile call_stack_profile = 9; Note that the same comments w.r.t. landing in google3 first apply to this CL as well.
On 2015/03/10 01:44:47, Ilya Sherman wrote: > On 2015/03/07 01:29:44, Mike Wittman wrote: > > Also, Ilya: what's the best way to test the CallStackProfileMetricsProvider > > code? ProvideGeneralMetrics doesn't appear to be invoked, even with > > --metrics-recording-only. > > In unit tests, you should call ProvideGeneralMetrics directly. Please do add > unit tests. > > If you mean manual testing, then you'll need to wait for an UMA log to be cut, > which takes about 30 seconds. In a local build, you can find the appropriate > constant and set it to something lower. I think it might be this constant: > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... Oh, I'm realizing that UMA metrics reporting is also disabled in developer builds. You might need to stub out this function as well: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
wittman@chromium.org changed reviewers: - cpu@chromium.org
On 2015/03/09 20:56:40, cpu wrote: > can you please split this into at least two CLs, > > 1. Just the things that go in base and their tests. That is, if you want to have > this permanently in base it needs tests. > > 2. The rest which could be one or two CLs, the components/ and the chrome/ part. Sure. I'll use this CL for (2) since Ilya has already commented on the metrics provider implementation. I've split (1) off into https://crrev.com/1016563004 and added tests. On 2015/03/10 23:40:13, Ilya Sherman wrote: > On 2015/03/10 01:44:47, Ilya Sherman wrote: > > On 2015/03/07 01:29:44, Mike Wittman wrote: > > > Also, Ilya: what's the best way to test the CallStackProfileMetricsProvider > > > code? ProvideGeneralMetrics doesn't appear to be invoked, even with > > > --metrics-recording-only. > > > > In unit tests, you should call ProvideGeneralMetrics directly. Please do add > > unit tests. > > > > If you mean manual testing, then you'll need to wait for an UMA log to be cut, > > which takes about 30 seconds. In a local build, you can find the appropriate > > constant and set it to something lower. I think it might be this constant: > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... > > Oh, I'm realizing that UMA metrics reporting is also disabled in developer > builds. You might need to stub out this function as well: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met... I still wasn't able to get ProvideGeneralMetrics to be invoked, even after stubbing out that function and letting it run for several minutes. But I've added unit tests now, so manual testing isn't too important as long as we're confident the function gets invoked in release Chrome. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:13: using metrics::CallStackEntry; On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: Please alphabetize Done. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:18: namespace { On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: Please include a blank line after this one. Done. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:29: CallStackProfile::Sample* proto_sample) { On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: Please document all functions, including local ones. Done. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:45: if (!current_sample_proto || *it != *(it - 1)) { On 2015/03/10 01:44:46, Ilya Sherman wrote: > Hmm, this seems like a surprising design. Why doesn't the Profile use a map > representation to count the number of samples for each function? After further discussion, we want to support two different use cases here: profiles where the sequence of sample stacks is important and should be preserved, and profiles where the sample stacks are essentially independent and sequence is not important. I've added a preserve_sample_ordering variable to the input to distinguish between the two, and updated the code to handle the unordered case with a map. https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/981143006/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:297: call_stack_profile_metrics_provider_ = new CallStackProfileMetricsProvider; On 2015/03/10 01:44:46, Ilya Sherman wrote: > I don't see any reason to cache this pointer (nor the one above, honestly). Do > you? No, removed. > Note: If it *is* important to cache it, then please make sure to initialize the > variable in the constructor. And, yeah, we should probably initialize the other > profiler var as well, if we keep it around. All the other metrics providers are referenced within the class. I added initializers for them. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:19: repeated CallStackEntry entries = 1; On 2015/03/10 01:44:46, Ilya Sherman wrote: > I'm pretty sure that the protobuf convention is to name repeated fields as > singular, i.e. "entry" rather than "entries". This makes things like > "add_entry()" look sensible, whereas "add_entries()" looks odd. Done. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:55: } On 2015/03/10 01:44:46, Ilya Sherman wrote: > Has this protocol buffer already been reviewed and landed in google3? That is a > prerequisite for checking it in to the Chromium repository. I can initially > review it here, if you prefer, but keep in mind that the google3 CL absolutely > needs to be reviewed by the internal logging and privacy teams prior to landing > this Chromium CL. Yes, and re-reviewed with suggested changes. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... File components/metrics/proto/sampled_profile.proto (right): https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:12: import "call_stack_profile.proto"; On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: Please alphabetize. Done. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:63: // Sample profile data collected from Linux perf tool. On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: "Sample" -> "Sampled" Done. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:66: // Sample profile data collected by means other than Linux perf. On 2015/03/10 01:44:46, Ilya Sherman wrote: > nit: This is too vague. Please be more specific. Done. https://codereview.chromium.org/981143006/diff/40001/components/metrics/proto... components/metrics/proto/sampled_profile.proto:67: optional CallStackProfile call_stack_profile = 9; On 2015/03/10 01:44:46, Ilya Sherman wrote: > Note that the same comments w.r.t. landing in google3 first apply to this CL as > well. Acknowledged.
Thanks, Mike. https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:30: return *reinterpret_cast<uint64*>(&md5.a[0]); Are you sure this is endianness-safe? Also, IIRC, reinterpret_cast has undefined behavior when used in this way. Seems like it might be wise to re-use the existing hashing code, by moving the implementation into a location where it can be shared. https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:34: // compute module IP offsets. Optional nit: I assume that "IP" is "instruction pointer"; but in Chrome code, it might as easily be "Internet protocol". Up to you whether you want to disambiguate. https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:57: if (profile.preserve_sample_ordering) { It's still not clear to me, given that all of the decisions seem to be made at the base layer, why the base layer doesn't just use a data structure that pre-computes this representation. Besides making the metrics code simpler (which is probably a wash, since it makes the base code more complex), it would save on memory use, which seems like a definite win. https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:73: auto loc = sample_index.find(*it); nit: Please spell out "location" https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... File chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:17: using Sample = StackSamplingProfiler::Sample; nit: Why the different syntax on lines 14-17 vs. line 13? https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:98: const Frame profile_sample_frames[][2][3] = { It's not immediately clear to me what this variable represents. Could you please add a comment? https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:136: for (size_t i = 0; i < arraysize(profile_sample_frames); i++) { nit: ++i, ++j below https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:158: new CallStackProfileMetricsProvider); nit: Why not stack-allocate? https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:187: const char *instruction_pointer = reinterpret_cast<const char *>( nit: "char *" -> "char* " https://chromiumcodereview.appspot.com/981143006/diff/80001/chrome/browser/me... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:220: } This test is super long and has a lot of nested loops, which makes it rather hard to follow. It probably tests the right thing, but if a bug were to slip in(to the test code), it would be easy to miss. I wonder whether it might be at least a little easier to follow if the loops were unrolled, and if some of the constants were moved to the top of the file -- WDYT? https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... File components/metrics/proto/call_stack_profile.proto (right): https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:15: nit: Please replace this newline with documentation. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:19: repeated CallStackEntry entry = 1; nit: Please leave a blank line after this one. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:25: repeated Sample sample = 1; nit: Please leave a blank line here. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:30: optional int32 profile_duration_ms = 3; nit: Please leave a blank line here. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:38: optional uint64 address = 1; nit: Please leave a blank line here. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:41: } Why are some of the messages declared as inner message types, and others as outer ones? I think the general preference is to use inner message types where possible. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:51: optional bytes build_id = 1; nit: Please leave a blank line here. https://chromiumcodereview.appspot.com/981143006/diff/80001/components/metric... components/metrics/proto/call_stack_profile.proto:51: optional bytes build_id = 1; "bytes" fields are generally discouraged, same as string ones. Do we really need the flexibility that "bytes" provides here?
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:16: using metrics::CallStackEntry; If this file is moved to metrics component, all these using declaration can be removed. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:25: uint64 HashModuleFilename(const base::FilePath& filename) { Can you just use HashMetricName()? https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:114: for (StackSamplingProfiler::Profile profile : profiles) { Nit: const ref https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.h:18: class CallStackProfileMetricsProvider : public metrics::MetricsProvider { If this only depends on code in base and the metrics component, let's put in the component. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.h:30: void SetSourceProfilesForTest( Nit: metrics code prefers ForTesting https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:30: #if defined(OS_WIN) Do the actual paths matter? If not, I suggest simplifying by using FILE_PATH_LITERAL to remove the ifdef. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:111: #if defined(ENABLE_PLUGINS) FYI: If https://codereview.chromium.org/999623002/ lands first, you'll have a merge conflict.
https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:30: return *reinterpret_cast<uint64*>(&md5.a[0]); On 2015/03/17 04:54:35, Ilya Sherman wrote: > Are you sure this is endianness-safe? Also, IIRC, reinterpret_cast has > undefined behavior when used in this way. Seems like it might be wise to re-use > the existing hashing code, by moving the implementation into a location where it > can be shared. Looks like I can reuse the existing code as-is. Also updated the code to avoid undefined behavior, although it seems a net loss in readability. (reinterpret_cast to a pointer of a different type is used all across the Chrome codebase AFAICT.) https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:34: // compute module IP offsets. On 2015/03/17 04:54:34, Ilya Sherman wrote: > Optional nit: I assume that "IP" is "instruction pointer"; but in Chrome code, > it might as easily be "Internet protocol". Up to you whether you want to > disambiguate. Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:57: if (profile.preserve_sample_ordering) { On 2015/03/17 04:54:35, Ilya Sherman wrote: > It's still not clear to me, given that all of the decisions seem to be made at > the base layer, why the base layer doesn't just use a data structure that > pre-computes this representation. Besides making the metrics code simpler > (which is probably a wash, since it makes the base code more complex), it would > save on memory use, which seems like a definite win. It could be computed in base, but it's not clear yet what representation will work best for all possible users of collected stacks (in-proc as well as UMA). It's possible we may want an even more aggressive compression scheme than this. We're initially targeting just getting some representative data from a small percentage of Win x64 users on startup. Can we table the decision on the compression approach until we have that data and can make an informed decision? https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:73: auto loc = sample_index.find(*it); On 2015/03/17 04:54:34, Ilya Sherman wrote: > nit: Please spell out "location" Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:17: using Sample = StackSamplingProfiler::Sample; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Why the different syntax on lines 14-17 vs. line 13? "using X::Y" is only valid if X is a namespace. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:98: const Frame profile_sample_frames[][2][3] = { On 2015/03/17 04:54:35, Ilya Sherman wrote: > It's not immediately clear to me what this variable represents. Could you > please add a comment? Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:136: for (size_t i = 0; i < arraysize(profile_sample_frames); i++) { On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: ++i, ++j below Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:158: new CallStackProfileMetricsProvider); On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Why not stack-allocate? Stack allocation works. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:187: const char *instruction_pointer = reinterpret_cast<const char *>( On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: "char *" -> "char* " Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:220: } On 2015/03/17 04:54:35, Ilya Sherman wrote: > This test is super long and has a lot of nested loops, which makes it rather > hard to follow. It probably tests the right thing, but if a bug were to slip > in(to the test code), it would be easy to miss. I wonder whether it might be at > least a little easier to follow if the loops were unrolled, and if some of the > constants were moved to the top of the file -- WDYT? It is very long. But it also tests that there's no cross-contamination between different profiles across all the represented state, which I think is valuable and I'm not sure how to adequately test otherwise. Although the looping/indexing isn't the easiest to follow, it does have the nice property that a test bug is unlikely to work across all tested inputs simultaneously. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:15: On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please replace this newline with documentation. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:19: repeated CallStackEntry entry = 1; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:25: repeated Sample sample = 1; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please leave a blank line here. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:30: optional int32 profile_duration_ms = 3; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please leave a blank line here. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:38: optional uint64 address = 1; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please leave a blank line here. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:41: } On 2015/03/17 04:54:35, Ilya Sherman wrote: > Why are some of the messages declared as inner message types, and others as > outer ones? I think the general preference is to use inner message types where > possible. In the server-side review (cr/87065567) Sample was the only proto requested to be nested. Possibly the others may be reused? https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:51: optional bytes build_id = 1; On 2015/03/17 04:54:35, Ilya Sherman wrote: > nit: Please leave a blank line here. Done. https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:51: optional bytes build_id = 1; On 2015/03/17 04:54:35, Ilya Sherman wrote: > "bytes" fields are generally discouraged, same as string ones. Do we really > need the flexibility that "bytes" provides here? Yes. This was addressed in the server-side review: "Windows might have a fixed convention, but there is not a fixed length for build-id on linux. There's not even a requirement that it be a multiple of 4- or 8-bytes. If you want to support multiple platforms, you'll need a byte array."
Patchset #6 (id:100001) has been deleted
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:16: using metrics::CallStackEntry; On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > If this file is moved to metrics component, all these using declaration can be > removed. Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:25: uint64 HashModuleFilename(const base::FilePath& filename) { On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > Can you just use HashMetricName()? > > https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics... Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.cc:114: for (StackSamplingProfiler::Profile profile : profiles) { On 2015/03/17 23:10:09, Alexei Svitkine (away) wrote: > Nit: const ref Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.h:18: class CallStackProfileMetricsProvider : public metrics::MetricsProvider { On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > If this only depends on code in base and the metrics component, let's put in the > component. Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider.h:30: void SetSourceProfilesForTest( On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > Nit: metrics code prefers ForTesting Done. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/call_stack_profile_metrics_provider_unittest.cc:30: #if defined(OS_WIN) On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > Do the actual paths matter? > > If not, I suggest simplifying by using FILE_PATH_LITERAL to remove the ifdef. Not that much. However, even if we use FILE_PATH_LITERAL here, we still need the ifdefs in profile_expected_name_md5_prefixes due to the md5 sum over the different character representations. I'd prefer to leave the ifdefs in both places to avoid confusion over this subtlety. https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/981143006/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:111: #if defined(ENABLE_PLUGINS) On 2015/03/17 23:10:10, Alexei Svitkine (away) wrote: > FYI: If https://codereview.chromium.org/999623002/ lands first, you'll have a > merge conflict. Thanks for the heads up.
Once the .proto file settles down here, let's do a (hopefully) final update of the google3 version, and then this CL should be good to go =) https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:41: } On 2015/03/18 00:54:28, Mike Wittman wrote: > On 2015/03/17 04:54:35, Ilya Sherman wrote: > > Why are some of the messages declared as inner message types, and others as > > outer ones? I think the general preference is to use inner message types > where > > possible. > > In the server-side review (cr/87065567) Sample was the only proto requested to > be nested. Possibly the others may be reused? That's fair, but it doesn't look like the reviewer who made that suggestion was particularly familiar with the other UMA .proto files. I'm pretty sure that every other .proto file defines only a single message in the outer namespace. Please follow that convention in this file as well. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:25: const base::FilePath::StringType basename = filename.BaseName().value(); nit: Probably no need to make a copy here, right? https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:30: basename.size() * sizeof(base::FilePath::CharType)); nit: Please cache this computation, rather than performing it twice -- it would be really bad to have a mismatch between the sizes used in one location vs. the other. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:42: // A frame may not have a valid module. If so we can't compute the nit: "If so we can't" -> "If so, we can't" https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:99: // Frames for all samples for all profiles in the test. What I really meant with my previous request was, could you provide some intuition in the comment as to what the frames represent? It's hard for me to intuit from reading the code. https://codereview.chromium.org/981143006/diff/180001/components/metrics/prot... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/prot... components/metrics/proto/call_stack_profile.proto:15: nit: Please omit this newline.
https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:25: const base::FilePath::StringType basename = filename.BaseName().value(); On 2015/03/19 00:10:19, Ilya Sherman wrote: > nit: Probably no need to make a copy here, right? Done. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:30: basename.size() * sizeof(base::FilePath::CharType)); On 2015/03/19 00:10:19, Ilya Sherman wrote: > nit: Please cache this computation, rather than performing it twice -- it would > be really bad to have a mismatch between the sizes used in one location vs. the > other. Done. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:42: // A frame may not have a valid module. If so we can't compute the On 2015/03/19 00:10:19, Ilya Sherman wrote: > nit: "If so we can't" -> "If so, we can't" Done. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider_unittest.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider_unittest.cc:99: // Frames for all samples for all profiles in the test. On 2015/03/19 00:10:20, Ilya Sherman wrote: > What I really meant with my previous request was, could you provide some > intuition in the comment as to what the frames represent? It's hard for me to > intuit from reading the code. Done. https://codereview.chromium.org/981143006/diff/180001/components/metrics/prot... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/prot... components/metrics/proto/call_stack_profile.proto:15: On 2015/03/19 00:10:20, Ilya Sherman wrote: > nit: Please omit this newline. Done.
https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:41: } On 2015/03/19 00:10:19, Ilya Sherman wrote: > On 2015/03/18 00:54:28, Mike Wittman wrote: > > On 2015/03/17 04:54:35, Ilya Sherman wrote: > > > Why are some of the messages declared as inner message types, and others as > > > outer ones? I think the general preference is to use inner message types > > where > > > possible. > > > > In the server-side review (cr/87065567) Sample was the only proto requested to > > be nested. Possibly the others may be reused? > > That's fair, but it doesn't look like the reviewer who made that suggestion was > particularly familiar with the other UMA .proto files. I'm pretty sure that > every other .proto file defines only a single message in the outer namespace. > Please follow that convention in this file as well. Bump.
https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... File components/metrics/proto/call_stack_profile.proto (right): https://codereview.chromium.org/981143006/diff/80001/components/metrics/proto... components/metrics/proto/call_stack_profile.proto:41: } On 2015/03/19 02:01:16, Ilya Sherman wrote: > On 2015/03/19 00:10:19, Ilya Sherman wrote: > > On 2015/03/18 00:54:28, Mike Wittman wrote: > > > On 2015/03/17 04:54:35, Ilya Sherman wrote: > > > > Why are some of the messages declared as inner message types, and others > as > > > > outer ones? I think the general preference is to use inner message types > > > where > > > > possible. > > > > > > In the server-side review (cr/87065567) Sample was the only proto requested > to > > > be nested. Possibly the others may be reused? > > > > That's fair, but it doesn't look like the reviewer who made that suggestion > was > > particularly familiar with the other UMA .proto files. I'm pretty sure that > > every other .proto file defines only a single message in the outer namespace. > > Please follow that convention in this file as well. > > Bump. Sorry, this slipped my mind. Made the messages internal. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:25: const base::FilePath::StringType basename = filename.BaseName().value(); On 2015/03/19 00:56:58, Mike Wittman wrote: > On 2015/03/19 00:10:19, Ilya Sherman wrote: > > nit: Probably no need to make a copy here, right? > > Done. Actually, we do need to make a copy here. value() returns a reference to the path in the temporary object produced by BaseName(), which goes away.
Thanks. LG to me. Will stamp for real once you've landed the .proto changes upstream. https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... File components/metrics/call_stack_profile_metrics_provider.cc (right): https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... components/metrics/call_stack_profile_metrics_provider.cc:25: const base::FilePath::StringType basename = filename.BaseName().value(); On 2015/03/19 21:51:18, Mike Wittman wrote: > On 2015/03/19 00:56:58, Mike Wittman wrote: > > On 2015/03/19 00:10:19, Ilya Sherman wrote: > > > nit: Probably no need to make a copy here, right? > > > > Done. > > Actually, we do need to make a copy here. value() returns a reference to the > path in the temporary object produced by BaseName(), which goes away. Wait, really? Fascinating! If I'm understanding correctly, then the following two snippets have different behavior: const base::FilePath& path = filename.BaseName(); const base::FilePath::StringType& basename = path.value(); const base::FilePath::StringType& basename = filename.BaseName().value(); Dear C++, that is super confusing. (No follow-up needed, I'm just really intrigued by this C++ wart.)
On 2015/03/19 22:00:11, Ilya Sherman wrote: > Thanks. LG to me. Will stamp for real once you've landed the .proto changes > upstream. > > https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... > File components/metrics/call_stack_profile_metrics_provider.cc (right): > > https://codereview.chromium.org/981143006/diff/180001/components/metrics/call... > components/metrics/call_stack_profile_metrics_provider.cc:25: const > base::FilePath::StringType basename = filename.BaseName().value(); > On 2015/03/19 21:51:18, Mike Wittman wrote: > > On 2015/03/19 00:56:58, Mike Wittman wrote: > > > On 2015/03/19 00:10:19, Ilya Sherman wrote: > > > > nit: Probably no need to make a copy here, right? > > > > > > Done. > > > > Actually, we do need to make a copy here. value() returns a reference to the > > path in the temporary object produced by BaseName(), which goes away. > > Wait, really? Fascinating! If I'm understanding correctly, then the following > two snippets have different behavior: > > const base::FilePath& path = filename.BaseName(); > const base::FilePath::StringType& basename = path.value(); > > const base::FilePath::StringType& basename = filename.BaseName().value(); > > Dear C++, that is super confusing. (No follow-up needed, I'm just really > intrigued by this C++ wart.) Yep, the magic const-ref-extends-lifetime-of-temporary-object behavior doesn't apply when initialized from a reference. :) Seems like a good argument for defaulting to make a copy of the result of multiple function calls... even if it works now, a refactoring of an intermediate return type from const reference to value could silently cause things to break.
LGTM, thanks.
wittman@chromium.org changed reviewers: + cpu@chromium.org
Hi Carlos, can you give approval for the rebase-induced changes on base?
lgtm for /base
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/981143006/#ps260001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981143006/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, cpu@chromium.org Link to the patchset: https://codereview.chromium.org/981143006/#ps280001 (title: "fix compilation error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981143006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by wittman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, cpu@chromium.org Link to the patchset: https://codereview.chromium.org/981143006/#ps300001 (title: "fix multiline comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/981143006/300001
Message was sent while issue was closed.
Committed patchset #14 (id:300001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/d19f5200062542283ffc6ab281ea961e481b5846 Cr-Commit-Position: refs/heads/master@{#321928} |
