metrics: Use absolute interval-based perf collection

chrome/browser/metrics/perf_provider_chromeos.cc

Index: chrome/browser/metrics/perf_provider_chromeos.cc
diff --git a/chrome/browser/metrics/perf_provider_chromeos.cc b/chrome/browser/metrics/perf_provider_chromeos.cc
index efa0d00dea7efbfa886af51959877dc2879316c9..0f1cac8acf637402dbf1b1583599c029b6b8c282 100644
--- a/chrome/browser/metrics/perf_provider_chromeos.cc
+++ b/chrome/browser/metrics/perf_provider_chromeos.cc
@@ -24,26 +24,10 @@
 
 namespace {
 
-// Default time in seconds between invocations of perf.
-// This period is roughly 6.5 hours.
-// This is chosen to be relatively prime with the number of seconds in:
-// - one minute (60)
-// - one hour (3600)
-// - one day (86400)
-const size_t kPerfCommandIntervalDefaultSeconds = 23093;
-
-// The first collection interval is different from the interval above. This is
-// because we want to collect the first profile quickly after Chrome is started.
-// If this period is too long, the user will log off and Chrome will be killed
-// before it is triggered. The following 2 variables determine the upper and
-// lower bound on the interval.
-// The reason we do not always want to collect the initial profile after a fixed
-// period is to not over-represent task X in the profile where task X always
-// runs at a fixed period after start-up. By selecting a period randomly between
-// a lower and upper bound, we will hopefully collect a more fair profile.
-const size_t kPerfCommandStartIntervalLowerBoundMinutes = 10;
-
-const size_t kPerfCommandStartIntervalUpperBoundMinutes = 20;
+// Partition time since the epoch into successive intervals of this size. In
+// each interval that hasn't passed, pick a random time to collect a profile.
+// This interval is twenty-four hours.

[Ilya Sherman, 2014/05/29 22:56:49]

nit: Please update this comment.

[Simon Que, 2014/05/30 01:21:45]

Done.

+const size_t kPerfProfilingIntervalMs = 24 * 60 * 60 * 1000;
 
 // Default time in seconds perf is run for.
 const size_t kPerfCommandDurationDefaultSeconds = 2;
@@ -119,6 +103,7 @@ class WindowedIncognitoObserver : public chrome::BrowserListObserver {
 
 PerfProvider::PerfProvider()
       : 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_);
@@ -135,7 +120,7 @@ PerfProvider::~PerfProvider() {
   chromeos::LoginState::Get()->RemoveObserver(&login_observer_);
 }
 
-bool PerfProvider::GetPerfData(std::vector<PerfDataProto>* perf_data) {
+bool PerfProvider::GetPerfData(std::vector<SampledProfile>* perf_data) {
   DCHECK(CalledOnValidThread());
   if (cached_perf_data_.empty()) {
     AddToPerfHistogram(NOT_READY_TO_UPLOAD);
@@ -160,10 +145,8 @@ void PerfProvider::LoginObserver::LoggedInStateChanged() {
 }
 
 void PerfProvider::Activate() {

[Ilya Sherman, 2014/05/29 22:56:49]

nit: It might be wise to rename this method to something like
"OnUserDidLogIn()", now that it directly sets login_time_ internally.  As is, it
would be easy to accidentally start calling Activate() from some other trigger
as well, without realizing that it has this side-effect.

[Simon Que, 2014/05/30 01:21:45]

Done.

-  size_t collection_interval_minutes = base::RandInt(
-      kPerfCommandStartIntervalLowerBoundMinutes,
-      kPerfCommandStartIntervalUpperBoundMinutes);
-  ScheduleCollection(base::TimeDelta::FromMinutes(collection_interval_minutes));
+  login_time_ = base::TimeTicks::Now();
+  ScheduleCollection();
 }
 
 void PerfProvider::Deactivate() {
@@ -171,16 +154,29 @@ void PerfProvider::Deactivate() {
   timer_.Stop();
 }
 
-void PerfProvider::ScheduleCollection(const base::TimeDelta& interval) {
+void PerfProvider::ScheduleCollection() {
   DCHECK(CalledOnValidThread());
   if (timer_.IsRunning())
     return;
 
-  timer_.Start(FROM_HERE, interval, this,
-               &PerfProvider::CollectIfNecessaryAndReschedule);
+  // Pick a random time in the current interval.
+  base::TimeTicks scheduled_time =
+      next_profiling_interval_start_ +
+      base::TimeDelta::FromMilliseconds(
+          base::RandGenerator(kPerfProfilingIntervalMs));
+
+  // 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;
+  }

[Ilya Sherman, 2014/05/29 22:56:49]

nit: No need for curly braces.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  timer_.Start(FROM_HERE, scheduled_time - now, this,
+               &PerfProvider::DoPeriodicCollection);
 }
 
-void PerfProvider::CollectIfNecessary() {
+void PerfProvider::CollectIfNecessary(
+    SampledProfile::TriggerEvent trigger_event) {
   DCHECK(CalledOnValidThread());
 
   // Do not collect further data if we've already collected a substantial amount
@@ -213,17 +209,22 @@ void PerfProvider::CollectIfNecessary() {
   client->GetPerfData(collection_duration.InSeconds(),
                       base::Bind(&PerfProvider::ParseProtoIfValid,
                                  weak_factory_.GetWeakPtr(),
-                                 base::Passed(&incognito_observer)));
+                                 base::Passed(&incognito_observer),
+                                 trigger_event));
 }
 
-void PerfProvider::CollectIfNecessaryAndReschedule() {
-  CollectIfNecessary();
-  ScheduleCollection(
-      base::TimeDelta::FromSeconds(kPerfCommandIntervalDefaultSeconds));
+void PerfProvider::DoPeriodicCollection() {
+  CollectIfNecessary(SampledProfile::PERIODIC_COLLECTION);
+
+  // Update the profiling interval tracker to the start of the next interval.
+  next_profiling_interval_start_ +=
+      base::TimeDelta::FromMilliseconds(kPerfProfilingIntervalMs);

[Ilya Sherman, 2014/05/29 22:56:49]

nit: I think it would be slightly better to reset this within
ScheduleCollection().  That way, if somebody were to accidentally call
ScheduleCollection() multiple times, it wouldn't doubly-schedule collections
during a single interval; and if the code were to be refactored to call
ScheduleCollection() from somewhere else, it wouldn't be possible to forget to
update the next_profile_interval_start_.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  ScheduleCollection();
 }
 
 void PerfProvider::ParseProtoIfValid(
     scoped_ptr<WindowedIncognitoObserver> incognito_observer,
+    SampledProfile::TriggerEvent trigger_event,
     const std::vector<uint8>& data) {
   DCHECK(CalledOnValidThread());
 
@@ -238,10 +239,16 @@ void PerfProvider::ParseProtoIfValid(
     return;
   }
 
-  // Append a new PerfDataProto to the |cached_perf_data_| vector and swap in
-  // the contents.
+  // Populate a profile collection protobuf with the collected perf data and
+  // extra metadata.
   cached_perf_data_.resize(cached_perf_data_.size() + 1);
-  cached_perf_data_.back().Swap(&perf_data_proto);
+  SampledProfile& collection_data = cached_perf_data_.back();
+  collection_data.set_trigger_event(trigger_event);
+  collection_data.set_ms_after_boot(perf_data_proto.timestamp_sec());

[Ilya Sherman, 2014/05/29 22:56:49]

You are still assigning seconds to a millisecond field.

[Simon Que, 2014/05/30 01:21:45]

Done.

+  collection_data.
+      set_ms_after_login((base::TimeTicks::Now() - login_time_)

[Ilya Sherman, 2014/05/29 22:56:49]

nit: Please add a DCHECK that login_time_ is non-null.

[Simon Que, 2014/05/30 01:21:45]

Done.

+          .InMilliseconds());
+  *collection_data.mutable_perf_data() = perf_data_proto;
 }
 
 }  // namespace metrics