Initialize the now_funciton to the HighResNowWrapper in case High Res is supported

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 65 matching lines @@
   return FileTimeToMicroseconds(ft);
 }
 
 // Time between resampling the un-granular clock for this API.  60 seconds.
 const int kMaxMillisecondsToAvoidDrift = 60 * Time::kMillisecondsPerSecond;
 
 int64 initial_time = 0;
 TimeTicks initial_ticks;
 
 void InitializeClock() {
   initial_ticks = TimeTicks::Now();

[jar (doing other things), 2014/08/08 00:30:53]

None of this seems to be lock protected.  How do you ensure that the entire 64
bit quantity is written atomically by any one thread?

IF this method was called by several threads around the same time, it is not
clear what the result would be, especially if you "lucky" enough to call around
the point at which the lower 32bit quantity was near its max, and about to roll
another bit into the high-order 32 bits.

Equally questionable would be whether the resulting initial_time (see next line)
would really be written at a "corresponding time" to the setting of initial
ticks.

Could you explain this... or point me at a design doc?

[fmeawad, 2014/08/15 00:38:02]

I could not locate a design doc, this particular code is from the early versions
of chrome.
I added a lock to protect reads and writes of the pair (initial_ticks and
initial_time).

   initial_time = CurrentWallclockMicroseconds();
 }
 
 }  // 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)

[jar (doing other things), 2014/08/08 00:30:53]

This sure seems scary, given that Time::Now() can be called asynchronously on
all threads.  

[fmeawad, 2014/08/15 00:38:02]

With the lock added to InitializeClock and the scoped lock in the while loop,
even if multiple threads call Time::Now, worst case we will get multiple
synchronized initialization.

     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
   // counter (initial_ctr).  To compute the initial time, we can check
   // the number of ticks that have elapsed, and compute the delta.
   //
   // To avoid any drift, we periodically resync the counters to the system
   // clock.
   while (true) {
     TimeTicks ticks = TimeTicks::Now();
 
     // Calculate the time elapsed since we started our timer
     TimeDelta elapsed = ticks - initial_ticks;
 
     // Check if enough time has elapsed that we need to resync the clock.
     if (elapsed.InMilliseconds() > kMaxMillisecondsToAvoidDrift) {
       InitializeClock();
       continue;
     }
 
     return Time(elapsed + Time(initial_time));

[jar (doing other things), 2014/08/08 00:30:53]

This seems to access initial_time in a racy fashion, as another thread might
have changed its value between line 123 (when elapsed was calculated), and now
(when it is hoped that intial_time corresponds(?) in some sense to initial_ticks
(used on line 123).

It sure looks like the results can be very random (use old elapsed value, in
concern with an updated initial_time value, due to some other thread).

What am I missing?

[fmeawad, 2014/08/15 00:38:02]

Now we read initial_ticks and initial_time in a protected way.

   }
 }
 
 // static
 Time Time::NowFromSystemTime() {
   // Force resync.
   InitializeClock();
   return Time(initial_time);
 }
 
@@ skipping 210 matching lines @@
 // reality due to bugs in BIOS or HAL on some, especially old computers.
 // With recent updates on HAL and newer BIOS, QPC is getting more reliable but
 // it should be used with caution.
 //
 // (3) System time. The system time provides a low-resolution (typically 10ms
 // to 55 milliseconds) time stamp but is comparatively less expensive to
 // retrieve and more reliable.
 class HighResNowSingleton {
  public:
   HighResNowSingleton()
     : ticks_per_second_(0),

[jar (doing other things), 2014/08/08 00:30:53]

nit: [Yeah... this wasn't your code... but I had to read this to understand the
patch]  Indent the colon 4 beyond the previous line (2 more than currently), and
continue to align member names (next line gets indented 2 more also).

[fmeawad, 2014/08/15 00:38:02]

Done.

       skew_(0) {
     InitializeClock();
 
     base::CPU cpu;
     if (IsBuggyAthlon(cpu))
       DisableHighResClock();
   }
 
   bool IsUsingHighResClock() {
     return ticks_per_second_ != 0.0;
@@ skipping 26 matching lines @@
     int64 whole_seconds = qpc_value / ticks_per_second_;
     int64 leftover_ticks = qpc_value % ticks_per_second_;
     int64 microseconds = (whole_seconds * Time::kMicrosecondsPerSecond) +
                          ((leftover_ticks * Time::kMicrosecondsPerSecond) /
                           ticks_per_second_);
     return microseconds;
   }
 
  private:
   // Synchronize the QPC clock with GetSystemTimeAsFileTime.
   void InitializeClock() {

[jar (doing other things), 2014/08/08 00:30:53]

Is this private method called exactly once, on line 364?  If so, it is a little
questionable as to whether it should be pulled out of line... but more of a
readability issue... It is bothersome that it has the same name as a function in
an anonymous namespace in this file.

Could you clean this up one way or the other (rename to use different names...
and/or don't factor it out of its one call site.)

[fmeawad, 2014/08/15 00:38:02]

I factored it inside its caller.

     LARGE_INTEGER ticks_per_sec = {0};
     if (!QueryPerformanceFrequency(&ticks_per_sec))
       return;  // Broken, we don't guarantee this function works.
     ticks_per_second_ = ticks_per_sec.QuadPart;
 
     skew_ = UnreliableNow() - ReliableNow();
   }
 
   // Get the number of microseconds since boot in an unreliable fashion.
   int64 UnreliableNow() {
@@ skipping 49 matching lines @@
   return old;
 }
 
 // static
 bool TimeTicks::SetNowIsHighResNowIfSupported() {
   if (!CPUReliablySupportsHighResTime()) {
     return false;
   }
 
   now_function = HighResNowWrapper;
+  InitializeClock();
   return true;
 }
 
 // static
 TimeTicks TimeTicks::Now() {
   return TimeTicks() + now_function();
 }
 
 // static
 TimeTicks TimeTicks::HighResNow() {
@@ skipping 38 matching lines @@
     return TimeTicks() + TimeDelta::FromMilliseconds(timeGetTime());
   }
 }
 
 // TimeDelta ------------------------------------------------------------------
 
 // static
 TimeDelta TimeDelta::FromQPCValue(LONGLONG qpc_value) {
   return TimeDelta(GetHighResNowSingleton()->QPCValueToMicroseconds(qpc_value));
 }