Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2752573002:
Add BeginFrameLatency UMA (Closed)
DescriptionAdd BeginFrameLatency UMA
The purpose of this UMA is to estimate v-sync latency i.e. the time
from hardware v-sync which is represented by BeginFrame timestamp
to the time BeginFrame is handled in Scheduler.
This metric will be useful to understand impact of GPU V-sync which
has been recently landed behind a feature switch.
BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2752573002
Cr-Commit-Position: refs/heads/master@{#457190}
Committed: https://chromium.googlesource.com/chromium/src/+/2b0182bca127df14fa7f3c8becb8ff682b3f1bd6
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed CR feedback #
Total comments: 2
Patch Set 3 : Skip frames that originate from BeginFrameArgs::MISSED args #
Total comments: 2
Messages
Total messages: 32 (22 generated)
Description was changed from ========== Added BeginFrameLatency UMA BUG= ========== to ========== Added BeginFrameLatency UMA BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Added BeginFrameLatency UMA BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add BeginFrameLatency UMA The purpose of this UMA is to estimate v-sync latency i.e. the time from hardware v-sync which is represented by BeginFrame timestamp to the time BeginFrame is handled in Scheduler. This metric will be useful to understand impact of GPU V-sync which has been recently landed behind a feature switch. BUG=701181 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
stanisc@chromium.org changed reviewers: + ajuma@chromium.org, asvitkine@chromium.org
Description was changed from ========== Add BeginFrameLatency UMA The purpose of this UMA is to estimate v-sync latency i.e. the time from hardware v-sync which is represented by BeginFrame timestamp to the time BeginFrame is handled in Scheduler. This metric will be useful to understand impact of GPU V-sync which has been recently landed behind a feature switch. BUG=701181 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add BeginFrameLatency UMA The purpose of this UMA is to estimate v-sync latency i.e. the time from hardware v-sync which is represented by BeginFrame timestamp to the time BeginFrame is handled in Scheduler. This metric will be useful to understand impact of GPU V-sync which has been recently landed behind a feature switch. BUG=701181 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ajuma@, please review cc\scheduler\compositor_timing_history.cc asvitkine@, please review tools\metrics\histograms\histograms.xml
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
lgtm https://codereview.chromium.org/2752573002/diff/1/cc/scheduler/compositor_tim... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2752573002/diff/1/cc/scheduler/compositor_tim... cc/scheduler/compositor_timing_history.cc:204: "Scheduling.Renderer.BeginMainFrameLatency", delta); Bikeshed: given the number of BeginFrame timing metrics we have, a more specific name might make it easier to understand how this differs from the other metrics. Maybe something like VsyncToBeginMainFrameLatency or VsyncToBeginMainFrameStartDuration? https://codereview.chromium.org/2752573002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752573002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60337: +<histogram name="Scheduling.BeginMainFrameLatency2" units="microseconds"> The '2' shouldn't be needed here (since we don't have an existing BeginMainFrameLatency metric), but either way the name here should match the one used in compositor_timing_history.cc.
stanisc@chromium.org changed reviewers: + isherman@chromium.org - asvitkine@chromium.org
Addressed feedback from ajuma@. +isherman@ for histograms.xml review (asvitkine@ seems to be busy). https://codereview.chromium.org/2752573002/diff/1/cc/scheduler/compositor_tim... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2752573002/diff/1/cc/scheduler/compositor_tim... cc/scheduler/compositor_timing_history.cc:204: "Scheduling.Renderer.BeginMainFrameLatency", delta); On 2017/03/14 14:54:10, ajuma wrote: > Bikeshed: given the number of BeginFrame timing metrics we have, a more specific > name might make it easier to understand how this differs from the other metrics. > Maybe something like VsyncToBeginMainFrameLatency or > VsyncToBeginMainFrameStartDuration? OK. Changed this to VsyncToBeginMainFrameLatency. https://codereview.chromium.org/2752573002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752573002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:60337: +<histogram name="Scheduling.BeginMainFrameLatency2" units="microseconds"> On 2017/03/14 14:54:10, ajuma wrote: > The '2' shouldn't be needed here (since we don't have an existing > BeginMainFrameLatency metric), but either way the name here should match the one > used in compositor_timing_history.cc. Agree, '2' shouldn't be needed but it is added by UMA_HISTOGRAM_CUSTOM_TIMES_DURATION macro so I had no other choice.
Metrics LGTM https://codereview.chromium.org/2752573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:60636: +<histogram name="Scheduling.VsyncToBeginMainFrameLatency2" units="microseconds"> Hmm, is it useful to include a "2" in this metric name? I do see that the macro you used does so... but I wonder whether that's really working as intended.
https://codereview.chromium.org/2752573002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2752573002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:60636: +<histogram name="Scheduling.VsyncToBeginMainFrameLatency2" units="microseconds"> On 2017/03/14 21:32:11, Ilya Sherman wrote: > Hmm, is it useful to include a "2" in this metric name? I do see that the macro > you used does so... but I wonder whether that's really working as intended. My understanding is that the macro was changed at some point to collect metrics in a more data bandwidth efficient way and "2" was appended to all metric names at that point by changing the macro. For this new metric appending "2" obviously isn't very useful, but I didn't want to duplicate the macro just for this one metric. At least it is consistent with all other scheduler metrics. I verified that it works as intended locally by looking at chrome://histograms.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
stanisc@chromium.org changed reviewers: + brianderson@chromium.org
Added a change suggested by brianderson@
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 ajuma@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2752573002/#ps40001 (title: "Skip frames that originate from BeginFrameArgs::MISSED args")
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": 40001, "attempt_start_ts": 1489609524741410,
"parent_rev": "ff386cfefd3fe0f9cef72ff28f7c7f09d972608d", "commit_rev":
"2b0182bca127df14fa7f3c8becb8ff682b3f1bd6"}
Message was sent while issue was closed.
Description was changed from ========== Add BeginFrameLatency UMA The purpose of this UMA is to estimate v-sync latency i.e. the time from hardware v-sync which is represented by BeginFrame timestamp to the time BeginFrame is handled in Scheduler. This metric will be useful to understand impact of GPU V-sync which has been recently landed behind a feature switch. BUG=701181 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add BeginFrameLatency UMA The purpose of this UMA is to estimate v-sync latency i.e. the time from hardware v-sync which is represented by BeginFrame timestamp to the time BeginFrame is handled in Scheduler. This metric will be useful to understand impact of GPU V-sync which has been recently landed behind a feature switch. BUG=701181 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2752573002 Cr-Commit-Position: refs/heads/master@{#457190} Committed: https://chromium.googlesource.com/chromium/src/+/2b0182bca127df14fa7f3c8becb8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2b0182bca127df14fa7f3c8becb8...
Message was sent while issue was closed.
https://codereview.chromium.org/2752573002/diff/40001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2752573002/diff/40001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:469: void CompositorTimingHistory::WillBeginImplFrame( You may want to track the latency of this entry point instead (or also). The BeginMainFrame can be throttled because of back pressure, which will make latency look worse than it otherwise would. BeginImplFrame always runs as soon as we get the BeginFrame message, so its latency will better reflect OS scheduling badness.
Message was sent while issue was closed.
https://codereview.chromium.org/2752573002/diff/40001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2752573002/diff/40001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:469: void CompositorTimingHistory::WillBeginImplFrame( On 2017/03/15 21:08:05, brianderson wrote: > You may want to track the latency of this entry point instead (or also). > > The BeginMainFrame can be throttled because of back pressure, which will make > latency look worse than it otherwise would. > > BeginImplFrame always runs as soon as we get the BeginFrame message, so its > latency will better reflect OS scheduling badness. Thanks, I'll do an additional change to implement this suggestion. So this is called before WillBeginMainFrame, right? Should I capture Now() here and use it in WillBeginMainFrame to report the latency? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
