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

We have noticed a clock shift when QPC was deployed. It shows as a regression on the perf bots 
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-win8-dual&tests=startup.warm.dirty.blank_page%2Fwindow_display_time&rev=286928

It is not a real regression, the initial_time and initial_ticks are not properly initialized when switching to HighResolution (i.e. QPC).
This CL initializes the now_function to the HighResNowWrapper instead of setting it to RolloverProtectedNow then to the HighResNowWrapper.

By doing that, we avoid getting an incorrect initial_time and initial_ticks using the RolloverProtectedNow and avoid having to reinitialize.

BUG=158234
Committed: https://crrev.com/05399593156ef3b8d96933258cd32fe716f63461
Cr-Commit-Position: refs/heads/master@{#291974}

[fmeawad, 2014-08-07 00:18:33]

After the switch to QPC, several performance graphs regressed, forcing the
initialization seems to fix the issue. Can you take a look and suggest and
owner?

[fmeawad, 2014-08-07 20:07:18]

Hi Will,

Can you PTAL? thanks.

[willchan no longer on Chromium, 2014-08-07 20:12:00]

Sending to jar@.

If jar@ approves, TBR to me.

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

There are a lot of seemingly racy actions here, and considering the precision
that is hoped for, I really think this needs to be nailed down much better.  I'm
even suspicious that the "false regression" may be equally likely to result from
the races.

Please help me understand what this is intending to do, so I can better
understand how this change is meant to help.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:86: initial_ticks = TimeTicks::Now();
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?

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:106: if (initial_time == 0)
This sure seems scary, given that Time::Now() can be called asynchronously on
all threads.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:131: return Time(elapsed + Time(initial_time));
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?

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:362: : ticks_per_second_(0),
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).

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:409: void InitializeClock() {
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:03]

(last patch also includes a rebase).

Hi Jar,

I have addressed your issues. I have added locks. I have ran some experiments
and I did not see any performance regressions due to the added locks. (Compared
with and without, and it seems to be as performant, so I decided to go with the
simple solution).

PTAL.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:86: initial_ticks = TimeTicks::Now();
On 2014/08/08 00:30:53, jar wrote:
> 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?

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).

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:106: if (initial_time == 0)
On 2014/08/08 00:30:53, jar wrote:
> This sure seems scary, given that Time::Now() can be called asynchronously on
> all threads.  

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.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:131: return Time(elapsed + Time(initial_time));
On 2014/08/08 00:30:53, jar wrote:
> 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?

Now we read initial_ticks and initial_time in a protected way.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:362: : ticks_per_second_(0),
On 2014/08/08 00:30:53, jar wrote:
> 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).

Done.

https://codereview.chromium.org/446203002/diff/40001/base/time/time_win.cc#ne...
base/time/time_win.cc:409: void InitializeClock() {
On 2014/08/08 00:30:53, jar wrote:
> 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.)

I factored it inside its caller.

[jar (doing other things), 2014-08-15 17:40:39]

Please add a note to the description of the fix that indicates that we need to
be alert to possible performance regressions (I don't expect one... but I know
to be careful here!).  Please also review the perf bots a day after you landed
to see if you can see any impact.

With the one small item below, you should be done.  The rest looks great.
thanks!

https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc#ne...
base/time/time_win.cc:82: base::Lock initialize_clock_lock;
nit: We're not allowed to have such items at global scope (re: they are not
POD).  You need to use a singleton construct, and you are allowed to have them
at global scope.  This will ensure that the lock is initialized before use, and
will be relatively efficient in the "fast path" later in the program's lifetime.
 It would IMO be fine to use leaky lazy singleton, and leak the lock at process
termination.

[fmeawad, 2014-08-15 21:50:50]

Hi Jim,

Thank you for the prompt review. I have updated the description.
Concerning the singleton, please see my comments inlined.

https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc#ne...
base/time/time_win.cc:82: base::Lock initialize_clock_lock;
On 2014/08/15 17:40:39, jar wrote:
> nit: We're not allowed to have such items at global scope (re: they are not
> POD).  You need to use a singleton construct, and you are allowed to have them
> at global scope.  This will ensure that the lock is initialized before use,
and
> will be relatively efficient in the "fast path" later in the program's
lifetime.
>  It would IMO be fine to use leaky lazy singleton, and leak the lock at
process
> termination.

I copied the lock from rollover_lock in the same file, and I added a comment on
why to use it in this patch.

I would be happy to use the leaky lazy singleton if you think it is still
needed.

