|
|
Created:
6 years, 4 months ago by fmeawad Modified:
6 years, 3 months ago Reviewers:
willchan no longer on Chromium, cpu_(ooo_6.6-7.5), jam, M-A Ruel, Nico, jar (doing other things) CC:
chromium-reviews, darin-cc_chromium.org, jam, tonyg, brianderson, cpu_(ooo_6.6-7.5) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionWe 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}
Patch Set 1 #Patch Set 2 : Initialize early, instead of initializing per call #Patch Set 3 : Remove added NowFromSystemTime #
Total comments: 10
Patch Set 4 : Add thread safety to InitializeClock #
Total comments: 2
Patch Set 5 : Adding a comment to explain the global lock #
Total comments: 3
Patch Set 6 : Enable HiRes by default instead of using LowRes then switching to it. #
Total comments: 2
Patch Set 7 : nit fixes #
Total comments: 2
Patch Set 8 : Remove the now_function static Initializer #
Messages
Total messages: 33 (0 generated)
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?
Hi Will, Can you PTAL? thanks.
Sending to jar@. If jar@ approves, TBR to me.
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.)
(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.
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.
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.
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.
+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.
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.)
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.
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.
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.
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. :)
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.
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!)
+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.
lgtm
The CQ bit was checked by cpu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/446203002/120001
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?
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...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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.
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;
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.
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.
The CQ bit was checked by fmeawad@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/446203002/120001
Message was sent while issue was closed.
Committed patchset #7 (120001) as 10c40c221c314e41add0a5b4df1ee7467681a430
Message was sent while issue was closed.
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..
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/05399593156ef3b8d96933258cd32fe716f63461 Cr-Commit-Position: refs/heads/master@{#291974} |