Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2258253002:
cc: Add a Scheduling.SwapAckWasFast UMA. (Closed)
|
Created:
4 years, 4 months ago by brianderson Modified:
4 years, 2 months ago CC:
aelias_OOO_until_Jul13, asvitkine+watch_chromium.org, cc-bugs_chromium.org, chromium-reviews, piman, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Add a Scheduling.SwapAckWasFast UMA.
Records false if we haven't received a swap ack for more
than 8 seconds. Otherwise, records true.
BUG=602486
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/b97331bed2cb89b0758ab0488a7e6b8eefd1acec
Cr-Commit-Position: refs/heads/master@{#422930}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix histograms.xml #
Total comments: 4
Patch Set 3 : always call Now() #
Total comments: 3
Patch Set 4 : sunny's review #Patch Set 5 : rebase #
Messages
Total messages: 26 (17 generated)
Description was changed from ========== cc: Add a Scheduling.SwapAckWasFast UMA. Records false if we haven't received a swap ack for more than 8 seconds. Otherwise, records true. BUG=602486 ========== to ========== cc: Add a Scheduling.SwapAckWasFast UMA. Records false if we haven't received a swap ack for more than 8 seconds. Otherwise, records true. BUG=602486 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by brianderson@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...
brianderson@chromium.org changed reviewers: + isherman@chromium.org, sunnyps@chromium.org
ptal! This replaces https://codereview.chromium.org/2156933003, which I've closed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Metrics lgtm % nits: https://codereview.chromium.org/2258253002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2258253002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:50673: +<histogram name="Scheduling.SwapAckWasFast" units="boolean"> Please provide a custom BooleanWasFast enum, rather than specifying a units attribute. (Only need to make changes in histograms.xml -- no C++ changes needed.) https://codereview.chromium.org/2258253002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:50677: + swap. It would be great to mention within this description why roughly 8 seconds is "fast".
https://codereview.chromium.org/2258253002/diff/1/cc/scheduler/compositor_tim... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/1/cc/scheduler/compositor_tim... cc/scheduler/compositor_timing_history.cc:63: // actual timestamps to avoid calling Now() too often or adding Is this optimization really needed? Even without the optimization wouldn't Now() only be called once per frame? https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:64: // unnecessary idle wakeups. I don't understand the point about idle wakeups. And I'm not sure the optimization to avoid calling Now is needed given that we would only call it once per frame. I think an important goal is to avoid excessive UMA reporting. So should we even report when swap ack is fast? I think we should only report once if swap ack is late, then reset the "swap ack is late" state only when we get the next swap ack. That would be once per user-perceived renderer freeze. WDYT? https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:283: UMA_HISTOGRAM_BOOLEAN("Scheduling.Browser.SwapAckWasFast", was_fast); nit: seems to be extra indentation here
https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:64: // unnecessary idle wakeups. On 2016/08/30 18:31:04, sunnyps wrote: > I don't understand the point about idle wakeups. And I'm not sure the > optimization to avoid calling Now is needed given that we would only call it > once per frame. > > I think an important goal is to avoid excessive UMA reporting. So should we even > report when swap ack is fast? > > I think we should only report once if swap ack is late, then reset the "swap ack > is late" state only when we get the next swap ack. That would be once per > user-perceived renderer freeze. WDYT? In general, it's better to build in a baseline for a metric than not to -- the baseline gives context to the number that you are primarily focused on. Incrementing a bucket in an UMA histogram is intentionally *very* cheap, so it shouldn't be terribly expensive to report fast swap acks. From the metric team's perspective, reducing the number of histograms that we report to UMA is generally speaking quite useful. Reducing the number of buckets in an UMA histogram by one is generally not very impactful.
ptal https://codereview.chromium.org/2258253002/diff/1/cc/scheduler/compositor_tim... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/1/cc/scheduler/compositor_tim... cc/scheduler/compositor_timing_history.cc:63: // actual timestamps to avoid calling Now() too often or adding On 2016/08/30 18:31:04, sunnyps wrote: > Is this optimization really needed? Even without the optimization wouldn't Now() > only be called once per frame? A few years ago, we were especially concerned about buggy AMD cpu's that needed special (slow) workarounds to ensure the performance timer is monotonic on Windows. Revisiting the code in time_win.cc, it looks like we just avoid using it now on that cpu, so I'll remove the optimization. https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/20001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:64: // unnecessary idle wakeups. On 2016/08/30 18:31:04, sunnyps wrote: > I don't understand the point about idle wakeups. And I'm not sure the > optimization to avoid calling Now is needed given that we would only call it > once per frame. > > I think an important goal is to avoid excessive UMA reporting. So should we even > report when swap ack is fast? > > I think we should only report once if swap ack is late, then reset the "swap ack > is late" state only when we get the next swap ack. That would be once per > user-perceived renderer freeze. WDYT? Regarding idle wakeups: I was referring to posting an 8s delayed task after every swap, but wasn't clear. I'll just avoid the reference.
lgtm % comments https://codereview.chromium.org/2258253002/diff/40001/cc/scheduler/compositor... File cc/scheduler/compositor_timing_history.cc (right): https://codereview.chromium.org/2258253002/diff/40001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:62: const base::TimeDelta kSwapAckWatchdogTimeout = base::TimeDelta is a non-POD type and those cannot be static according to the style guide: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Can you please make this constexpr instead? That should work because FromMicroseconds is constexpr. (Or you can use a static int64.) https://codereview.chromium.org/2258253002/diff/40001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:460: swap_ack_watchdog_enabled_ = false; nit: If we lose the output surface or if it's reinitialized we should get the DidSwapBuffersComplete callback (if there was a pending swap) so maybe this can be a DCHECK(!swap_ack_watchdog_enabled_)? https://codereview.chromium.org/2258253002/diff/40001/cc/scheduler/compositor... cc/scheduler/compositor_timing_history.cc:478: if (swap_not_acked_time_ > kSwapAckWatchdogTimeout) { nit: >= here because we use < in DidSwapBuffersComplete
The CQ bit was checked by brianderson@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: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by brianderson@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: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
The CQ bit was checked by brianderson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/2258253002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== cc: Add a Scheduling.SwapAckWasFast UMA. Records false if we haven't received a swap ack for more than 8 seconds. Otherwise, records true. BUG=602486 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== cc: Add a Scheduling.SwapAckWasFast UMA. Records false if we haven't received a swap ack for more than 8 seconds. Otherwise, records true. BUG=602486 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/b97331bed2cb89b0758ab0488a7e6b8eefd1acec Cr-Commit-Position: refs/heads/master@{#422930} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b97331bed2cb89b0758ab0488a7e6b8eefd1acec Cr-Commit-Position: refs/heads/master@{#422930} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