[jar (doing other things), 2014-08-15 22:28:01]

On 2014/08/15 21:50:50, fmeawad-cr wrote:
> Hi Jim,
> 
> Thank you for the prompt review. I have updated the description.
> Concerning the singleton, please see my comments inlined.
> 
> https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc
> File base/time/time_win.cc (right):
> 
>
https://codereview.chromium.org/446203002/diff/60001/base/time/time_win.cc#ne...
> base/time/time_win.cc:82: base::Lock initialize_clock_lock;
> On 2014/08/15 17:40:39, jar wrote:
> > nit: We're not allowed to have such items at global scope (re: they are not
> > POD).  You need to use a singleton construct, and you are allowed to have
them
> > at global scope.  This will ensure that the lock is initialized before use,
> and
> > will be relatively efficient in the "fast path" later in the program's
> lifetime.
> >  It would IMO be fine to use leaky lazy singleton, and leak the lock at
> process
> > termination.
> 
> I copied the lock from rollover_lock in the same file, and I added a comment
on
> why to use it in this patch.
> 
> I would be happy to use the leaky lazy singleton if you think it is still
> needed.

Thanks for the pointer.  I can LGTM with that.

I suspect the "right" thing is to create a low-level singleton <sigh>.... but
lets not go there for now.

I'm hopeful that we'll ask for the time before we go multi-threaded... which
will drive the static initializer, and all will be well.

IF we go multithreaded first, then we might race to construct the Lock, and bad
things can happen.

I believe the hang monitor does some timing before the multi-thread setup.... so
that should be keeping us safe.

[willchan no longer on Chromium, 2014-08-15 22:37:03]

+maruel, +thakis

I'm disappointed by the additional static initializer, but I see there are
tradeoffs here. It's indeed unfortunate to, for such low level code, to pay the
small overhead for a LazyInstance each time we want to access the time. Nico has
been squashing our static initializers, so I wanted to loop him in here.
Marc-Antoine also authored the LazyInstance code, so maybe he has a good idea of
how better to do this.

I also agree with jar@ that perhaps a better solution would be a one-off low
level singleton construct specifically for the time case since this is so low
level. I'd have to think more about it. In absence of other opinions, I will
approve this on Monday.

