Track input latency for both begin and end of GPU swap in UMAs

Track input latency for both begin and end of GPU swap in UMAs

crrev.com/318812 switched our latency metrics from using
the end of GPU swap to the beginning. The abrupt switch
makes it difficult to compare latency in M43 to previous
versions.

This patch restores the old behavior for the existing
latency metrics and adds new metrics instead for the
new behavior.

Only UMA is affected. The abrupt switch for telemetry
remains in place.

BUG=476213
Committed: https://crrev.com/b2e3b981d8a02ddaed909aac7941281f93f47f27
Cr-Commit-Position: refs/heads/master@{#326931}

[brianderson, 2015-04-17 21:21:51]

brianderson@chromium.org changed reviewers:
+ jdduke@chromium.org, mpearson@chromium.org, tdresser@chromium.org

[brianderson, 2015-04-17 21:21:52]

ptal

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:435:
gpu_swap_end_component.event_time - tab_switch_component.event_time;
jbauman: Heads up that this will change MPArch.RWH_TabSwitchPaintDuration to use
the end of swap, which is what it was using before.

[brianderson, 2015-04-17 21:26:11]

+mpearson for histogram.xml changes
+jbauman for heads up on MPArch.RWH_TabSwitchPaintDuration

[jdduke (slow), 2015-04-17 21:26:40]

jdduke@chromium.org changed reviewers:
+ miletus@chromium.org

[jdduke (slow), 2015-04-17 21:26:40]

+miletus

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:455:
gpu_swap_begin_component.event_time - browser_swap_component.event_time;
Hmm, I'm wondering if it makes sense to use the "end" component here.

[Yufeng Shen (Slow to review), 2015-04-17 21:49:58]

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:171: 1,
1000000, 100);
a UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY macro for this similar to 
UMA_HISTOGRAM_SCROLL_LATENCY_LONG/SHORT above ?

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:176:
UMA_HISTOGRAM_CUSTOM_COUNTS(
you can do this in the same loop above.

[brianderson, 2015-04-17 21:51:29]

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:455:
gpu_swap_begin_component.event_time - browser_swap_component.event_time;
On 2015/04/17 21:26:40, jdduke wrote:
> Hmm, I'm wondering if it makes sense to use the "end" component here.

For this one I'd prefer to keep the begin component since the end will have more
variance in the blocking behavior depending on the platform and GPU state.

If we need to be more conservative in the deadline estimation, I would prefer to
play with the percentile + fudge factor of the
browser_composite_latency_hitory_.

If the swap starts to block longer (pushing out the end component), it likely
means the Browser is in a high latency mode. Using the end component in that
case will increase the estimated Browser composite costs and make the Renderer's
deadline earlier, which will likely put the Renderer in a high latency mode too.

We really need OS feedback to avoid heuristics like this...

[brianderson, 2015-04-17 22:01:41]

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:171: 1,
1000000, 100);
On 2015/04/17 21:49:58, Yufeng Shen wrote:
> a UMA_HISTOGRAM_TOUCH_TO_SCROLL_LATENCY macro for this similar to 
> UMA_HISTOGRAM_SCROLL_LATENCY_LONG/SHORT above ?

Done.

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:176:
UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2015/04/17 21:49:58, Yufeng Shen wrote:
> you can do this in the same loop above.

Done.

[jdduke (slow), 2015-04-17 22:03:22]

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:455:
gpu_swap_begin_component.event_time - browser_swap_component.event_time;
On 2015/04/17 21:51:29, brianderson wrote:
> On 2015/04/17 21:26:40, jdduke wrote:
> > Hmm, I'm wondering if it makes sense to use the "end" component here.
> 
> For this one I'd prefer to keep the begin component since the end will have
more
> variance in the blocking behavior depending on the platform and GPU state.
> 
> If we need to be more conservative in the deadline estimation, I would prefer
to
> play with the percentile + fudge factor of the
> browser_composite_latency_hitory_.
> 
> If the swap starts to block longer (pushing out the end component), it likely
> means the Browser is in a high latency mode. Using the end component in that
> case will increase the estimated Browser composite costs and make the
Renderer's
> deadline earlier, which will likely put the Renderer in a high latency mode
too.
> 
> We really need OS feedback to avoid heuristics like this...

