PerfProvider: Restructure collection parameters into a struct

chrome/browser/metrics/perf/perf_provider_chromeos.cc

 // 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.
 
 #include "chrome/browser/metrics/perf/perf_provider_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/metrics/perf/windowed_incognito_observer.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 
+namespace metrics {
+
 namespace {
 
-// Partition time since login into successive intervals of this size. In each
-// interval, pick a random time to collect a profile.
-const size_t kPerfProfilingIntervalMs = 3 * 60 * 60 * 1000;
-
-// Default time in seconds perf is run for.
-const size_t kPerfCommandDurationDefaultSeconds = 2;
-
 // Limit the total size of protobufs that can be cached, so they don't take up
 // too much memory. If the size of cached protobufs exceeds this value, stop
 // collecting further perf data. The current value is 4 MB.
 const size_t kCachedPerfDataProtobufSizeThreshold = 4 * 1024 * 1024;
 
-// There may be too many suspends to collect a profile each time there is a
-// resume. To limit the number of profiles, collect one for 1 in 10 resumes.
-// Adjust this number as needed.
-const int kResumeSamplingFactor = 10;
-
-// There may be too many session restores to collect a profile each time. Limit
-// the collection rate by collecting one per 10 restores. Adjust this number as
-// needed.
-const int kRestoreSessionSamplingFactor = 10;
-
 // This is used to space out session restore collections in the face of several
 // notifications in a short period of time. There should be no less than this
-// much time between collections. The current value is 30 seconds.
-const int kMinIntervalBetweenSessionRestoreCollectionsMs = 30 * 1000;
-
-// If collecting after a resume, add a random delay before collecting. The delay
-// should be randomly selected between 0 and this value. Currently the value is
-// equal to 5 seconds.
-const int kMaxResumeCollectionDelayMs = 5 * 1000;
-
-// If collecting after a session restore, add a random delay before collecting.
-// The delay should be randomly selected between 0 and this value. Currently the
-// value is equal to 10 seconds.
-const int kMaxRestoreSessionCollectionDelayMs = 10 * 1000;
+// much time between collections.
+const base::TimeDelta kMinIntervalBetweenSessionRestoreCollections =

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

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.

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

You're right. This is left over from when I thought I could turn TimeDelta into
POD.

Done.

+    base::TimeDelta::FromSeconds(30);
 
 // Enumeration representing success and various failure modes for collecting and
 // sending perf data.
 enum GetPerfDataOutcome {
   SUCCESS,
   NOT_READY_TO_UPLOAD,
   NOT_READY_TO_COLLECT,
   INCOGNITO_ACTIVE,
   INCOGNITO_LAUNCHED,
   PROTOBUF_NOT_PARSED,
@@ skipping 10 matching lines @@
                             outcome,
                             NUM_OUTCOMES);
 }
 
 // Returns true if a normal user is logged in. Returns false otherwise (e.g. if
 // logged in as a guest or as a kiosk app).
 bool IsNormalUserLoggedIn() {
   return chromeos::LoginState::Get()->IsUserAuthenticated();
 }
 
+// Returns a random TimeDelta between zero and |max|.
+base::TimeDelta RandomTimeDelta(base::TimeDelta max) {
+  return base::TimeDelta::FromMicroseconds(
+      base::RandGenerator(max.InMicroseconds()));
+}
+
 }  // namespace
 
