Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 489793003: Fix logic on high Windows resolution timer and have (Closed)

Created:
5 years, 11 months ago by cpu_(ooo_6.6-7.5)
Modified:
5 years, 11 months ago
Reviewers:
jamesr, Nico
CC:
chromium-reviews, erikwright+watch_chromium.org, fmeawad
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix logic on high Windows resolution timer and have two possible period values for timeBeginPeriod and timeEndPeriod. Currently while on battery we disable calls to timeBeginPeriod which make the windows timers have 15ms resolution. This change makes it so when EnableHighResolutionTimer(true) which is on AC power the timer is 1ms and EnableHighResolutionTimer(false) is 4ms. This should provide significant power savings while meeting some timer resolution requirements needed by the GPU compositor. But also this CL fixes the following: EnableHighResolutionTimer() and ActivateHighResolutionTimer() are pretty broken. This CL fixes most issues: 1- The existing logic fails to account that EnableHighResolutionTimer can be called while the browser is running 2- All related functions need to be thread safe. 3- ActivateHighResolutionTimer was buggy. BUG=153139 Committed: https://crrev.com/be8f40e67f300e9452cfabb3ad594d907cfaa947 Cr-Commit-Position: refs/heads/master@{#292094}

Patch Set 1 #

Total comments: 2

Patch Set 2 : lock fixes and utest #

Total comments: 3

Patch Set 3 : nits and leaky singleton #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -64 lines) Patch
M base/time/time.h View 2 chunks +1 line, -17 lines 0 comments Download
M base/time/time_win.cc View 1 2 4 chunks +46 lines, -32 lines 0 comments Download
M base/timer/hi_res_timer_manager_unittest.cc View 1 2 chunks +12 lines, -15 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
cpu_(ooo_6.6-7.5)
cpu@chromium.org changed reviewers: + jamesr@chromium.org, thakis@chromium.org
5 years, 11 months ago (2014-08-26 03:28:35 UTC) #1
cpu_(ooo_6.6-7.5)
ptal.
5 years, 11 months ago (2014-08-26 03:35:46 UTC) #2
jamesr
Can you re-enable HiResTimerManagerTest.DISABLED_ToggleOnOff in base/timer/hi_res_timer_manager_unittest.cc and close http://crbug.com/114048 as well? Or if this doesn't ...
5 years, 11 months ago (2014-08-26 05:03:17 UTC) #3
cpu_(ooo_6.6-7.5)
Sadface indeed. Went back to locking the whole call. Adding the disabled test with some ...
5 years, 11 months ago (2014-08-26 19:43:38 UTC) #4
jamesr
lgtm https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc#newcode180 base/time/time_win.cc:180: if (enable) { this bit might be worth ...
5 years, 11 months ago (2014-08-26 19:47:40 UTC) #5
Nico
rs-lgtm https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc#newcode97 base/time/time_win.cc:97: base::LazyInstance<base::Lock> g_high_res_lock = LAZY_INSTANCE_INITIALIZER; Can you make this ...
5 years, 11 months ago (2014-08-26 20:15:59 UTC) #6
cpu_(ooo_6.6-7.5)
The CQ bit was checked by cpu@chromium.org
5 years, 11 months ago (2014-08-27 01:29:32 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cpu@chromium.org/489793003/40001
5 years, 11 months ago (2014-08-27 01:30:49 UTC) #8
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win ...
5 years, 11 months ago (2014-08-27 03:00:11 UTC) #9
commit-bot: I haz the power
Committed patchset #3 (40001) as 3f4fda54db1983799fe12606b0d154f8f4d816eb
5 years, 11 months ago (2014-08-27 03:58:55 UTC) #10
cpu_(ooo_6.6-7.5)
Patchset #4 (id:60001) has been deleted
5 years, 11 months ago (2014-08-28 01:14:33 UTC) #11
commit-bot: I haz the power
5 years, 11 months ago (2014-09-10 02:49:37 UTC) #12
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/be8f40e67f300e9452cfabb3ad594d907cfaa947
Cr-Commit-Position: refs/heads/master@{#292094}

Powered by Google App Engine
This is Rietveld 408576698