I see. By "renderer" do you mean Blink? Or the renderer compositor? I was under
the impression that a shorter deadline would make it less likely for the
compositor to enter high latency mode (trading off Blink latency), which is the
critical path for most gestures.

[Yufeng Shen (Slow to review), 2015-04-17 22:06:22]

https://codereview.chromium.org/1093923002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:142: 100)
please use (end - start) to make it less confusing.

[brianderson, 2015-04-17 22:20:04]

On 2015/04/17 22:03:22, jdduke wrote:
>
https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_latency_tracker.cc
> (right):
> 
>
https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_latency_tracker.cc:455:
> gpu_swap_begin_component.event_time - browser_swap_component.event_time;
> On 2015/04/17 21:51:29, brianderson wrote:
> > On 2015/04/17 21:26:40, jdduke wrote:
> > > Hmm, I'm wondering if it makes sense to use the "end" component here.
> > 
> > For this one I'd prefer to keep the begin component since the end will have
> more
> > variance in the blocking behavior depending on the platform and GPU state.
> > 
> > If we need to be more conservative in the deadline estimation, I would
prefer
> to
> > play with the percentile + fudge factor of the
> > browser_composite_latency_hitory_.
> > 
> > If the swap starts to block longer (pushing out the end component), it
likely
> > means the Browser is in a high latency mode. Using the end component in that
> > case will increase the estimated Browser composite costs and make the
> Renderer's
> > deadline earlier, which will likely put the Renderer in a high latency mode
> too.
> > 
> > We really need OS feedback to avoid heuristics like this...
> 
> I see. By "renderer" do you mean Blink? Or the renderer compositor? I was
under
> the impression that a shorter deadline would make it less likely for the
> compositor to enter high latency mode (trading off Blink latency), which is
the
> critical path for most gestures.

You are correct - I was thinking of Blink. An earlier deadline would improve
compositor thread latency at the cost of main thread latency.

However, if there isn't a scroll listener on the main thread, the compositor
currently triggers the deadline early anyway and this calculation doesn't
matter.

Let's follow up with discussion of this in crbug.com/469938 since whatever we
decide will likely go into a separate patch.

[jdduke (slow), 2015-04-17 22:20:57]

On 2015/04/17 22:20:04, brianderson wrote:
> On 2015/04/17 22:03:22, jdduke wrote:
> >
>
https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
> > File content/browser/renderer_host/render_widget_host_latency_tracker.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1093923002/diff/1/content/browser/renderer_ho...
> > content/browser/renderer_host/render_widget_host_latency_tracker.cc:455:
> > gpu_swap_begin_component.event_time - browser_swap_component.event_time;
> > On 2015/04/17 21:51:29, brianderson wrote:
> > > On 2015/04/17 21:26:40, jdduke wrote:
> > > > Hmm, I'm wondering if it makes sense to use the "end" component here.
> > > 
> > > For this one I'd prefer to keep the begin component since the end will
have
> > more
> > > variance in the blocking behavior depending on the platform and GPU state.
> > > 
> > > If we need to be more conservative in the deadline estimation, I would
> prefer
> > to
> > > play with the percentile + fudge factor of the
> > > browser_composite_latency_hitory_.
> > > 
> > > If the swap starts to block longer (pushing out the end component), it
> likely
> > > means the Browser is in a high latency mode. Using the end component in
that
> > > case will increase the estimated Browser composite costs and make the
> > Renderer's
> > > deadline earlier, which will likely put the Renderer in a high latency
mode
> > too.
> > > 
> > > We really need OS feedback to avoid heuristics like this...
> > 
> > I see. By "renderer" do you mean Blink? Or the renderer compositor? I was
> under
> > the impression that a shorter deadline would make it less likely for the
> > compositor to enter high latency mode (trading off Blink latency), which is
> the
> > critical path for most gestures.
> 
> You are correct - I was thinking of Blink. An earlier deadline would improve
> compositor thread latency at the cost of main thread latency.
> 
> However, if there isn't a scroll listener on the main thread, the compositor
> currently triggers the deadline early anyway and this calculation doesn't
> matter.
> 
> Let's follow up with discussion of this in crbug.com/469938 since whatever we
> decide will likely go into a separate patch.