-namespace metrics {
+PerfProvider::CollectionParams::CollectionParams(
+    base::TimeDelta collection_duration,
+    base::TimeDelta periodic_interval,
+    TriggerParams resume_from_suspend,
+    TriggerParams restore_session)
+    : collection_duration_(collection_duration.ToInternalValue()),
+      periodic_interval_(periodic_interval.ToInternalValue()),
+      resume_from_suspend_(resume_from_suspend),
+      restore_session_(restore_session) {
+}
+
+PerfProvider::CollectionParams::TriggerParams::TriggerParams(
+    int64 sampling_factor,
+    base::TimeDelta max_collection_delay)
+    : sampling_factor_(sampling_factor),
+      max_collection_delay_(max_collection_delay.ToInternalValue()) {
+}
+
+const PerfProvider::CollectionParams kDefaultParameters(

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

This should be in the anon namespace above.

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

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.

+  /* collection_duration = */ base::TimeDelta::FromSeconds(2),
+  /* periodic_interval = */ base::TimeDelta::FromHours(3),
+  /* resume_from_suspend = */ PerfProvider::CollectionParams::TriggerParams(
+      /* sampling_factor = */ 10,
+      /* max_collection_delay = */ base::TimeDelta::FromSeconds(5)),
+  /* restore_session = */ PerfProvider::CollectionParams::TriggerParams(
+      /* sampling_factor = */ 10,
+      /* max_collection_delay = */ base::TimeDelta::FromSeconds(10)));
 
 PerfProvider::PerfProvider()
-      : login_observer_(this),
-        next_profiling_interval_start_(base::TimeTicks::Now()),
-        weak_factory_(this) {
+    : collection_params_(kDefaultParameters),
+      login_observer_(this),
+      next_profiling_interval_start_(base::TimeTicks::Now()),
+      weak_factory_(this) {
   // Register the login observer with LoginState.
   chromeos::LoginState::Get()->AddObserver(&login_observer_);
 
   // Register as an observer of power manager events.
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->
       AddObserver(this);
 
   // Register as an observer of session restore.
   on_session_restored_callback_subscription_ =
       SessionRestore::RegisterOnSessionRestoredCallback(
@@ skipping 93 matching lines @@
   // canceled. Do not collect anything if that's the case.
   if (sleep_duration == base::TimeDelta())
     return;
 
   // Do not collect a profile unless logged in. The system behavior when closing
   // the lid or idling when not logged in is currently to shut down instead of
   // suspending. But it's good to enforce the rule here in case that changes.
   if (!IsNormalUserLoggedIn())
     return;
 
-  // Collect a profile only 1/|kResumeSamplingFactor| of the time, to avoid
+  // Collect a profile only 1/|sampling_factor| of the time, to avoid
   // collecting too much data.
-  if (base::RandGenerator(kResumeSamplingFactor) != 0)
+  const auto& resume_params = collection_params_.resume_from_suspend();
+  if (base::RandGenerator(resume_params.sampling_factor()) != 0)
     return;
 
   // Override any existing profiling.
   if (timer_.IsRunning())
     timer_.Stop();
 
   // Randomly pick a delay before doing the collection.
-  base::TimeDelta collection_delay =
-      base::TimeDelta::FromMilliseconds(
-          base::RandGenerator(kMaxResumeCollectionDelayMs));
+  base::TimeDelta collection_delay = RandomTimeDelta(
+      resume_params.max_collection_delay());
   timer_.Start(FROM_HERE,
                collection_delay,
                base::Bind(&PerfProvider::CollectPerfDataAfterResume,
                           weak_factory_.GetWeakPtr(),
                           sleep_duration,
                           collection_delay));
 }
 
 void PerfProvider::OnSessionRestoreDone(int num_tabs_restored) {
   // Do not collect a profile unless logged in as a normal user.
   if (!IsNormalUserLoggedIn())
     return;
 
-  // Collect a profile only 1/|kRestoreSessionSamplingFactor| of the time, to
+  // Collect a profile only 1/|sampling_factor| of the time, to
   // avoid collecting too much data and potentially causing UI latency.
-  if (base::RandGenerator(kRestoreSessionSamplingFactor) != 0)
+  const auto& restore_params = collection_params_.restore_session();
+  if (base::RandGenerator(restore_params.sampling_factor()) != 0)
     return;
 
-  const base::TimeDelta min_interval =
-      base::TimeDelta::FromMilliseconds(
-          kMinIntervalBetweenSessionRestoreCollectionsMs);
+  const auto min_interval = kMinIntervalBetweenSessionRestoreCollections;
   const base::TimeDelta time_since_last_collection =
       (base::TimeTicks::Now() - last_session_restore_collection_time_);
   // Do not collect if there hasn't been enough elapsed time since the last
   // collection.
   if (!last_session_restore_collection_time_.is_null() &&
       time_since_last_collection < min_interval) {
     return;
   }
 
   // Stop any existing scheduled collection.
   if (timer_.IsRunning())
     timer_.Stop();
 
   // Randomly pick a delay before doing the collection.
-  base::TimeDelta collection_delay =
-      base::TimeDelta::FromMilliseconds(
-          base::RandGenerator(kMaxRestoreSessionCollectionDelayMs));
+  base::TimeDelta collection_delay = RandomTimeDelta(
+      restore_params.max_collection_delay());
   timer_.Start(
       FROM_HERE,
       collection_delay,
       base::Bind(&PerfProvider::CollectPerfDataAfterSessionRestore,
                  weak_factory_.GetWeakPtr(),
                  collection_delay,
                  num_tabs_restored));
 }
 
 void PerfProvider::OnUserLoggedIn() {
   login_time_ = base::TimeTicks::Now();
   ScheduleIntervalCollection();
 }
 
 void PerfProvider::Deactivate() {
   // Stop the timer, but leave |cached_perf_data_| intact.
   timer_.Stop();
 }
 
 void PerfProvider::ScheduleIntervalCollection() {
   DCHECK(CalledOnValidThread());
   if (timer_.IsRunning())
     return;
 
   // Pick a random time in the current interval.
   base::TimeTicks scheduled_time =
       next_profiling_interval_start_ +
-      base::TimeDelta::FromMilliseconds(
-          base::RandGenerator(kPerfProfilingIntervalMs));
+      RandomTimeDelta(collection_params_.periodic_interval());
 
   // If the scheduled time has already passed in the time it took to make the
   // above calculations, trigger the collection event immediately.
   base::TimeTicks now = base::TimeTicks::Now();
   if (scheduled_time < now)
     scheduled_time = now;
 
   timer_.Start(FROM_HERE, scheduled_time - now, this,
                &PerfProvider::DoPeriodicCollection);
 
   // Update the profiling interval tracker to the start of the next interval.
-  next_profiling_interval_start_ +=
-      base::TimeDelta::FromMilliseconds(kPerfProfilingIntervalMs);
+  next_profiling_interval_start_ += collection_params_.periodic_interval();
 }
 
 void PerfProvider::CollectIfNecessary(
     scoped_ptr<SampledProfile> sampled_profile) {
   DCHECK(CalledOnValidThread());
 
   // Schedule another interval collection. This call makes sense regardless of
   // whether or not the current collection was interval-triggered. If it had
   // been another type of trigger event, the interval timer would have been
   // halted, so it makes sense to reschedule a new interval collection.
@@ skipping 16 matching lines @@
     AddToPerfHistogram(INCOGNITO_ACTIVE);
     return;
   }
 
   scoped_ptr<WindowedIncognitoObserver> incognito_observer(
       new WindowedIncognitoObserver);
 
   chromeos::DebugDaemonClient* client =
       chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
 
-  base::TimeDelta collection_duration = base::TimeDelta::FromSeconds(
-      kPerfCommandDurationDefaultSeconds);
-
   client->GetPerfOutput(
-      collection_duration.InSeconds(),
+      collection_params_.collection_duration().InSeconds(),
       base::Bind(&PerfProvider::ParseOutputProtoIfValid,
                  weak_factory_.GetWeakPtr(), base::Passed(&incognito_observer),
                  base::Passed(&sampled_profile)));
 }
 
 void PerfProvider::DoPeriodicCollection() {
   scoped_ptr<SampledProfile> sampled_profile(new SampledProfile);
   sampled_profile->set_trigger_event(SampledProfile::PERIODIC_COLLECTION);
 
   CollectIfNecessary(sampled_profile.Pass());
@@ skipping 18 matching lines @@
   scoped_ptr<SampledProfile> sampled_profile(new SampledProfile);
   sampled_profile->set_trigger_event(SampledProfile::RESTORE_SESSION);
   sampled_profile->set_ms_after_restore(time_after_restore.InMilliseconds());
   sampled_profile->set_num_tabs_restored(num_tabs_restored);
 
   CollectIfNecessary(sampled_profile.Pass());
   last_session_restore_collection_time_ = base::TimeTicks::Now();
 }
 
 }  // namespace metrics