Fix timer interval race condition

The feature flag that I added to control whether the Windows timer
interval on DC power should be 4 ms or 8 ms had a race condition in that
we might query the feature flag before the feature-flag system was
configured. This change fixes that by adding the
ReadMinTimerIntervalLowResMs function which is called after the feature
system is initialized.

While working on this I realized that ActivateHighResolutionTimer and
EnableHighResolutionTimer were extremely subtle so I simplified them.
When calling timeBeginPeriod you *must* eventually call timeEndPeriod to
undo your request, and you must call it with exactly the same value.
With the raised timer frequency now having three possible values (1, 4,
or 8 ms) the logic was getting far too messy so I now do the simpler
task of just recording the value passed to timeBeginPeriod and using it
in timeEndPeriod. This is more obviously correct.

The new rule is that timeBeginPeriod and timeEndPeriod should always be
called with g_last_interval_requested as the parameter.

I also tidied up and expanded on some comments - I always get confused
by the purpose and semantics of ActivateHighResolutionTimer and
EnableHighResolutionTimer.

I tested with --disable-highres-timer, with and without
--enable-features=SlowDCTimerInterruptsWin, using
trace_timer_intervals.bat to confirm that all Chrome processes were
lower the timer interrupt setting to 4 ms or 8 ms as requested.

Bug: 1033108, 927165
Change-Id: I69b9253b456d7e3273ea1823a61c20926b0a4510
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1965846Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726561}

base/time/time.h

