Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951413003:
Add UMA for High Resolution Timer Usage (Closed)
DescriptionAdd UMA for High Resolution Timer Usage
The goal of this change is to understand the pattern on activating the
high resolution system timer on Windows and to track the improvement
when we switch to GPU V-sync and improve task coalescing.
The change consists of two parts:
1) In Time class I added two methods that allow to track the high
resolution timer activation over an arbitrary time interval:
void ResetHighResolutionTimerUsage();
double GetHighResolutionTimerUsage();
The first method resets the accumulated usage value and the second
one returns the percentage of time the high resolution timer was
activated since the last reset.
2) HighResolutionTimerManager looks like a perfect place to call
the two new Time class functions.
In general HighResolutionTimerManager takes the snapshot of the
usage and logs the UMA metric every 10 minutes. There is a special
logic for dealing with suspend / resume because we don't want to
count the time when the machine is sleeping.
I should mention that when fixing this bug I've noticed that
HighResolutionTimerManager was missing in the GPU process. The main
responsibility of HighResolutionTimerManager before this change was
to change high-resolution timer interval between 1 ms and 4 ms
depending on whether the machine is on battery power. The lack
of HighResolutionTimerManager in the GPU process could theoretically
result in running the high resolution timer at 4 ms interval even
when the machine is on AC power. But in reality I think that wouldn't
matter because other Chrome processes would likely have more impact
on the actual high resolution timer frequency.
BUG=736489, 736490
Review-Url: https://codereview.chromium.org/2951413003
Cr-Commit-Position: refs/heads/master@{#484626}
Committed: https://chromium.googlesource.com/chromium/src/+/61507093a130b03867bb13996a2af515b67e4972
Patch Set 1 #Patch Set 2 : Added HiResTimerManager to GPU process and fixed HiResTimerManagerTest.ToggleOnOff test #Patch Set 3 : Self review #Patch Set 4 : Added suspend / resume logic #
Total comments: 16
Patch Set 5 : Addressed CR feedback and added TaskManager instantiation in NaCl #Patch Set 6 : Switched to using Timer in HighResolutionTimerManager #
Total comments: 1
Patch Set 7 : Use RepeatingTimer #Patch Set 8 : Removed NaCl changes #
Messages
Total messages: 44 (33 generated)
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add UMA for High Resolution Timer BUG= ========== to ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489 ==========
Description was changed from ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489 ========== to ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489 ==========
stanisc@chromium.org changed reviewers: + gab@chromium.org, jbauman@chromium.org, rkaplow@chromium.org
gab@chromium.org: Please review changes in base/time and base/timer jbauman@chromium.org: Please review changes in content/gpu/gpu_main.cc rkaplow@chromium.org: Please review changes in histograms.xml
Description was changed from ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489 ========== to ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489,736490 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm histogram lg https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:86622: + Percentage of elapsed time the high resolution timer is activated for each Pretty minor - I think this can be rephrased in a way that is a bit easier to grok. So this is record on 10m intervals - so this is the percentage of that interval that it is activated? And not sure how this works for 'each' Chrome process - does each record seperately?
lgtm
https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:86622: + Percentage of elapsed time the high resolution timer is activated for each On 2017/06/27 21:49:18, rkaplow wrote: > Pretty minor - I think this can be rephrased in a way that is a bit easier to > grok. So this is record on 10m intervals - so this is the percentage of that > interval that it is activated? And not sure how this works for 'each' Chrome > process - does each record seperately? Yeah, the high resolution timer is actually activated if any process requests that, but I don't think we can join the histogram data from all process easily without doing something complicated like sending all the data over IPC to the browser process and joining all the activation intervals there. So we thought that it isn't worth the complexity and it should be good enough to just log the UMA from each process individually and let the the data be aggregated at the histogram level. How about this version: Percentage of elapsed time the high resolution timer is activated. The usage is reported by each of Chrome processes individually (without aggregation) and logged every 10 minutes.
On 2017/06/27 22:07:31, stanisc wrote: > https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... > tools/metrics/histograms/histograms.xml:86622: + Percentage of elapsed time > the high resolution timer is activated for each > On 2017/06/27 21:49:18, rkaplow wrote: > > Pretty minor - I think this can be rephrased in a way that is a bit easier to > > grok. So this is record on 10m intervals - so this is the percentage of that > > interval that it is activated? And not sure how this works for 'each' Chrome > > process - does each record seperately? > > Yeah, the high resolution timer is actually activated if any process requests > that, but I don't think we can join the histogram data from all process easily > without doing something complicated like sending all the data over IPC to the > browser process and joining all the activation intervals there. So we thought > that it isn't worth the complexity and it should be good enough to just log the > UMA from each process individually and let the the data be aggregated at the > histogram level. > > How about this version: > Percentage of elapsed time the high resolution timer is activated. The usage is > reported by each of Chrome processes individually (without aggregation) and > logged every 10 minutes. I find that clearer. thanks
https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:558: // activated. Also document that Get() is invalid before Reset() was called at least once? (although your test verify that case, it's undefined to use Get() before Reset() when the highres timer was activated already) https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:561: #endif #endif // defined(OS_WIN) (this block is becoming larger and larger and a closing comment is now justified) https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:307: EXPECT_EQ(0.0, Time::GetHighResolutionTimerUsage()); Why is this guaranteed? Can't Now() tick between Reset() and Get()? https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:325: EXPECT_GT(usage1, 0.0); Precise enough to use EXPECT_NEAR? (and below) https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_unittest.cc:27: base::test::ScopedTaskScheduler task_scheduler(&loop); ScopedTaskScheduler is deprecated. Please replace the MessageLoop with base::test::ScopedTaskEnvironment instead. https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_win.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_win.cc:76: ScheduleHighResolutionTimerUsageReport(); It's possible that the task fired from here runs at the same time as the recurring task. Since Time::Get...() isn't thread-safe you at least need to post to a SequencedTaskRunner (e.g. anonymous LazySequencedTaskRunner). Overall though I think base::Timer is probably what you want (assuming HighResolutionTimerManager is constructed and notified on the same sequence then all you need is to Timer::Reset() to avoid running the delayed task).
https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:558: // activated. Also document that Get() is invalid before Reset() was called at least once? (although your test verify that case, it's undefined to use Get() before Reset() when the highres timer was activated already) https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:561: #endif #endif // defined(OS_WIN) (this block is becoming larger and larger and a closing comment is now justified) https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:307: EXPECT_EQ(0.0, Time::GetHighResolutionTimerUsage()); Why is this guaranteed? Can't Now() tick between Reset() and Get()? https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:325: EXPECT_GT(usage1, 0.0); Precise enough to use EXPECT_NEAR? (and below) https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_unittest.cc:27: base::test::ScopedTaskScheduler task_scheduler(&loop); ScopedTaskScheduler is deprecated. Please replace the MessageLoop with base::test::ScopedTaskEnvironment instead. https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_win.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_win.cc:76: ScheduleHighResolutionTimerUsageReport(); It's possible that the task fired from here runs at the same time as the recurring task. Since Time::Get...() isn't thread-safe you at least need to post to a SequencedTaskRunner (e.g. anonymous LazySequencedTaskRunner). Overall though I think base::Timer is probably what you want (assuming HighResolutionTimerManager is constructed and notified on the same sequence then all you need is to Timer::Reset() to avoid running the delayed task).
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
stanisc@chromium.org changed reviewers: + dschuff@chromium.org
dschuff@chromium.org: OWNER for NaCl changes. gab@, please also review if the way I create TaskScheduler instance in NaCl process is a correct one. NaCl specific browser tests seem to be happy with it. https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:558: // activated. On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > Also document that Get() is invalid before Reset() was called at least once? > (although your test verify that case, it's undefined to use Get() before Reset() > when the highres timer was activated already) Done. https://codereview.chromium.org/2951413003/diff/60001/base/time/time.h#newcod... base/time/time.h:561: #endif On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > #endif // defined(OS_WIN) > > (this block is becoming larger and larger and a closing comment is now > justified) Done. https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:307: EXPECT_EQ(0.0, Time::GetHighResolutionTimerUsage()); On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > Why is this guaranteed? Can't Now() tick between Reset() and Get()? Since the high resolution timer isn't activated yet it doesn't matter how much timer elapses since the reset. I'll change the test comments to make that more obvious. https://codereview.chromium.org/2951413003/diff/60001/base/time/time_win_unit... base/time/time_win_unittest.cc:325: EXPECT_GT(usage1, 0.0); On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > Precise enough to use EXPECT_NEAR? (and below) In my experience anything involving time checks is very unreliable in tests that run in swarming environment. That Sleep(20) could sleep for an arbitrary amount on time so the actual usage could be pretty far from 50%. All I can tell for sure that it must be greater than 0% and less than 100%. I considered adding a way to override Now() function similar to TimeTicks::SetMockTickFunction(). That would be the most reliable way to deal with time. But that seemed fragile because the test infrastructure likely uses TimeTicks::Now too. https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_unittest.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_unittest.cc:27: base::test::ScopedTaskScheduler task_scheduler(&loop); On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > ScopedTaskScheduler is deprecated. Please replace the MessageLoop with > base::test::ScopedTaskEnvironment instead. Done. https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_win.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_win.cc:76: ScheduleHighResolutionTimerUsageReport(); On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > It's possible that the task fired from here runs at the same time as the > recurring task. Since Time::Get...() isn't thread-safe you at least need to post > to a SequencedTaskRunner (e.g. anonymous LazySequencedTaskRunner). > > Overall though I think base::Timer is probably what you want (assuming > HighResolutionTimerManager is constructed and notified on the same sequence then > all you need is to Timer::Reset() to avoid running the delayed task). report_id was supposed to protect from running concurrent tasks, but you are right that using base::Timer is much simpler in this case. Done. https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2951413003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:86622: + Percentage of elapsed time the high resolution timer is activated for each On 2017/06/27 22:07:31, stanisc wrote: > On 2017/06/27 21:49:18, rkaplow wrote: > > Pretty minor - I think this can be rephrased in a way that is a bit easier to > > grok. So this is record on 10m intervals - so this is the percentage of that > > interval that it is activated? And not sure how this works for 'each' Chrome > > process - does each record seperately? > > Yeah, the high resolution timer is actually activated if any process requests > that, but I don't think we can join the histogram data from all process easily > without doing something complicated like sending all the data over IPC to the > browser process and joining all the activation intervals there. So we thought > that it isn't worth the complexity and it should be good enough to just log the > UMA from each process individually and let the the data be aggregated at the > histogram level. > > How about this version: > Percentage of elapsed time the high resolution timer is activated. The usage is > reported by each of Chrome processes individually (without aggregation) and > logged every 10 minutes. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ nit https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... File base/timer/hi_res_timer_manager_win.cc (right): https://codereview.chromium.org/2951413003/diff/60001/base/timer/hi_res_timer... base/timer/hi_res_timer_manager_win.cc:76: ScheduleHighResolutionTimerUsageReport(); On 2017/06/28 22:28:35, stanisc wrote: > On 2017/06/28 16:39:33, gab (slow Tue-Wed) wrote: > > It's possible that the task fired from here runs at the same time as the > > recurring task. Since Time::Get...() isn't thread-safe you at least need to > post > > to a SequencedTaskRunner (e.g. anonymous LazySequencedTaskRunner). > > > > Overall though I think base::Timer is probably what you want (assuming > > HighResolutionTimerManager is constructed and notified on the same sequence > then > > all you need is to Timer::Reset() to avoid running the delayed task). > > report_id was supposed to protect from running concurrent tasks, but you are > right that using base::Timer is much simpler in this case. Done. FWIW atomics merely provide eventual consistency so it was still possible for two threads to each see the value they expected at the same time and execute in parallel. https://codereview.chromium.org/2951413003/diff/100001/base/timer/hi_res_time... File base/timer/hi_res_timer_manager_win.cc (right): https://codereview.chromium.org/2951413003/diff/100001/base/timer/hi_res_time... base/timer/hi_res_timer_manager_win.cc:35: true /* is_repeating */) { Use base::RepeatingTimer?
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by stanisc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, jbauman@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2951413003/#ps140001 (title: "Removed NaCl changes")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499358756566280,
"parent_rev": "b5230e468c0236f021df3a4939c99f9ab86433af", "commit_rev":
"61507093a130b03867bb13996a2af515b67e4972"}
Message was sent while issue was closed.
Description was changed from ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489,736490 ========== to ========== Add UMA for High Resolution Timer Usage The goal of this change is to understand the pattern on activating the high resolution system timer on Windows and to track the improvement when we switch to GPU V-sync and improve task coalescing. The change consists of two parts: 1) In Time class I added two methods that allow to track the high resolution timer activation over an arbitrary time interval: void ResetHighResolutionTimerUsage(); double GetHighResolutionTimerUsage(); The first method resets the accumulated usage value and the second one returns the percentage of time the high resolution timer was activated since the last reset. 2) HighResolutionTimerManager looks like a perfect place to call the two new Time class functions. In general HighResolutionTimerManager takes the snapshot of the usage and logs the UMA metric every 10 minutes. There is a special logic for dealing with suspend / resume because we don't want to count the time when the machine is sleeping. I should mention that when fixing this bug I've noticed that HighResolutionTimerManager was missing in the GPU process. The main responsibility of HighResolutionTimerManager before this change was to change high-resolution timer interval between 1 ms and 4 ms depending on whether the machine is on battery power. The lack of HighResolutionTimerManager in the GPU process could theoretically result in running the high resolution timer at 4 ms interval even when the machine is on AC power. But in reality I think that wouldn't matter because other Chrome processes would likely have more impact on the actual high resolution timer frequency. BUG=736489,736490 Review-Url: https://codereview.chromium.org/2951413003 Cr-Commit-Position: refs/heads/master@{#484626} Committed: https://chromium.googlesource.com/chromium/src/+/61507093a130b03867bb13996a2a... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/61507093a130b03867bb13996a2a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
