PerfProvider: Restructure collection parameters into a struct

chrome/browser/metrics/perf/perf_provider_chromeos.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_METRICS_PERF_PERF_PROVIDER_CHROMEOS_H_
 #define CHROME_BROWSER_METRICS_PERF_PERF_PROVIDER_CHROMEOS_H_
 
 #include <string>
+#include <type_traits>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/sessions/session_restore.h"
 #include "chromeos/dbus/power_manager_client.h"
 #include "chromeos/login/login_state.h"
 #include "components/metrics/proto/sampled_profile.pb.h"
 
 namespace metrics {
 
 class WindowedIncognitoObserver;
 
 // Provides access to ChromeOS perf data. perf aka "perf events" is a
 // performance profiling infrastructure built into the linux kernel. For more
 // information, see: https://perf.wiki.kernel.org/index.php/Main_Page.
 class PerfProvider : public base::NonThreadSafe,
                      public chromeos::PowerManagerClient::Observer {
  public:
   PerfProvider();
   ~PerfProvider() override;
 
   // Stores collected perf data protobufs in |sampled_profiles|. Clears all the
   // stored profile data. Returns true if it wrote to |sampled_profiles|.
   bool GetSampledProfiles(std::vector<SampledProfile>* sampled_profiles);
 
  protected:
+  typedef int64 TimeDeltaInternalType;
+
+  class CollectionParams {
+   public:
+    class TriggerParams {
+     public:
+      TriggerParams(int64 sampling_factor,
+                    base::TimeDelta max_collection_delay);
+
+      // Limit the number of profiles collected.

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

Nit: Put the comments above the member variables and you can leave the
getters/setters without comments. Same throughout.

[dhsharp, 2015/10/09 21:18:35]

Done.

+      int64 sampling_factor() const { return sampling_factor_; }
+      void set_sampling_factor(int64 factor) { sampling_factor_ = factor; }
+      // Add a random delay before collecting after the trigger.
+      // The delay should be randomly selected between 0 and this value.
+      base::TimeDelta max_collection_delay() const {
+        return base::TimeDelta::FromInternalValue(max_collection_delay_);
+      }
+      void set_max_collection_delay(base::TimeDelta delay) {
+        max_collection_delay_ = delay.ToInternalValue();
+      }

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

Nit: Add an empty line below this.

[dhsharp, 2015/10/09 21:18:34]

Done.

+     private:
+      TriggerParams() = default;  // POD

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

Nit: Add an empty line below this.

[dhsharp, 2015/10/09 21:18:34]

Done.

+      int64 sampling_factor_;

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

Nit: In new code, int64_t is preferred.

[dhsharp, 2015/10/09 21:18:34]

Done.

+      TimeDeltaInternalType max_collection_delay_;
+    };
+
+    CollectionParams(base::TimeDelta collection_duration,
+                     base::TimeDelta periodic_interval,
+                     TriggerParams resume_from_suspend,
+                     TriggerParams restore_session);
+
+    // Time perf is run for.
+    base::TimeDelta collection_duration() const {
+      return base::TimeDelta::FromInternalValue(collection_duration_);
+    }
+    void set_collection_duration(base::TimeDelta duration) {
+      collection_duration_ = duration.ToInternalValue();
+    }
+
+    // For PERIODIC_COLLECTION, partition time since login into successive
+    // intervals of this duration. In each interval, a random time is picked to
+    // collect a profile.
+    base::TimeDelta periodic_interval() const {
+      return base::TimeDelta::FromInternalValue(periodic_interval_);
+    }
+    void set_periodic_interval(base::TimeDelta interval) {
+      periodic_interval_ = interval.ToInternalValue();
+    }
+
+    // Parameters for RESUME_FROM_SUSPEND and RESTORE_SESSION collections:
+    const TriggerParams& resume_from_suspend() const {
+      return resume_from_suspend_;
+    }
+    TriggerParams& mutable_resume_from_suspend() {
+      return resume_from_suspend_;
+    }
+    const TriggerParams& restore_session() const {
+      return restore_session_;
+    }
+    TriggerParams& mutable_restore_session() {

[Alexei Svitkine (slow), 2015/10/09 19:24:32]

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.

[dhsharp, 2015/10/09 21:18:34]

D'oh, poor memory. You're right I was trying to emulate protos. Done.
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(-)

+      return restore_session_;
+    }
+
+   private:
+    CollectionParams() = default;  // POD
+
+    TimeDeltaInternalType collection_duration_;
+    TimeDeltaInternalType periodic_interval_;
+    TriggerParams resume_from_suspend_;
+    TriggerParams restore_session_;
+  };
+
   // Parses a PerfDataProto from serialized data |perf_data|, if it exists.
   // Parses a PerfStatProto from serialized data |perf_stat|, if it exists.
   // Only one of these may contain data. If both |perf_data| and |perf_stat|
   // contain data, it is counted as an error and neither is parsed.
   // |incognito_observer| indicates whether an incognito window had been opened
   // during the profile collection period. If there was an incognito window,
   // discard the incoming data.
   // |trigger_event| is the cause of the perf data collection.
   // |result| is the return value of running perf/quipper. It is 0 if successful
   // and nonzero if not successful.
@@ skipping 56 matching lines @@
   void CollectPerfDataAfterResume(const base::TimeDelta& sleep_duration,
                                   const base::TimeDelta& time_after_resume);
 
   // Collects perf data after a session restore. |time_after_restore| is how
   // long ago the session restore started. |num_tabs_restored| is the total
   // number of tabs being restored.
   void CollectPerfDataAfterSessionRestore(
       const base::TimeDelta& time_after_restore,
       int num_tabs_restored);
 
+  // Parameters controlling how profiles are collected.
+  CollectionParams collection_params_;
+
   // Vector of SampledProfile protobufs containing perf profiles.
   std::vector<SampledProfile> cached_perf_data_;
 
   // For scheduling collection of perf data.
   base::OneShotTimer timer_;
 
   // For detecting when changes to the login state.
   LoginObserver login_observer_;
 
   // Record of the last login time.
@@ skipping 15 matching lines @@
 
   // To pass around the "this" pointer across threads safely.
   base::WeakPtrFactory<PerfProvider> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PerfProvider);
 };
 
 }  // namespace metrics
 
 #endif  // CHROME_BROWSER_METRICS_PERF_PERF_PROVIDER_CHROMEOS_H_