Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2341413002:
New scroll latency metrics added for touch and wheel. (Closed)
DescriptionNew scroll latency metrics added for touch and wheel.
The new latency metrics are calculated based on the difference between
the first and last event time of the start and end componenets,
respectively. They deprecate the old metrics that use the average event
time of both components, and report the latency only for touch scrolls.
BUG=622827
TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms,
RenderWidgetHostLatencyTrackerTest.WheelTestHistograms
Committed: https://crrev.com/e05ae5d2865b5c1095b552a39f4dbafa14a82e0d
Cr-Commit-Position: refs/heads/master@{#423944}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added comments for deprecation #
Total comments: 6
Patch Set 3 : use of getFactory to recompute the histogram names. #
Total comments: 1
Patch Set 4 : Use thread_name to compute _MAin, _Impl only once. #
Messages
Total messages: 40 (24 generated)
The CQ bit was checked by sahel@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.
sahel@chromium.org changed reviewers: + tdresser@chromium.org
I didn't deprecate anything, yet. The histogram names are just rough names, for now.
+Deep and Lan to comment on names. I think these are probably reasonable, though we might want to slap a "2" on the end to make it clear that these are the successors of TouchToScrollUpdateSwapBegin and friends. https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", We want to exclude the first GSU from this metric. https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: // metrics. We should update this comment, as we are now reporting frame latency on wheel events, just not here. https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: ComputeScrollLatencyHistograms( Might be worth mentioning in a comment here that we're phasing this out.
On 2016/09/23 14:28:39, tdresser (OOO Sept 26-28) wrote: > +Deep and Lan to comment on names. > Spilt the 'touch'/'wheel' from the component phrases, sometime breaks the meaning. e.g. Event.Latency.ScrollUpdate.Touch.BrowserNotifiedToBeforeGpuSwap is fine. But Event.Latency.Touch.ToFirstScrollUpdateSwapBegin is not, TouchToFirstScrollUpdateSwapBegin is better. > I think these are probably reasonable, though we might want to slap a "2" on the > end to make it clear that these are the successors of > TouchToScrollUpdateSwapBegin and friends. > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: > "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", > We want to exclude the first GSU from this metric. > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: > // metrics. > We should update this comment, as we are now reporting frame latency on wheel > events, just not here. > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: > ComputeScrollLatencyHistograms( > Might be worth mentioning in a comment here that we're phasing this out.
On 2016/09/23 15:29:46, lanwei wrote: > On 2016/09/23 14:28:39, tdresser (OOO Sept 26-28) wrote: > > +Deep and Lan to comment on names. > > > Spilt the 'touch'/'wheel' from the component phrases, sometime breaks the > meaning. > e.g. Event.Latency.ScrollUpdate.Touch.BrowserNotifiedToBeforeGpuSwap is fine. > > But Event.Latency.Touch.ToFirstScrollUpdateSwapBegin is not, > TouchToFirstScrollUpdateSwapBegin is better. > > > > I think these are probably reasonable, though we might want to slap a "2" on > the > > end to make it clear that these are the successors of > > TouchToScrollUpdateSwapBegin and friends. > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > (right): > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: > > "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", > > We want to exclude the first GSU from this metric. > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: > > // metrics. > > We should update this comment, as we are now reporting frame latency on wheel > > events, just not here. > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: > > ComputeScrollLatencyHistograms( > > Might be worth mentioning in a comment here that we're phasing this out. Lan, just clarifying: You'd recommend remove the "." between the input modality and the metric name, for all of those metrics? I think I prefer the ".", because it makes it easy to view only the Touch or only the Wheel metrics using the timeline UI.
On 2016/09/23 15:47:37, tdresser (OOO Sept 26-28) wrote: > On 2016/09/23 15:29:46, lanwei wrote: > > On 2016/09/23 14:28:39, tdresser (OOO Sept 26-28) wrote: > > > +Deep and Lan to comment on names. > > > > > Spilt the 'touch'/'wheel' from the component phrases, sometime breaks the > > meaning. > > e.g. Event.Latency.ScrollUpdate.Touch.BrowserNotifiedToBeforeGpuSwap is fine. > > > > But Event.Latency.Touch.ToFirstScrollUpdateSwapBegin is not, > > TouchToFirstScrollUpdateSwapBegin is better. > > > > > > > I think these are probably reasonable, though we might want to slap a "2" on > > the > > > end to make it clear that these are the successors of > > > TouchToScrollUpdateSwapBegin and friends. > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > File > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: > > > "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", > > > We want to exclude the first GSU from this metric. > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: > > > // metrics. > > > We should update this comment, as we are now reporting frame latency on > wheel > > > events, just not here. > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: > > > ComputeScrollLatencyHistograms( > > > Might be worth mentioning in a comment here that we're phasing this out. > > Lan, just clarifying: > > You'd recommend remove the "." between the input modality and the metric name, > for all of those metrics? > > I think I prefer the ".", because it makes it easy to view only the Touch or > only the Wheel metrics using the timeline UI. I agree with Lan, that The second part that starts with To is not great. But, how about changing it to start with TimeTo?
On 2016/09/23 16:08:57, sahel wrote: > On 2016/09/23 15:47:37, tdresser (OOO Sept 26-28) wrote: > > On 2016/09/23 15:29:46, lanwei wrote: > > > On 2016/09/23 14:28:39, tdresser (OOO Sept 26-28) wrote: > > > > +Deep and Lan to comment on names. > > > > > > > Spilt the 'touch'/'wheel' from the component phrases, sometime breaks the > > > meaning. > > > e.g. Event.Latency.ScrollUpdate.Touch.BrowserNotifiedToBeforeGpuSwap is > fine. > > > > > > But Event.Latency.Touch.ToFirstScrollUpdateSwapBegin is not, > > > TouchToFirstScrollUpdateSwapBegin is better. > > > > > > > > > > I think these are probably reasonable, though we might want to slap a "2" > on > > > the > > > > end to make it clear that these are the successors of > > > > TouchToScrollUpdateSwapBegin and friends. > > > > > > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > File > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: > > > > "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", > > > > We want to exclude the first GSU from this metric. > > > > > > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: > > > > // metrics. > > > > We should update this comment, as we are now reporting frame latency on > > wheel > > > > events, just not here. > > > > > > > > > > > > > > https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... > > > > > > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: > > > > ComputeScrollLatencyHistograms( > > > > Might be worth mentioning in a comment here that we're phasing this out. > > > > Lan, just clarifying: > > > > You'd recommend remove the "." between the input modality and the metric name, > > for all of those metrics? > > > > I think I prefer the ".", because it makes it easy to view only the Touch or > > only the Wheel metrics using the timeline UI. > > I agree with Lan, that The second part that starts with To is not great. But, > how about changing it to start with TimeTo? I like starting with TimeTo. That SGTM.
sahel@chromium.org changed reviewers: + mkwst@chromium.org
mkwst@chromium.org: Please review changes in ipc holte@chromium.org: Please review changes in Histograms.xml https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:270: "Event.Latency." + event_type_name + ".ToScrollUpdateSwapBegin", On 2016/09/23 14:28:38, tdresser (OOO Sept 26-28) wrote: > We want to exclude the first GSU from this metric. Done. https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:708: // metrics. On 2016/09/23 14:28:38, tdresser (OOO Sept 26-28) wrote: > We should update this comment, as we are now reporting frame latency on wheel > events, just not here. Done. https://codereview.chromium.org/2341413002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:713: ComputeScrollLatencyHistograms( On 2016/09/23 14:28:38, tdresser (OOO Sept 26-28) wrote: > Might be worth mentioning in a comment here that we're phasing this out. Done.
The CQ bit was checked by sahel@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 don't remember if I've already published, or not. Since, I haven't got any feedback, I assume that I haven't.
Description was changed from ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms ========== to ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms ==========
sahel@chromium.org changed reviewers: + holte@chromium.org, nasko@chromium.org - mkwst@chromium.org
LGTM, thanks! https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:265: // created to when the scroll gesture results in final frame swap. Clarify that this excludes the first scroll. https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:720: } Let's move the deprecated metrics after the more current metrics. I think that'll just make the scope of the above comment a bit clearer.
IPC LGTM
https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:306: "Event.Latency.ScrollUpdate." + event_type_name + Computing names like this doesn't work when using UMA_HISTOGRAM_CUSTOM_COUNTS macro. This macro uses a static variable to keep a pointer to the Histogram object, which won't get recomputed when you hit this with different names. You need to either have a separate macro invocation for each name you are using, or use FactoryGet(args)->Add(sample) directly.
The CQ bit was checked by sahel@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.
https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:265: // created to when the scroll gesture results in final frame swap. On 2016/09/30 17:16:38, tdresser wrote: > Clarify that this excludes the first scroll. Done. https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:306: "Event.Latency.ScrollUpdate." + event_type_name + On 2016/09/30 22:10:05, Steven Holte wrote: > Computing names like this doesn't work when using UMA_HISTOGRAM_CUSTOM_COUNTS > macro. > This macro uses a static variable to keep a pointer to the Histogram object, > which won't get recomputed when you hit this with different names. > > You need to either have a separate macro invocation for each name you are using, > or use FactoryGet(args)->Add(sample) directly. Done. https://codereview.chromium.org/2341413002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:720: } On 2016/09/30 17:16:38, tdresser wrote: > Let's move the deprecated metrics after the more current metrics. > > I think that'll just make the scope of the above comment a bit clearer. Done.
histograms lgtm https://codereview.chromium.org/2341413002/diff/40001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2341413002/diff/40001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:291: ".TimeToHandled2_Main", Optional: since you are computing these names anyway, you could make _Main / _Impl part into a variable, since you use it a couple times in this function.
The CQ bit was checked by sahel@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 sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, nasko@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2341413002/#ps60001 (title: "Use thread_name to compute _MAin, _Impl only once.")
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.
Description was changed from ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms ========== to ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms ========== to ========== New scroll latency metrics added for touch and wheel. The new latency metrics are calculated based on the difference between the first and last event time of the start and end componenets, respectively. They deprecate the old metrics that use the average event time of both components, and report the latency only for touch scrolls. BUG=622827 TEST=RenderWidgetHostLatencyTrackerTest.TouchTestHistograms, RenderWidgetHostLatencyTrackerTest.WheelTestHistograms Committed: https://crrev.com/e05ae5d2865b5c1095b552a39f4dbafa14a82e0d Cr-Commit-Position: refs/heads/master@{#423944} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e05ae5d2865b5c1095b552a39f4dbafa14a82e0d Cr-Commit-Position: refs/heads/master@{#423944} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