Sounds good!

[brianderson, 2015-04-17 22:21:25]

https://codereview.chromium.org/1093923002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:142: 100)
On 2015/04/17 22:06:22, Yufeng Shen wrote:
> please use (end - start) to make it less confusing.

D'oh. Done.

[Yufeng Shen (Slow to review), 2015-04-17 22:28:42]

https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:175:
gpu_swap_begin_component, first_original_component);
the start/end order is reversed and ditto to all the calling sites below.

[brianderson, 2015-04-17 22:52:50]

https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:175:
gpu_swap_begin_component, first_original_component);
On 2015/04/17 22:28:42, Yufeng Shen wrote:
> the start/end order is reversed and ditto to all the calling sites below.

Done.

[Yufeng Shen (Slow to review), 2015-04-18 00:22:42]

On 2015/04/17 22:52:50, brianderson wrote:
>
https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
> File content/browser/renderer_host/render_widget_host_latency_tracker.cc
> (right):
> 
>
https://codereview.chromium.org/1093923002/diff/60001/content/browser/rendere...
> content/browser/renderer_host/render_widget_host_latency_tracker.cc:175:
> gpu_swap_begin_component, first_original_component);
> On 2015/04/17 22:28:42, Yufeng Shen wrote:
> > the start/end order is reversed and ditto to all the calling sites below.
> 
> Done.

lgtm

[brianderson, 2015-04-18 00:31:42]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-04-18 00:31:51]

The CQ bit was unchecked by brianderson@chromium.org

[tdresser, 2015-04-20 12:50:38]

LGTM with nit.

https://codereview.chromium.org/1093923002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:177: // data
with the metric above.
We're going to land this for M43, and one milestone of overlap should be
adequate. Can you file a bug to remove these in M44, and reference the bug in
these comments?

[brianderson, 2015-04-20 19:15:17]

brianderson@chromium.org changed reviewers:
+ sadrul@chromium.org

[brianderson, 2015-04-20 19:15:17]

+sadrul for owners review or content/browser/renderer_host.

mpearson: Can you also take a look at histograms.xml?

https://codereview.chromium.org/1093923002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_latency_tracker.cc
(right):

https://codereview.chromium.org/1093923002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_latency_tracker.cc:177: // data
with the metric above.
On 2015/04/20 12:50:38, tdresser wrote:
> We're going to land this for M43, and one milestone of overlap should be
> adequate. Can you file a bug to remove these in M44, and reference the bug in
> these comments?

Done.

[Mark P, 2015-04-20 23:12:53]

histograms.xml lgtm
(sorry for the delay; usually I'm faster at doing my first pass)

--mark

[sadrul, 2015-04-20 23:15:49]

rs lgtm

[brianderson, 2015-04-24 02:56:03]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-04-24 02:56:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
miletus@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1093923002/#ps120001
(title: "Add reference to bug to remove old metric")

[commit-bot: I haz the power, 2015-04-24 02:56:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093923002/120001

[commit-bot: I haz the power, 2015-04-24 02:59:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 02:59:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[brianderson, 2015-04-24 22:15:23]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-04-24 22:15:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org, miletus@chromium.org, sadrul@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1093923002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-24 22:16:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1093923002/140001

[commit-bot: I haz the power, 2015-04-24 23:29:04]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-24 23:30:01]

Patchset 8 (id:??) landed as
https://crrev.com/b2e3b981d8a02ddaed909aac7941281f93f47f27
Cr-Commit-Position: refs/heads/master@{#326931}