[Nico, 2014-08-15 23:17:38]

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc#ne...
base/time/time_win.cc:90: // code is low-level, and we don't want to use
Singletons here (it would be too
(fwiw, on linux or mac, "low-level" isn't an excuse. We don't allow static
initializers on mac and linux, period. Mac is at 0 static initializers, linux is
close to it.

However, on Windows we don't track static initializers, and we haven't removed
many of them, look e.g. for "Wexit-time-destructors" in
http://build.chromium.org/p/chromium.fyi/builders/Cr%20Win%20Clang/builds/94/...

The cost of static initializers isn't the cost in running the code, but that it
causes this page of code paged in during startup, and executables are often laid
out in a way that paging in the code bits with static initializers causes many
disk cache misses.)

[willchan no longer on Chromium, 2014-08-15 23:20:19]

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc#ne...
base/time/time_win.cc:90: // code is low-level, and we don't want to use
Singletons here (it would be too
On 2014/08/15 23:17:38, Nico (very away) wrote:
> (fwiw, on linux or mac, "low-level" isn't an excuse. We don't allow static
> initializers on mac and linux, period. Mac is at 0 static initializers, linux
is
> close to it.
> 
> However, on Windows we don't track static initializers, and we haven't removed
> many of them, look e.g. for "Wexit-time-destructors" in
>
http://build.chromium.org/p/chromium.fyi/builders/Cr%20Win%20Clang/builds/94/...
> 
> The cost of static initializers isn't the cost in running the code, but that
it
> causes this page of code paged in during startup, and executables are often
laid
> out in a way that paging in the code bits with static initializers causes many
> disk cache misses.)

This is very true. But what's the alternative? Do we always go through the
synchronization required to access a LazyInstance after process startup? For
something called as frequently as time code, it's not obvious to me that that's
the right tradeoff to make. I'd love to hear opinions or other alternatives
though.

[Nico, 2014-08-15 23:33:08]

On Fri, Aug 15, 2014 at 4:20 PM, <willchan@chromium.org> wrote:

>
> https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc
> File base/time/time_win.cc (right):
>
> https://codereview.chromium.org/446203002/diff/80001/base/
> time/time_win.cc#newcode90
> base/time/time_win.cc:90: // code is low-level, and we don't want to use
> Singletons here (it would be too
> On 2014/08/15 23:17:38, Nico (very away) wrote:
>
>> (fwiw, on linux or mac, "low-level" isn't an excuse. We don't allow
>>
> static
>
>> initializers on mac and linux, period. Mac is at 0 static
>>
> initializers, linux is
>
>> close to it.
>>
>
>  However, on Windows we don't track static initializers, and we haven't
>>
> removed
>
>> many of them, look e.g. for "Wexit-time-destructors" in
>>
>
> http://build.chromium.org/p/chromium.fyi/builders/Cr%
> 20Win%20Clang/builds/94/steps/compile/logs/stdio
>
>  The cost of static initializers isn't the cost in running the code,
>>
> but that it
>
>> causes this page of code paged in during startup, and executables are
>>
> often laid
>
>> out in a way that paging in the code bits with static initializers
>>
> causes many
>
>> disk cache misses.)
>>
>
> This is very true. But what's the alternative? Do we always go through
> the synchronization required to access a LazyInstance after process
> startup? For something called as frequently as time code, it's not
> obvious to me that that's the right tradeoff to make. I'd love to hear
> opinions or other alternatives though.
>

You can have an init() method that you can call early manually, I suppose.
(Is that better than an initializer? For many libraries, the init work can
happen after the browser window is up. Several libraries are only needed in
some process types. "No initializers, ever" is an easy to remember rule.
Only the last of these probably applies to this file, though.)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-08-15 23:56:52]

On Fri, Aug 15, 2014 at 4:33 PM, Nico Weber <thakis@chromium.org> wrote:

> On Fri, Aug 15, 2014 at 4:20 PM, <willchan@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/446203002/diff/80001/base/
>> time/time_win.cc
>> File base/time/time_win.cc (right):
>>
>> https://codereview.chromium.org/446203002/diff/80001/base/
>> time/time_win.cc#newcode90
>> base/time/time_win.cc:90: // code is low-level, and we don't want to use
>> Singletons here (it would be too
>> On 2014/08/15 23:17:38, Nico (very away) wrote:
>>
>>> (fwiw, on linux or mac, "low-level" isn't an excuse. We don't allow
>>>
>> static
>>
>>> initializers on mac and linux, period. Mac is at 0 static
>>>
>> initializers, linux is
>>
>>> close to it.
>>>
>>
>>  However, on Windows we don't track static initializers, and we haven't
>>>
>> removed
>>
>>> many of them, look e.g. for "Wexit-time-destructors" in
>>>
>>
>> http://build.chromium.org/p/chromium.fyi/builders/Cr%
>> 20Win%20Clang/builds/94/steps/compile/logs/stdio
>>
>>  The cost of static initializers isn't the cost in running the code,
>>>
>> but that it
>>
>>> causes this page of code paged in during startup, and executables are
>>>
>> often laid
>>
>>> out in a way that paging in the code bits with static initializers
>>>
>> causes many
>>
>>> disk cache misses.)
>>>
>>
>> This is very true. But what's the alternative? Do we always go through
>> the synchronization required to access a LazyInstance after process
>> startup? For something called as frequently as time code, it's not
>> obvious to me that that's the right tradeoff to make. I'd love to hear
>> opinions or other alternatives though.
>>
>
> You can have an init() method that you can call early manually, I suppose.
> (Is that better than an initializer? For many libraries, the init work can
> happen after the browser window is up. Several libraries are only needed in
> some process types. "No initializers, ever" is an easy to remember rule.
> Only the last of these probably applies to this file, though.)
>

Agreed, this is a viable alternative. Do you think it's better? If you
think it's potentially better, maybe the right thing to do is punt work to
fmeawad@ to figure out the earliest invocation of time to see if manual
invocation is reasonable. I'm a bit worried that that will be fragile
(maybe we can add assertions to prevent bugs).

I'm hopeful that eventually when we can use C++11 on Windows, this will
become a non-issue, since we can use the linker initialized mutex. So even
if we don't like the current tradeoff we might have to make, there's a
light at the end of the tunnel. But you know better than I do if the light
is indeed there.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[M-A Ruel, 2014-08-16 00:40:14]

Here's some code. It's probably overkill but hey. The current headers do not
expose InterlockedCompareExchange64(). At worst it's a function call in the DLL.
Probably good to step in assembler with a debugger on optimized code..

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/80001/base/time/time_win.cc#ne...
base/time/time_win.cc:97: base::AutoLock locked(initialize_clock_lock);
Because I'm a nice guy, here's the code you actually want. Please pardon my
rusty C++;

// Yes, it's a good use of volatile.
volatile int64 initial_ticks = 0;
volatile int64 initial_time = 0;

// Sets the clock only if it was set to 0.
void InitializeClock() {
  MemoryBarrier();
  if (initial_time != 0) {
    return;
  }
  for (;;) {
    int64 ticks = TimeTicks::Now();
    int time = CurrentWallclockMicroseconds();
    if (0 != InterlockedCompareExchange64(&initial_ticks, ticks, 0) {
      // We lost.
      return;
    }
    if (0 != InterlockedCompareExchange64(&initial_time, time, 0) {
      // That's disturbing but nothing can be done.
    }
  }
}

(int64, int64) ReadClock() {
  for (;;) {
    MemoryBarrier();
    int64 time = initial_time;
    int64 ticks = initial_ticks;
    MemoryBarrier();
    int64 time2 = initial_time;
    int64 ticks2 = initial_ticks;
    if (time == time2 && ticks == ticks2) {
      return time, ticks; // Remind me it's not Go.
    }
  }
}

// Forcibly set the clock.
void SetClock() {
  for (;;) {
    int64 ticks = TimeTicks::Now();
    int time = CurrentWallclockMicroseconds();
    MemoryBarrier();
    int64 old_time = initial_time;
    int64 old_ticks = initial_ticks;
    if (old_ticks != InterlockedCompareExchange64(&initial_ticks, ticks,
old_ticks) {
      continue;
    }
    if (old_time != InterlockedCompareExchange64(&initial_time, time, old_time)
{
      // That's disturbing but nothing can be done.
      continue;
    }
    return;
  }
}

Given with no guarantee whatsoever. May break, kill or cause a thermonuclear
war. InterlockedCompareExchange64() IIRC is fine because we already require
SSE2.

If you cared about perf, your wouldn't use a class for TimeTicks and only use
raw int64. But that's C++, not Go, so you can't do strongly typed typedef. Oh
well. :)

[fmeawad, 2014-08-21 00:43:16]

Jim, Will,

Instead of adding an extra InitializeClock call that may add a race condition, I
made the system using the HighRes clock from get go instead. This way, the first
initialization would use the HighRes clock and the InitializeClock call count
would not increase.

I tracked when do we call InitializeClock, and it is called from the main thread
2 times on program start, and every minute after the first minute to avoid
drift. + any calls from NowFromSystemTime.

I am looking in removing the periodic call as I do not think we need it with
QPC. Similarly with NowFromSystemTime. But I think those should go in a
different CL.

[jar (doing other things), 2014-08-22 19:11:53]

I think this is nice forward progress... improving code only now... and not
adding anything that folks could argue about... so....

LGTM (%tiny nit). (WillChan can rubber stamp for base).

Please file a bug to work on the thread safety of the now function.

https://codereview.chromium.org/446203002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/100001/base/time/time_win.cc#n...
base/time/time_win.cc:40: 
nit: kill spaces (which you noticed!)

[fmeawad, 2014-08-22 21:13:12]

+jam for 
chrome\browser\
content\app\

https://codereview.chromium.org/446203002/diff/100001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/100001/base/time/time_win.cc#n...
base/time/time_win.cc:40: 
On 2014/08/22 19:11:53, jar (gone till 9-27-2014) wrote:
> nit: kill spaces (which you noticed!)

Done.

[jam, 2014-08-25 01:57:49]

lgtm

[cpu_(ooo_6.6-7.5), 2014-08-25 18:22:07]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-08-25 18:23:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fmeawad@chromium.org/446203002/120001

[fmeawad, 2014-08-25 18:36:31]

On 2014/08/25 18:23:14, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/fmeawad@chromium.org/446203002/120001

Hey Will,
Can you have another look?

[commit-bot: I haz the power, 2014-08-25 19:28:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-25 19:30:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 19:30:40]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[willchan no longer on Chromium, 2014-08-25 23:07:25]

Rubberstamping the LGTM from jar@

That said, I note again that there's a static initializer getting created here,
and that's problematic for Windows. Since you guys own startup time too, I'm
fine with you guys making this tradeoff if that's what you want. I just want to
note it.

https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc#n...
base/time/time_win.cc:461: NowFunction now_function = GetNowFunction();
FYI: this results in a static initializer. This may slow down Windows Chrome
startup. You may want to use a function local static instead since it won't
create a static initializer. Of course, since we don't lock function local
static initialization, that will possibly lead to races, depending on how this
code gets run.

[M-A Ruel, 2014-08-26 02:03:19]

https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc#n...
base/time/time_win.cc:461: NowFunction now_function = GetNowFunction();
On 2014/08/25 23:07:25, willchan wrote:
> FYI: this results in a static initializer. This may slow down Windows Chrome
> startup. You may want to use a function local static instead since it won't
> create a static initializer. Of course, since we don't lock function local
> static initialization, that will possibly lead to races, depending on how this
> code gets run.

Furthermore, the static initializer is unneeded:

TimeDelta InitialNowFunction() {
  if (!CPUReliablySupportsHighResTime()) {
    InterlockedExchange(&now_function, RolloverProtectedNow);
    return RolloverProtectedNow();
  }
  InterlockedExchange(&now_function, HighResNowWrapper);
  return HighResNowWrapper();
}

NowFunction now_function = InitialNowFunction;

[M-A Ruel, 2014-08-26 02:03:52]

On 2014/08/26 02:03:19, M-A Ruel wrote:
> https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc
> File base/time/time_win.cc (right):
> 
>
https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc#n...
> base/time/time_win.cc:461: NowFunction now_function = GetNowFunction();
> On 2014/08/25 23:07:25, willchan wrote:
> > FYI: this results in a static initializer. This may slow down Windows Chrome
> > startup. You may want to use a function local static instead since it won't
> > create a static initializer. Of course, since we don't lock function local
> > static initialization, that will possibly lead to races, depending on how
this
> > code gets run.
> 
> Furthermore, the static initializer is unneeded:
> 
> TimeDelta InitialNowFunction() {
>   if (!CPUReliablySupportsHighResTime()) {
>     InterlockedExchange(&now_function, RolloverProtectedNow);
>     return RolloverProtectedNow();
>   }
>   InterlockedExchange(&now_function, HighResNowWrapper);
>   return HighResNowWrapper();
> }
> 
> NowFunction now_function = InitialNowFunction;

It's not InterlockedExchange() that is needed but you see the idea.

[fmeawad, 2014-08-26 20:04:02]

On 2014/08/26 02:03:52, M-A Ruel wrote:
> On 2014/08/26 02:03:19, M-A Ruel wrote:
> > https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc
> > File base/time/time_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/446203002/diff/120001/base/time/time_win.cc#n...
> > base/time/time_win.cc:461: NowFunction now_function = GetNowFunction();
> > On 2014/08/25 23:07:25, willchan wrote:
> > > FYI: this results in a static initializer. This may slow down Windows
Chrome
> > > startup. You may want to use a function local static instead since it
won't
> > > create a static initializer. Of course, since we don't lock function local
> > > static initialization, that will possibly lead to races, depending on how
> this
> > > code gets run.
> > 
> > Furthermore, the static initializer is unneeded:
> > 
> > TimeDelta InitialNowFunction() {
> >   if (!CPUReliablySupportsHighResTime()) {
> >     InterlockedExchange(&now_function, RolloverProtectedNow);
> >     return RolloverProtectedNow();
> >   }
> >   InterlockedExchange(&now_function, HighResNowWrapper);
> >   return HighResNowWrapper();
> > }
> > 
> > NowFunction now_function = InitialNowFunction;
> 
> It's not InterlockedExchange() that is needed but you see the idea.

The time function gets called really early, having the cost paid in startup is
acceptable in this cases. But I will start tacking static initializers on
windows as well, will also monitor for any noticeable regressions.

[fmeawad, 2014-08-26 20:04:10]

The CQ bit was checked by fmeawad@chromium.org

[commit-bot: I haz the power, 2014-08-26 20:05:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fmeawad@chromium.org/446203002/120001

[commit-bot: I haz the power, 2014-08-26 20:11:39]

Committed patchset #7 (120001) as 10c40c221c314e41add0a5b4df1ee7467681a430

[fmeawad, 2014-08-28 00:00:45]

A revert of this CL (patchset #7) has been created in
https://codereview.chromium.org/516693002/ by fmeawad@chromium.org.

The reason for reverting is: It causes a Canary crash

https://code.google.com/p/chromium/issues/detail?id=408354

The added static initializer causes a crash in libpeerconnetion used by WebRTC

Fix to reland: remove static initializer..

[commit-bot: I haz the power, 2014-09-10 02:45:22]

Patchset 8 (id:??) landed as
https://crrev.com/05399593156ef3b8d96933258cd32fe716f63461
Cr-Commit-Position: refs/heads/master@{#291974}