Add Startup Timing to CPM

chrome/browser/performance_monitor/startup_timer.h

Index: chrome/browser/performance_monitor/startup_timer.h
diff --git a/chrome/browser/performance_monitor/startup_timer.h b/chrome/browser/performance_monitor/startup_timer.h
new file mode 100644
index 0000000000000000000000000000000000000000..ce74d62691f9b84eb6226cea6cb0ae35d1babb50
--- /dev/null
+++ b/chrome/browser/performance_monitor/startup_timer.h
@@ -0,0 +1,95 @@
+// 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_PERFORMANCE_MONITOR_STARTUP_TIMER_H_
+#define CHROME_BROWSER_PERFORMANCE_MONITOR_STARTUP_TIMER_H_
+
+#include "base/time.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+namespace performance_monitor {
+
+// This class is responsible for recording the startup and session restore times
+// (if applicable) for PerformanceMonitor. This allows us to initialize this
+// relatively small object early in the startup process, and start the whole of
+// PerformanceMonitor at a later time. StartupTimer will record the times and
+// insert them into PerformanceMonitor's database.
+class StartupTimer : public content::NotificationObserver {
+ public:
+  StartupTimer();
+  virtual ~StartupTimer();
+
+  // Inform StartupTimer that the startup process has been completed; stop the
+  // timer. Returns false if the timer has already stopped.
+  bool SignalStartupComplete(bool normal_startup);

[Yoyo Zhou, 2012/07/31 09:57:32]

This would be more understandable as an enum, since the choice is not "normal"
or "abnormal" but rather "normal" or "test".

[Devlin, 2012/07/31 16:43:40]

Done.

+
+  // Pauses the timer until Unpause is called; any time spent within a pause
+  // does not count towards the total.

[Yoyo Zhou, 2012/07/31 09:57:32]

Please document the return values.

[Devlin, 2012/07/31 16:43:40]

Done.

+  static bool PauseTimer();
+  static bool UnpauseTimer();
+
+  // content::NotificationObserver
+  // We keep track of whether or not PerformanceMonitor has been started via
+  // the PERFORMANCE_MONITOR_INITIALIZED notification; we need to know this so
+  // we know when to insert startup data into the database. We either insert
+  // data as we gather it (if PerformanceMonitor is started prior to data
+  // collection) or at the notification (if PerformanceMonitor is started
+  // later).
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  static void SetSessionRestoreTime(
+      const base::TimeDelta& session_restore_time);
+
+ private:
+  bool PauseTimerImpl();
+  bool UnpauseTimerImpl();
+  void InsertStartupTime();

[Yoyo Zhou, 2012/07/31 09:57:32]

These names are vaguely confusing because "time" can mean both a point and a
duration. InsertElapsedStartupTime would be better because that disambiguates
time. I'd suggest a similar change for session restore time.

[Devlin, 2012/07/31 16:43:40]

Done.

+  void InsertSessionRestoreTime();
+
+  // The time at which the startup process begins (the creation of
+  // ChromeBrowserMain).
+  base::TimeTicks startup_begin_;
+
+  // The time at which the timer was most recently paused, or null if the timer
+  // is not currently paused.
+  base::TimeTicks pause_started_;
+
+  // The total duration for which the timer has been paused.
+  base::TimeDelta total_pause_;
+
+  // A flag of whether or not this was a "normal" startup (e.g. whether or not
+  // this was in a testing environment, which would change the startup time
+  // values). If it is not a normal startup, we use a different metric.
+  bool normal_startup_;
+
+  // The total duration of the startup process, minus any pauses.
+  base::TimeDelta elapsed_startup_time_;
+
+  // The total duration of the session restore, if one occurred. This is
+  // independent of the startup time, because:
+  // - If the user has auto-restore on, the restore is synchronous, and we pause
+  //   the startup timer during the session restore; the restore will not
+  //   interfere with startup timing.
+  // - If Chrome crashed and the user chooses to restore the crashed session,
+  //   then the startup is already completed; the restore will not interfere
+  //   with startup timing.
+  base::TimeDelta session_restore_time_;
+
+  // Flag whether or not PerformanceMonitor has been fully started.
+  bool performance_monitor_initialized_;
+
+  content::NotificationRegistrar registrar_;
+
+  // The singleton of this class.

[Yoyo Zhou, 2012/07/31 09:57:32]

Use base/memory/singleton.h for this purpose.

[Devlin, 2012/07/31 16:43:40]

And...sorry in advance for a long comment.

Pursuant to the header in base/memory/singleton.h, I opted for not using the
Singleton class (would link to code, but codesearch is refusing to cooperate).
It asks to please find another class that can govern the object's lifespan.
Since ChromeBrowserMainParts works for this purpose, I went with that approach,
rather than "adding another singleton to the mix" and leading to "buggy code and
spurious crashes" (perhaps a little exaggerated, but they have a point).

Also, take a look at StartupTimeBomb's implementation
(http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/metrics/thr...).
This actually serves a somewhat similar purpose (so much so I was contemplating
combining the two classes, but they aren't quite similar enough). It is
constructed at the start of ChromeBrowserMainParts, keeps an eye on everything
to make sure it goes okay, is signaled at completion, and is then destructed at
destruction of ChromeBrowserMainParts.

Finally, I thought the reason we usually used base::Singleton was to avoid leaky
singletons (e.g. c/b/e/extension_error_reporter.*). If you look at how this one
(and StartupTimeBomb) are created, these don't actually leak - they just enforce
that there's only one in existence. They are still destructed like any other
data member of ChromeBrowserMainParts.

I'm still happy to make this a base::Singleton if it strikes you as better (I
don't have a strong opinion one way or the other - I would have made it a
base::Singleton, if not for the comment in the header), but I wanted to make
sure I explained my initial reasoning.

Okay, that's it :) Whew...

+  static StartupTimer* g_startup_timer_;
+
+  DISALLOW_COPY_AND_ASSIGN(StartupTimer);
+};
+
+}  // namespace performance_monitor
+
+#endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_STARTUP_TIMER_H_