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.
+      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) {

[Alexei Svitkine (slow), 2015/10/09 17:27:21]

Nit: Is this setter used?

[dhsharp, 2015/10/09 18:29:48]

It's used in an upcoming change to set the value from Finch.

+        max_collection_delay_ = delay.ToInternalValue();
+      }
+     private:
+      TriggerParams() = default;  // POD
+      int64 sampling_factor_;
+      TimeDeltaInternalType max_collection_delay_;
+    };

[Alexei Svitkine (slow), 2015/10/09 17:27:21]

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.

[dhsharp, 2015/10/09 18:29:48]

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?

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

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.

+
+    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() {
+      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.
   void ParseOutputProtoIfValid(
       scoped_ptr<WindowedIncognitoObserver> incognito_observer,
       scoped_ptr<SampledProfile> sampled_profile,
       int result,
       const std::vector<uint8>& perf_data,
       const std::vector<uint8>& perf_stat);
 
  private:
+  static const CollectionParams kDefaultParameters;
+
   // Class that listens for changes to the login state. When a normal user logs
   // in, it updates PerfProvider to start collecting data.
   class LoginObserver : public chromeos::LoginState::Observer {
    public:
     explicit LoginObserver(PerfProvider* perf_provider);
 
     // Called when either the login state or the logged in user type changes.
     // Activates |perf_provider_| to start collecting.
     void LoggedInStateChanged() override;
 
@@ skipping 38 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_