Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1399703002:
PerfProvider: Restructure collection parameters into a struct (Closed)
|
Created:
5 years, 2 months ago by dhsharp Modified:
5 years, 2 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerfProvider: Restructure collection parameters into a struct
Move collection parameters from several constants into a structure member
of PerfProvider. This is the first step towards having these values
configurable by Variation parameters.
Use base::TimeDelta for the time parameters, and add a helper function
RandomTimeTelta to compute random times.
BUG=538759
Committed: https://crrev.com/b4acf656ed24327849a7ce49f6676b2e8a60cf9e
Cr-Commit-Position: refs/heads/master@{#353396}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments on PS1 #
Total comments: 14
Patch Set 3 : Address comments on PS2 #
Total comments: 2
Patch Set 4 : Remove unused include used for debugging #
Messages
Total messages: 14 (3 generated)
dhsharp@chromium.org changed reviewers: + asvitkine@chromium.org
https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:22: PerfProvider::CollectionParams::CollectionParams( Nit: Move this below the anon namespace decl. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:31: } Nit: Add an empty line below. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:39: const PerfProvider::CollectionParams PerfProvider::kDefaultParameters( I'd not make this a static member but instead just define it in the anon namespace as a file local. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:42: /*resume_from_suspend*/ PerfProvider::CollectionParams::TriggerParams( Nit: I'd prefer the comments to be: /* resume_from_suspend = */ https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.h:56: void set_max_collection_delay(base::TimeDelta delay) { Nit: Is this setter used? https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.h:63: }; How about just making these structs with const fields that are only initialized by the ctor? Then, you don't need all the extra getters and only need to add member functions for things tht do computations, such as the TimeDelta functions.
https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:22: PerfProvider::CollectionParams::CollectionParams( On 2015/10/09 17:27:21, Alexei Svitkine wrote: > Nit: Move this below the anon namespace decl. Done. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:31: } On 2015/10/09 17:27:21, Alexei Svitkine wrote: > Nit: Add an empty line below. Oops. Done. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:39: const PerfProvider::CollectionParams PerfProvider::kDefaultParameters( On 2015/10/09 17:27:21, Alexei Svitkine wrote: > I'd not make this a static member but instead just define it in the anon > namespace as a file local. I had been thinking I needed access to it from tests, but I ended up not doing that. Done. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.cc:42: /*resume_from_suspend*/ PerfProvider::CollectionParams::TriggerParams( On 2015/10/09 17:27:21, Alexei Svitkine wrote: > Nit: I'd prefer the comments to be: > > /* resume_from_suspend = */ Done. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.h:56: void set_max_collection_delay(base::TimeDelta delay) { On 2015/10/09 17:27:21, Alexei Svitkine wrote: > Nit: Is this setter used? It's used in an upcoming change to set the value from Finch. https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.h:63: }; On 2015/10/09 17:27:21, Alexei Svitkine wrote: > How about just making these structs with const fields that are only initialized > by the ctor? > > Then, you don't need all the extra getters and only need to add member functions > for things tht do computations, such as the TimeDelta functions. I'm not sure I know what you're suggesting. These fields will need to be altered by Finch later, in an upcoming change. Finch doesn't have to overwrite all the fields if they aren't specified in the params, though, so making a new struct and copying into PerfProvider::collection_params_ would be inefficient. I also would like to point out that one of the features of this class is to be POD so that kDefaultParameters can be statically initialized. Making the fields const and initializing them in the constructor might make this class non-POD, unless it also has a defaulted constructor... but with const fields that doesn't make sense, as you would never be able to fill them in (without const_cast). If you're suggesting the fields be TimeDeltas, this is where I started. But that doesn't work, because TimeDelta is not a POD, making this class non-POD. (I also went through a lengthy process to try to make TimeDelta a POD, or an alternative TimeDeltaPOD class, but this didn't work out.) If you're suggesting I make the TimeDeltaInternalType members public, I don't like this, since the only correct way to use them is converting them to and from TimeDelta. I much prefer the encapsulation of the getters and setters. It seems to me it's the use case that setters and getters were actually designed for. Or did you mean something else?
https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/1/chrome/browser/metrics/perf... chrome/browser/metrics/perf/perf_provider_chromeos.h:63: }; On 2015/10/09 18:29:48, dhsharp wrote: > On 2015/10/09 17:27:21, Alexei Svitkine wrote: > > How about just making these structs with const fields that are only > initialized > > by the ctor? > > > > Then, you don't need all the extra getters and only need to add member > functions > > for things tht do computations, such as the TimeDelta functions. > > I'm not sure I know what you're suggesting. > > These fields will need to be altered by Finch later, in an upcoming change. > Finch doesn't have to overwrite all the fields if they aren't specified in the > params, though, so making a new struct and copying into > PerfProvider::collection_params_ would be inefficient. > > I also would like to point out that one of the features of this class is to be > POD so that kDefaultParameters can be statically initialized. Making the fields > const and initializing them in the constructor might make this class non-POD, > unless it also has a defaulted constructor... but with const fields that doesn't > make sense, as you would never be able to fill them in (without const_cast). > > If you're suggesting the fields be TimeDeltas, this is where I started. But that > doesn't work, because TimeDelta is not a POD, making this class non-POD. (I also > went through a lengthy process to try to make TimeDelta a POD, or an alternative > TimeDeltaPOD class, but this didn't work out.) > > If you're suggesting I make the TimeDeltaInternalType members public, I don't > like this, since the only correct way to use them is converting them to and from > TimeDelta. I much prefer the encapsulation of the getters and setters. It seems > to me it's the use case that setters and getters were actually designed for. > > Or did you mean something else? I was hoping the header file could be made much more concise by avoiding the need for getters. However, you raise good points that they can't be const given you want to set them (in an upcoming CL), and that you don't want to expose the underlying TimeDeltaInternalType members. Fair enough. I take back this suggestion. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.cc:32: const base::TimeDelta kMinIntervalBetweenSessionRestoreCollections = I'm not sure this is ok to do - I think this generates a stiatic initializer which is not allowed. I'd just define this as an int and in seconds and do the conversion inline. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.cc:90: const PerfProvider::CollectionParams kDefaultParameters( This should be in the anon namespace above. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:48: // Limit the number of profiles collected. Nit: Put the comments above the member variables and you can leave the getters/setters without comments. Same throughout. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:58: } Nit: Add an empty line below this. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:60: TriggerParams() = default; // POD Nit: Add an empty line below this. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:61: int64 sampling_factor_; Nit: In new code, int64_t is preferred. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:98: TriggerParams& mutable_restore_session() { Non-const refs are discouraged. This should return a pointer, as protos do. However, given your current CL is not using these, suggest removing all of these mutable/setters and re-adding them in the CL that actually integrations with variations params.
https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right): https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.cc:32: const base::TimeDelta kMinIntervalBetweenSessionRestoreCollections = On 2015/10/09 19:24:32, Alexei Svitkine wrote: > I'm not sure this is ok to do - I think this generates a stiatic initializer > which is not allowed. I'd just define this as an int and in seconds and do the > conversion inline. You're right. This is left over from when I thought I could turn TimeDelta into POD. Done. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.cc:90: const PerfProvider::CollectionParams kDefaultParameters( On 2015/10/09 19:24:32, Alexei Svitkine wrote: > This should be in the anon namespace above. This actually doesn't compile (did I forget to try it before uploading?). The compiler just reminded me why I made kDefaultParameters a static member of PerfProvider in the first place: If it's global, then CollectionParams has to be public, and I think it's better if it's not public. (And if kDefaultParamters is static, then it can't be defined inside the anon namespace--must be same namespace as PerfProvider). So, I'm putting this back to a private static member of PerfProvider. The alternative is to make CollectionParams public. Let me know if you'd prefer that. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:48: // Limit the number of profiles collected. On 2015/10/09 19:24:32, Alexei Svitkine wrote: > Nit: Put the comments above the member variables and you can leave the > getters/setters without comments. Same throughout. Done. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:58: } On 2015/10/09 19:24:32, Alexei Svitkine wrote: > Nit: Add an empty line below this. Done. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:60: TriggerParams() = default; // POD On 2015/10/09 19:24:32, Alexei Svitkine wrote: > Nit: Add an empty line below this. Done. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:61: int64 sampling_factor_; On 2015/10/09 19:24:32, Alexei Svitkine wrote: > Nit: In new code, int64_t is preferred. Done. https://codereview.chromium.org/1399703002/diff/20001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:98: TriggerParams& mutable_restore_session() { On 2015/10/09 19:24:32, Alexei Svitkine wrote: > Non-const refs are discouraged. This should return a pointer, as protos do. D'oh, poor memory. You're right I was trying to emulate protos. Done. > > However, given your current CL is not using these, suggest removing all of these > mutable/setters and re-adding them in the CL that actually integrations with > variations params. That change is pretty substantial already. I think I'm doing you a favor by leaving this in this change. $ git show --stat cwp-finch <snip> chrome/browser/metrics/perf/perf_provider_chromeos.cc | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++- chrome/browser/metrics/perf/perf_provider_chromeos.h | 10 ++++ chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc | 253 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- chrome/browser/metrics/perf/random_selector.cc | 16 +++++- chrome/browser/metrics/perf/random_selector.h | 18 +++++-- chrome/browser/metrics/perf/random_selector_unittest.cc | 42 ++++++++++++++- 6 files changed, 478 insertions(+), 10 deletions(-)
lgtm https://codereview.chromium.org/1399703002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:9: #include <type_traits> Out of curiosity, what's this for?
I've uploaded (but not sent for review) the next two changes that build on this one for your reference. I wasn't planning to ask for review until the previous change is submitted. PerfProvider: Run a randomly selected perf command https://codereview.chromium.org/1395273002/ PerfProvider: Get collection parameters from Finch https://codereview.chromium.org/1392153003/
https://codereview.chromium.org/1399703002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/perf/perf_provider_chromeos.h (right): https://codereview.chromium.org/1399703002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/perf/perf_provider_chromeos.h:9: #include <type_traits> On 2015/10/09 21:23:27, Alexei Svitkine wrote: > Out of curiosity, what's this for? Leftovers... I'll remove it. I had been using: static_assert(std::is_pod<CollectionParams>::value, "CollectionParams must be POD"); But, since std::is_pod is a C++11 library feature, it is not allowed. Was still a nice check locally.
The CQ bit was checked by dhsharp@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/1399703002/#ps60001 (title: "Remove unused include used for debugging")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1399703002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1399703002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b4acf656ed24327849a7ce49f6676b2e8a60cf9e Cr-Commit-Position: refs/heads/master@{#353396} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
