Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2268163002:
Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin (Closed)
|
Created:
4 years, 4 months ago by lanwei Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, asvitkine+watch_chromium.org, dtapuska+chromiumwatch_chromium.org, tdresser+watch_chromium.org, dproy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin.
We defined a set of sub-metrics to measure the latencies inside
Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand
the time spent on each stage during scroll. Now we will add the same set
of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin.
The details are in this doc:
https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3lnpQ/edit#heading=h.r6af7ldx7kcw.
BUG=616482
Committed: https://crrev.com/eb43b09f06395377e66c684522d76ec002563fb0
Cr-Commit-Position: refs/heads/master@{#434682}
Patch Set 1 #
Total comments: 4
Patch Set 2 : git cl try #
Total comments: 9
Patch Set 3 : submetric #Patch Set 4 : submetric #Patch Set 5 : Create a new macro without cache #Patch Set 6 : Add flag in the macro #
Total comments: 2
Patch Set 7 : submetric #Patch Set 8 : submetric #
Total comments: 1
Patch Set 9 : submetric #
Total comments: 3
Messages
Total messages: 100 (75 generated)
Description was changed from ========== submetric submetric tsub metric submetric ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
The CQ bit was checked by lanwei@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.
Can you update https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln... with the new image generated from that dot file? https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:132: // are present and their event times are legit. miletus doesn't work here anymore. I assume this is just copied? We can probably just get rid of this TODO. https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:142: goto allScrollUpdate; Rewrite without using goto. It's barely mentioned in the style guide, but it's called out as "vanishingly rare", which is enough to not want to use it. You could refactor these into a method that returns when a component isn't found?
The CQ bit was checked by lanwei@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.
I will update the graphic if you think the metric names are ok, should I also change the owner to you? https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:132: // are present and their event times are legit. On 2016/08/23 13:57:00, tdresser wrote: > miletus doesn't work here anymore. > I assume this is just copied? > > We can probably just get rid of this TODO. Done. https://codereview.chromium.org/2268163002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:142: goto allScrollUpdate; On 2016/08/23 13:57:00, tdresser wrote: > Rewrite without using goto. > > It's barely mentioned in the style guide, but it's called out as "vanishingly > rare", which is enough to not want to use it. > > You could refactor these into a method that returns when a component isn't > found? Done.
+dproy as FYI Metric names look great. Can you change the owner for these metrics and the existing ScrollUpdate metrics to me? Thanks! https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:196: ComputeFirstScrollLatencyHistograms( Could we start by checking if this is the first scroll or not, and then have a function that records FirstScrollUpdate or ScrollUpdate metrics depending on a parameter? https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:231: &rendering_scheduled_component)) { Are these changes needed? https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:75: void HistogramTotalCountEq(const std::string& name, int count) { Since this contains an expectation, it should have Expect in the name.
https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:196: ComputeFirstScrollLatencyHistograms( On 2016/08/24 12:48:57, tdresser wrote: > Could we start by checking if this is the first scroll or not, and then have a > function that records FirstScrollUpdate or ScrollUpdate metrics depending on a > parameter? We'll end up having to look up some components more than once if we take this approach, but I think that's acceptable. In the long term, we won't have a metrics that records both scroll start and continuous scroll, only one or the other, which will allow us to stop recomputing these twice.
The CQ bit was checked by lanwei@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 lanwei@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...
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:196: ComputeFirstScrollLatencyHistograms( On 2016/08/24 12:48:57, tdresser wrote: > Could we start by checking if this is the first scroll or not, and then have a > function that records FirstScrollUpdate or ScrollUpdate metrics depending on a > parameter? I tried several ways, but none of them works, because the metric function is defined as macro. Using a variable as the metric name will mess up later, and make Checkname fail, https://codesearch.chromium.org/chromium/src/base/metrics/histogram_base.cc?d.... Is there any way to make this work? https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:231: &rendering_scheduled_component)) { On 2016/08/24 12:48:57, tdresser wrote: > Are these changes needed? I remembered you told me that if the statement has multi-line, then use {}, am I wrong? https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker_unittest.cc:75: void HistogramTotalCountEq(const std::string& name, int count) { On 2016/08/24 12:48:57, tdresser wrote: > Since this contains an expectation, it should have Expect in the name. Done.
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_...)
tdresser@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ to comment on instantiating multiple similar histograms without massive code duplication. asvitkine@ is OOO until Monday, as is rkaplow@... https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:196: ComputeFirstScrollLatencyHistograms( On 2016/08/25 04:17:11, lanwei wrote: > On 2016/08/24 12:48:57, tdresser wrote: > > Could we start by checking if this is the first scroll or not, and then have a > > function that records FirstScrollUpdate or ScrollUpdate metrics depending on a > > parameter? > > I tried several ways, but none of them works, because the metric function is > defined as macro. Using a variable as the metric name will mess up later, and > make Checkname fail, > https://codesearch.chromium.org/chromium/src/base/metrics/histogram_base.cc?d.... > > Is there any way to make this work? Hmmm, we want something like: https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=66 I'm not sure if we should move that method to somewhere more generic, or whether there's a preferable approach. asvitkine would know. https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:231: &rendering_scheduled_component)) { On 2016/08/25 04:17:11, lanwei wrote: > On 2016/08/24 12:48:57, tdresser wrote: > > Are these changes needed? > > I remembered you told me that if the statement has multi-line, then use {}, am I > wrong? Er, nope, I prefer this, sorry.
On 2016/08/25 13:31:05, tdresser wrote: > +asvitkine@ to comment on instantiating multiple similar histograms without > massive code duplication. > > asvitkine@ is OOO until Monday, as is rkaplow@... > > https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:196: > ComputeFirstScrollLatencyHistograms( > On 2016/08/25 04:17:11, lanwei wrote: > > On 2016/08/24 12:48:57, tdresser wrote: > > > Could we start by checking if this is the first scroll or not, and then have > a > > > function that records FirstScrollUpdate or ScrollUpdate metrics depending on > a > > > parameter? > > > > I tried several ways, but none of them works, because the metric function is > > defined as macro. Using a variable as the metric name will mess up later, and > > make Checkname fail, > > > https://codesearch.chromium.org/chromium/src/base/metrics/histogram_base.cc?d.... > > > > Is there any way to make this work? > > Hmmm, we want something like: > > https://cs.chromium.org/chromium/src/ui/events/event.cc?rcl=0&l=66 > > I'm not sure if we should move that method to somewhere more generic, or whether > there's a preferable approach. asvitkine would know. So you can move that macro to base/metrics/histogram_macros.h and use it or you can use the FactoryGet API on histograms to do what the macro does. If you use FactoryGet(), you can pass in a variable name. > > https://codereview.chromium.org/2268163002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:231: > &rendering_scheduled_component)) { > On 2016/08/25 04:17:11, lanwei wrote: > > On 2016/08/24 12:48:57, tdresser wrote: > > > Are these changes needed? > > > > I remembered you told me that if the statement has multi-line, then use {}, am > I > > wrong? > > Er, nope, I prefer this, sorry.
The CQ bit was checked by lanwei@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 lanwei@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...
Patchset #4 (id:80001) has been deleted
Patchset #5 (id:120001) has been deleted
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 lanwei@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 lanwei@chromium.org to run a CQ dry run
Patchset #6 (id:160001) has been deleted
Patchset #4 (id:100001) has been deleted
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lanwei@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 lanwei@chromium.org to run a CQ dry run
Patchset #5 (id:180001) has been deleted
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 patch 5 and 6 both work, but different approaches. Could you please take a look, and suggest me which one is better, thanks you?
I think the approach which doesn't need to find all the latency info components twice is better.
patchset 6 lgtm
lanwei@chromium.org changed reviewers: + isherman@chromium.org
Could you please take a look at patch 6, thanks?
lanwei@chromium.org changed reviewers: - isherman@chromium.org
Can you update the diagram in the doc (https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln... https://codereview.chromium.org/2268163002/diff/220001/content/browser/render... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2268163002/diff/220001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:78: base::StringPrintf(name, ""), \ Do we need this StringPrintf? Couldn't we just pass name? https://codereview.chromium.org/2268163002/diff/220001/content/browser/render... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:186: "Event.Latency.%sScrollUpdate.HandledToRendererSwap_Main", Passing these format strings around is a bit scary. Add a comment to the histogram macros that they take a printf style format string. Perhaps rename the "name" parameter of those macros as well, something like "name_format_string".
The CQ bit was checked by lanwei@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 lanwei@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 lanwei@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...
Patchset #8 (id:260001) has been deleted
Patchset #7 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+sahel@ for thoughts on histogram naming. Unfortunately, the histogram names here are out of date now... We want them to be consistent with the new names. Take a look at which histograms are obsoleted, and what the new names are. Something like: Event.Latency.ScrollUpdate.Touch.FOO I'm not sure what the best naming scheme is for dealing with the scroll start metrics. We currently have: Event.Latency.ScrollUpdate.Wheel.TimeToFirstScrollUpdateSwapBegin2 I can see a reasonable argument that we should rename this to: Event.Latency.ScrollBegin.Wheel.TimeToScrollUpdateSwapBegin2 And then move all the metrics you're adding into Event.Latency.ScrollBegin What do you think?
On 2016/11/23 16:27:42, tdresser wrote: > +sahel@ for thoughts on histogram naming. > > Unfortunately, the histogram names here are out of date now... > > We want them to be consistent with the new names. Take a look at which > histograms are obsoleted, and what the new names are. > > Something like: > Event.Latency.ScrollUpdate.Touch.FOO > > I'm not sure what the best naming scheme is for dealing with the scroll start > metrics. > > We currently have: > Event.Latency.ScrollUpdate.Wheel.TimeToFirstScrollUpdateSwapBegin2 > > I can see a reasonable argument that we should rename this to: > Event.Latency.ScrollBegin.Wheel.TimeToScrollUpdateSwapBegin2 > > And then move all the metrics you're adding into Event.Latency.ScrollBegin > > What do you think? Event.Latency.ScrollUpdate.Touch/Wheel.Foo is the new format of the histograms. I think using ScrollBegin instead of FirstScrollUpdate makes sense.
The CQ bit was checked by lanwei@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 lanwei@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...
Patchset #8 (id:300001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Awesome - I just want to take a look at the diagram, but the code looks good. https://codereview.chromium.org/2268163002/diff/320001/ui/events/latency_info... File ui/events/latency_info.dot (right): https://codereview.chromium.org/2268163002/diff/320001/ui/events/latency_info... ui/events/latency_info.dot:35: INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT -> INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_MAIN_COMPONENT [label="Event.Latency.ScrollBegin.*.TimeToHandled2_Main"]; Seems like the usage of * is inconsistent, can we make this consistent? And update the diagram in the doc? https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln...)
The CQ bit was checked by lanwei@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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Tim, I use * for ScrollBegin/ScrollUpdate and Touch/Wheel, but now it is just for Touch/Wheel, what do you think? I will update the graph once we decide on the names. Thanks.
https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... File ui/events/latency_info.dot (right): https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... ui/events/latency_info.dot:33: INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT -> INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT [label="Event.Latency.ScrollUpdate.*.TimeToHandled2_Impl"]; This is still inconsistent, right? We're using "ScrollBegin/ScrollUpdate" for some histograms, but not all?
https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... File ui/events/latency_info.dot (right): https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... ui/events/latency_info.dot:33: INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT -> INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT [label="Event.Latency.ScrollUpdate.*.TimeToHandled2_Impl"]; On 2016/11/24 19:16:16, tdresser wrote: > This is still inconsistent, right? We're using "ScrollBegin/ScrollUpdate" for > some histograms, but not all? Yes, because the histograms of TimeToScrollUpdateSwapBegin2 and TimeToHandled2 are measured from INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT or INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT, so we will use have separate ones for both ScrollBegin and ScrollUpdate. But the other histograms will presents them in just one. https://drive.google.com/a/google.com/file/d/0B6aINcG6kDLXcmR1NTBnZFVnM2M/vie...
Thanks! LGTM https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... File ui/events/latency_info.dot (right): https://codereview.chromium.org/2268163002/diff/340001/ui/events/latency_info... ui/events/latency_info.dot:33: INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT -> INPUT_EVENT_LATENCY_RENDERING_SCHEDULED_IMPL_COMPONENT [label="Event.Latency.ScrollUpdate.*.TimeToHandled2_Impl"]; On 2016/11/24 19:29:11, lanwei wrote: > On 2016/11/24 19:16:16, tdresser wrote: > > This is still inconsistent, right? We're using "ScrollBegin/ScrollUpdate" for > > some histograms, but not all? > > Yes, because the histograms of TimeToScrollUpdateSwapBegin2 and TimeToHandled2 > are measured from > INPUT_EVENT_LATENCY_FIRST_SCROLL_UPDATE_ORIGINAL_COMPONENT or > INPUT_EVENT_LATENCY_SCROLL_UPDATE_ORIGINAL_COMPONENT, so we will use have > separate ones for both ScrollBegin and ScrollUpdate. But the other histograms > will presents them in just one. > > https://drive.google.com/a/google.com/file/d/0B6aINcG6kDLXcmR1NTBnZFVnM2M/vie... Acknowledged.
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2268163002/#ps340001 (title: "submetric")
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": 340001, "attempt_start_ts": 1480352345822440,
"parent_rev": "f3690baa1f09dc9b10b2107ef26acafde7c3e40b", "commit_rev":
"f3c244462d430c397c437ca36343ed0575c03788"}
Message was sent while issue was closed.
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 ========== to ========== Add sub-metrics for Event.Latency.TouchToFirstScrollUpdateSwapBegin. We defined a set of sub-metrics to measure the latencies inside Event.Latency.TouchToScrollUpdateSwapBegin to help us better understand the time spent on each stage during scroll. Now we will add the same set of sub-metrics to Event.Latency.TouchToFirstScrollUpdateSwapBegin. The details are in this doc: https://docs.google.com/document/d/1EPKT652e2PcO1ysA7LQosGyyr1xmxbtiiiivNU3ln.... BUG=616482 Committed: https://crrev.com/eb43b09f06395377e66c684522d76ec002563fb0 Cr-Commit-Position: refs/heads/master@{#434682} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/eb43b09f06395377e66c684522d76ec002563fb0 Cr-Commit-Position: refs/heads/master@{#434682} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
