Fix logic on high Windows resolution timer and have

base/time/time_win.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.
 
 
 // Windows Timer Primer
 //
 // A good article:  http://www.ddj.com/windows/184416651
 // A good mozilla bug:  http://bugzilla.mozilla.org/show_bug.cgi?id=363258
 //
@@ skipping 11 matching lines @@
 // 1ms, but only if you call APIs (timeBeginPeriod()) which affect all other
 // applications on the system.  By default, precision is only 15.5ms.
 // Unfortunately, we don't want to call timeBeginPeriod because we don't
 // want to affect other applications.  Further, on mobile platforms, use of
 // faster multimedia timers can hurt battery life.  See the intel
 // article about this here:
 // http://softwarecommunity.intel.com/articles/eng/1086.htm
 //
 // To work around all this, we're going to generally use timeGetTime().  We
 // will only increase the system-wide timer if we're not running on battery
-// power.  Using timeBeginPeriod(1) is a requirement in order to make our
-// message loop waits have the same resolution that our time measurements
-// do.  Otherwise, WaitForSingleObject(..., 1) will no less than 15ms when
-// there is nothing else to waken the Wait.
+// power.
 
 #include "base/time/time.h"
 
 #pragma comment(lib, "winmm.lib")
 #include <windows.h>
 #include <mmsystem.h>
 
 #include "base/basictypes.h"
 #include "base/cpu.h"
 #include "base/lazy_instance.h"
@@ skipping 34 matching lines @@
 const int kMaxMillisecondsToAvoidDrift = 60 * Time::kMillisecondsPerSecond;
 
 int64 initial_time = 0;
 TimeTicks initial_ticks;
 
 void InitializeClock() {
   initial_ticks = TimeTicks::Now();
   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.
+const int kMinTimerIntervalHighResMs = 1;
+const int kMinTimerIntervalLowResMs = 4;

[jamesr, 2014/08/26 05:03:17]

since these 2 vars always go into a UINT can we just make the type uint32_t?

+// Track if kMinTimerIntervalHighResMs or kMinTimerIntervalLowResMs is active.
+bool g_high_res_timer_enabled = false;
+// How many times the high resolution timer has been called.
+int g_high_res_timer_count = 0;
+// The lock to control access to the above two variables.
+base::LazyInstance<base::Lock> g_high_res_lock = LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 // Time -----------------------------------------------------------------------
 
 // The internal representation of Time uses FILETIME, whose epoch is 1601-01-01
 // 00:00:00 UTC.  ((1970-1601)*365+89)*24*60*60*1000*1000, where 89 is the
 // number of leap year days between 1601 and 1970: (1970-1601)/4 excluding
 // 1700, 1800, and 1900.
 // static
 const int64 Time::kTimeTToMicrosecondsOffset = GG_INT64_C(11644473600000000);
 
-bool Time::high_resolution_timer_enabled_ = false;
-int Time::high_resolution_timer_activated_ = 0;
-
 // static
 Time Time::Now() {
   if (initial_time == 0)
     InitializeClock();
 
   // We implement time using the high-resolution timers so that we can get
   // timeouts which are smaller than 10-15ms.  If we just used
   // CurrentWallclockMicroseconds(), we'd have the less-granular timer.
   //
   // To make this work, we initialize the clock (initial_time) and the
@@ skipping 44 matching lines @@
     result.dwLowDateTime = std::numeric_limits<DWORD>::max();
     return result;
   }
   FILETIME utc_ft;
   MicrosecondsToFileTime(us_, &utc_ft);
   return utc_ft;
 }
 
 // static
 void Time::EnableHighResolutionTimer(bool enable) {
-  // Test for single-threaded access.
-  static PlatformThreadId my_thread = PlatformThread::CurrentId();
-  DCHECK(PlatformThread::CurrentId() == my_thread);
-
-  if (high_resolution_timer_enabled_ == enable)
-    return;
-
-  high_resolution_timer_enabled_ = enable;
+  {
+    base::AutoLock lock(g_high_res_lock.Get());
+    if (g_high_res_timer_enabled == enable)
+      return;
+    g_high_res_timer_enabled = enable;
+    if (!g_high_res_timer_count)
+      return;
+  }
+  if (enable) {

[jamesr, 2014/08/26 05:03:17]

let's say we're in high resolution mode with no active high res tasks (so no
pending timeBeginPeriod calls) and have two threads.  Thread A sees we're on
battery now and is coming in to call EnableHRTimer(false).  Thread B has a task
that wants high res timers and is coming in to call ActivateHRTimer(true).

1.) A and B race on the g_high_res_lock and B wins
2.) B executes lines 199-203 under the lock and releases is