@@ -649,6 +649,11 @@ class BASE_EXPORT Time : public time_internal::TimeBase<Time> {
   // Enable or disable Windows high resolution timer.
   static void EnableHighResolutionTimer(bool enable);
 
+  // Read the minimum timer interval from the feature list. This should be
+  // called once after the feature list is initialized. This is needed for
+  // an experiment - see https://crbug.com/927165
+  static void ReadMinTimerIntervalLowResMs();
+
   // Activates or deactivates the high resolution timer based on the |activate|
   // flag.  If the HighResolutionTimer is not Enabled (see
   // EnableHighResolutionTimer), this function will return false.  Otherwise

base/time/time_win.cc

@@ -87,25 +87,14 @@ void InitializeClock() {
   g_initial_time = CurrentWallclockMicroseconds();
 }
 
-// The two values that ActivateHighResolutionTimer uses to set the systemwide
-// timer interrupt frequency on Windows. It controls how precise timers are
-// but also has a big impact on battery life.
-// Used when running on AC power - plugged in - when a fast timer is wanted.
-UINT MinTimerIntervalHighResMs() {
-  return 1;
-}
-
-UINT MinTimerIntervalLowResMs() {
-  // Traditionally Chrome has used an interval of 4 ms when raising the timer
-  // interrupt frequency on battery power. However even 4 ms is too short an
-  // interval on modern CPUs - it wastes non-trivial power - so this experiment
-  // tests an interval of 8 ms, recommended by Intel.
-  static const UINT s_interval =
-      base::FeatureList::IsEnabled(base::kSlowDCTimerInterruptsWin) ? 8 : 4;
-  return s_interval;
-}
-
+// Interval to use when on DC power.
+UINT g_battery_power_interval_ms = 4;
+// Track the last value passed to timeBeginPeriod so that we can cancel that
+// call by calling timeEndPeriod with the same value. A value of zero means that
+// the timer frequency is not currently raised.
+UINT g_last_interval_requested_ms = 0;
 // Track if MinTimerIntervalHighResMs() or MinTimerIntervalLowResMs() is active.
+// For most purposes this could also be named g_is_on_ac_power.
 bool g_high_res_timer_enabled = false;
 // How many times the high resolution timer has been called.
 uint32_t g_high_res_timer_count = 0;
@@ -118,12 +107,62 @@ TimeDelta g_high_res_timer_usage;
 // Timestamp of the last activation change of the high resolution timer. This
 // is used to calculate the cumulative usage.
 TimeTicks g_high_res_timer_last_activation;
-// The lock to control access to the above two variables.
+// The lock to control access to the above set of variables.
 Lock* GetHighResLock() {
   static auto* lock = new Lock();
   return lock;
 }
 
+// The two values that ActivateHighResolutionTimer uses to set the systemwide
+// timer interrupt frequency on Windows. These control how precise timers are
+// but also have a big impact on battery life.
+
+// Used when a faster timer has been requested (g_high_res_timer_count > 0) and
+// the computer is running on AC power (plugged in) so that it's okay to go to
+// the highest frequency.
+UINT MinTimerIntervalHighResMs() {
+  return 1;
+}
+
+// Used when a faster timer has been requested (g_high_res_timer_count > 0) and
+// the computer is running on DC power (battery) so that we don't want to raise
+// the timer frequency as much.
+UINT MinTimerIntervalLowResMs() {
+  return g_battery_power_interval_ms;
+}
+
+// Calculate the desired timer interrupt interval. Note that zero means that the
+// system default should be used.
+UINT GetIntervalMs() {
+  if (!g_high_res_timer_count)
+    return 0;  // Use the default, typically 15.625
+  if (g_high_res_timer_enabled)
+    return MinTimerIntervalHighResMs();
+  return MinTimerIntervalLowResMs();
+}
+
+// Compare the currently requested timer interrupt interval to the last interval
+// requested and update if necessary (by cancelling the old request and making a
+// new request). If there is no change then do nothing.
+void UpdateTimerIntervalLocked() {
+  UINT new_interval = GetIntervalMs();
+  if (new_interval == g_last_interval_requested_ms)
+    return;
+  if (g_last_interval_requested_ms) {
+    // Record how long the timer interrupt frequency was raised.
+    g_high_res_timer_usage += subtle::TimeTicksNowIgnoringOverride() -
+                              g_high_res_timer_last_activation;
+    // Reset the timer interrupt back to the default.
+    timeEndPeriod(g_last_interval_requested_ms);
+  }
+  g_last_interval_requested_ms = new_interval;
+  if (g_last_interval_requested_ms) {
+    // Record when the timer interrupt was raised.
+    g_high_res_timer_last_activation = subtle::TimeTicksNowIgnoringOverride();
+    timeBeginPeriod(g_last_interval_requested_ms);
+  }
+}
+
 // Returns the current value of the performance counter.
 uint64_t QPCNowRaw() {
   LARGE_INTEGER perf_counter_now = {};
@@ -206,29 +245,34 @@ FILETIME Time::ToFileTime() const {
   return utc_ft;
 }
 
+void Time::ReadMinTimerIntervalLowResMs() {
+  AutoLock lock(*GetHighResLock());
+  // Read the setting for what interval to use on battery power.
+  g_battery_power_interval_ms =
+      base::FeatureList::IsEnabled(base::kSlowDCTimerInterruptsWin) ? 8 : 4;
+  UpdateTimerIntervalLocked();
+}
+
 // static
+// Enable raising of the system-global timer interrupt frequency to 1 kHz (when
+// enable is true, which happens when on AC power) or some lower frequency when
+// on battery power (when enable is false). If the g_high_res_timer_enabled
+// setting hasn't actually changed or if if there are no outstanding requests
+// (if g_high_res_timer_count is zero) then do nothing.
+// TL;DR - call this when going from AC to DC power or vice-versa.
 void Time::EnableHighResolutionTimer(bool enable) {
   AutoLock lock(*GetHighResLock());
-  if (g_high_res_timer_enabled == enable)
-    return;
   g_high_res_timer_enabled = enable;
-  if (!g_high_res_timer_count)
-    return;
-  // Since g_high_res_timer_count != 0, an ActivateHighResolutionTimer(true)
-  // was called which called timeBeginPeriod with g_high_res_timer_enabled
-  // with a value which is the opposite of |enable|. With that information we
-  // call timeEndPeriod with the same value used in timeBeginPeriod and
-  // therefore undo the period effect.
-  if (enable) {
-    timeEndPeriod(MinTimerIntervalLowResMs());
-    timeBeginPeriod(MinTimerIntervalHighResMs());
-  } else {
-    timeEndPeriod(MinTimerIntervalHighResMs());
-    timeBeginPeriod(MinTimerIntervalLowResMs());
-  }
+  UpdateTimerIntervalLocked();
 }
 
 // static
+// Request that the system-global Windows timer interrupt frequency be raised.
+// How high the frequency is raised depends on the system's power state and
+// possibly other options.
+// TL;DR - call this at the beginning and end of a time period where you want
+// higher frequency timer interrupts. Each call with activating=true must be
+// paired with a subsequent activating=false call.
 bool Time::ActivateHighResolutionTimer(bool activating) {
   // We only do work on the transition from zero to one or one to zero so we
   // can easily undo the effect (if necessary) when EnableHighResolutionTimer is
@@ -236,31 +280,22 @@ bool Time::ActivateHighResolutionTimer(bool activating) {
   const uint32_t max = std::numeric_limits<uint32_t>::max();
 
   AutoLock lock(*GetHighResLock());
-  UINT period = g_high_res_timer_enabled ? MinTimerIntervalHighResMs()
-                                         : MinTimerIntervalLowResMs();
   if (activating) {
     DCHECK_NE(g_high_res_timer_count, max);
     ++g_high_res_timer_count;
-    if (g_high_res_timer_count == 1) {
-      g_high_res_timer_last_activation = subtle::TimeTicksNowIgnoringOverride();
-      timeBeginPeriod(period);
-    }
   } else {
     DCHECK_NE(g_high_res_timer_count, 0u);
     --g_high_res_timer_count;
-    if (g_high_res_timer_count == 0) {
-      g_high_res_timer_usage += subtle::TimeTicksNowIgnoringOverride() -
-                                g_high_res_timer_last_activation;
-      timeEndPeriod(period);
-    }
   }
-  return period == MinTimerIntervalHighResMs();
+  UpdateTimerIntervalLocked();
+  return true;
 }
 
 // static
+// See if the timer interrupt interval has been set to the lowest value.
 bool Time::IsHighResolutionTimerInUse() {
   AutoLock lock(*GetHighResLock());
-  return g_high_res_timer_enabled && g_high_res_timer_count > 0;
+  return g_last_interval_requested_ms == MinTimerIntervalHighResMs();
 }
 
 // static

chrome/app/chrome_main_delegate.cc

@@ -614,6 +614,10 @@ void ChromeMainDelegate::PostFieldTrialInitialization() {
     chromeos::LockMainProgramText();
 #endif
   }
+
+#if defined(OS_WIN)
+  base::Time::ReadMinTimerIntervalLowResMs();
+#endif
 }
 
 bool ChromeMainDelegate::BasicStartupComplete(int* exit_code) {