now g_high_res_lock is released, g_high_res_timer_enabled is still true,
g_high_res_timer_count is 1.  In thread B, the locals period and high_res_count
are 1 and 1, respectively and activating is true

3.) Thread A aquires the lock and executes lines 175-180

now g_high_res_lock is released, g_high_res_timer_enabled is false,
g_high_res_timer_count is 1.   In thread A, local enable is set to false

4.) Thread A executes lines 182-186  enable is false so we call timeEndPeriod(1)

5.) Thread B executes lines 205-207. activating is true, high_res_count is 1 and
period is 1 so we call to ::timeBeginPeriod(1)

6.) Thread A executes line 187 and calls timeBeginPeriod(4)

now we're a bit buggered.  we've mismatched calls to
timeBeginPeriod/timeEndPeriod by calling timeEndPeriod(1) without a
corresponding timeBeginPeriod() and we've called timeBeginPeriod() twice in a
row.  If things go as planned we'll call timeEndPeriod(4) later on when we
de-activate, but we'll leak a timeBeginPeriod(1)

I think the lock has to surround the time{Begin,End}Period() calls since we're
communicating data into the win32 api and need to make sure that order is
consistent  It's sadface but trueface.

+    timeEndPeriod(kMinTimerIntervalLowResMs);
+    timeBeginPeriod(kMinTimerIntervalHighResMs);
+  } else {
+    timeEndPeriod(kMinTimerIntervalHighResMs);
+    timeBeginPeriod(kMinTimerIntervalLowResMs);
+  }
 }
 
 // static
 bool Time::ActivateHighResolutionTimer(bool activating) {
-  if (!high_resolution_timer_enabled_ && activating)
-    return false;
-
-  // Using anything other than 1ms makes timers granular
-  // to that interval.
-  const int kMinTimerIntervalMs = 1;
-  MMRESULT result;
+  // 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
+  // called.
+  int high_res_count;
+  UINT period;
+  {
+    base::AutoLock lock(g_high_res_lock.Get());
+    period = g_high_res_timer_enabled ? kMinTimerIntervalHighResMs
+                                      : kMinTimerIntervalLowResMs;
+    high_res_count =
+        activating ? ++g_high_res_timer_count : --g_high_res_timer_count;
+  }
   if (activating) {
-    result = timeBeginPeriod(kMinTimerIntervalMs);
-    high_resolution_timer_activated_++;
+    if (high_res_count == 1)
+      timeBeginPeriod(period);
   } else {
-    result = timeEndPeriod(kMinTimerIntervalMs);
-    high_resolution_timer_activated_--;
+    if (high_res_count == 0)
+      timeEndPeriod(period);
   }
-  return result == TIMERR_NOERROR;
+  return (period == kMinTimerIntervalHighResMs);
 }
 
 // static
 bool Time::IsHighResolutionTimerInUse() {
-  // Note:  we should track the high_resolution_timer_activated_ value
-  // under a lock if we want it to be accurate in a system with multiple
-  // message loops.  We don't do that - because we don't want to take the
-  // expense of a lock for this.  We *only* track this value so that unit
-  // tests can see if the high resolution timer is on or off.
-  return high_resolution_timer_enabled_ &&
-      high_resolution_timer_activated_ > 0;
+  base::AutoLock lock(g_high_res_lock.Get());
+  return g_high_res_timer_enabled && g_high_res_timer_count > 0;
 }
 
 // static
 Time Time::FromExploded(bool is_local, const Exploded& exploded) {
   // Create the system struct representing our exploded time. It will either be
   // in local time or UTC.
   SYSTEMTIME st;
   st.wYear = exploded.year;
   st.wMonth = exploded.month;
   st.wDayOfWeek = exploded.day_of_week;
@@ skipping 315 matching lines @@
     return TimeTicks() + TimeDelta::FromMilliseconds(timeGetTime());
   }
 }
 
 // TimeDelta ------------------------------------------------------------------
 
 // static
 TimeDelta TimeDelta::FromQPCValue(LONGLONG qpc_value) {
   return TimeDelta(GetHighResNowSingleton()->QPCValueToMicroseconds(qpc_value));
 }