Implement scroll handler latency tracking

Implement scroll handler latency tracking

Since scroll handlers do not block composited scrolling, the existing
input latency metric will only report the response time for the
compositor when scrolling a page with a handler. This is not accurate
for pages that implement scroll-synchronized effects, because the
scroll handler needs to be invoked so that the page contents are
updated with the new scroll offset.

This patch fixes the problem by forwarding latency information from the
impl to the main thread when we are scrolling a layer that has an active
scroll event handler.

In the case the scroll handler is no-op, the latency information will be
terminated as a failed commit.

Sample trace:
https://drive.google.com/file/d/0ByyxMXB38gLDRE40bkU5TVd0U00/edit?usp=sharing

BUG=347366
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289307

[Sami, 2014-07-01 15:44:51]

Hi Yufeng,

I hope this is along the lines of what we talked about during the convergence
meeting. PTAL.

[Yufeng Shen (Slow to review), 2014-07-04 18:34:34]

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1020: if (!latency_info.FindLatency(
why do we need this check ?

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1023:
ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_BLINK_COMPONENT, 0, 0);
Are we planning to use this new component anywhere ?
If we only care about end-to-end latency, then we don't need this new component.
Or do you plan to use this component to derive any new metrics ?

[Sami, 2014-07-07 14:28:18]

Thanks Yufeng. Another look please?

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1020: if (!latency_info.FindLatency(
Ah, you're right, the latency infos can only flow though here once so there's no
need for this check.

https://codereview.chromium.org/365463003/diff/1/cc/trees/layer_tree_host.cc#...
cc/trees/layer_tree_host.cc:1023:
ui::INPUT_EVENT_LATENCY_SCROLL_UPDATE_BLINK_COMPONENT, 0, 0);
Yeah, end-to-end is what really matters here. I wasn't planning to use the new
component for anything in particular, so let's just remove it.

[Yufeng Shen (Slow to review), 2014-07-07 19:35:04]

lgtm

[Sami, 2014-07-08 10:08:16]

Thanks. +danakj for cc and +jdduke for content/renderer/input, please.

[jdduke (slow), 2014-07-08 15:19:23]

On 2014/07/08 10:08:16, Sami wrote:
> Thanks. +danakj for cc and +jdduke for content/renderer/input, please.

content/renderer/input lgtm.

[danakj, 2014-07-08 16:49:08]

https://codereview.chromium.org/365463003/diff/20001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise_monitor.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise_monitor.cc:52:
layer_tree_host_impl_->QueueScrollUpdateLatencyInfo(*latency_);
Why doesn't this make a SwapPromise and queue that/pass that into the
ScrollAndScaleSet etc?

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1030: "Blink scroll update");
layering violation: cc doesn't know about blink. You can say main-thread scroll
update or something like that

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:146: std::vector<ui::LatencyInfo>
latency_infos;
this should be a ScopedPtrVector<SwapPromise>

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:505:
scroll_update_latency_infos_.push_back(latency_info);
Should we be dropping SwapPromises if the list gets too large like we do in
LayerTreeImpl::QueueSwapPromise?

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:319: void QueueScrollUpdateLatencyInfo(const
ui::LatencyInfo& latency_info);
this should be QueueSwapPromiseForMainThreadScrollUpdate

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:600: std::vector<ui::LatencyInfo>
scroll_update_latency_infos_;
ScopedPtrVector<SwapPromise> swap_promises_for_main_thread_scroll_update_

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:988:
ASSERT_TRUE(scroll_info->latency_infos[0].FindLatency(
use EXPECT

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:4530: (*set_needs_redraw_count_)++;
Looks like we could do ADD_FAILURE() here too and get rid of this variable.

[Sami, 2014-07-09 13:28:55]

Thank you for the good suggestions Dana -- all addressed.

https://codereview.chromium.org/365463003/diff/20001/cc/base/latency_info_swa...
File cc/base/latency_info_swap_promise_monitor.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/base/latency_info_swa...
cc/base/latency_info_swap_promise_monitor.cc:52:
layer_tree_host_impl_->QueueScrollUpdateLatencyInfo(*latency_);
On 2014/07/08 16:49:07, danakj wrote:
> Why doesn't this make a SwapPromise and queue that/pass that into the
> ScrollAndScaleSet etc?

Initially I avoided that because otherwise it got tricky to register the flow
event in LTH. However if I make this an explicit LatencyInfoSwapPromise then
that's not a problem anymore. Thanks for the suggestion, I think the result is
much cleaner.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1030: "Blink scroll update");
On 2014/07/08 16:49:07, danakj wrote:
> layering violation: cc doesn't know about blink. You can say main-thread
scroll
> update or something like that

Ah, sorry for the slip-up :) Fixed.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.h (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.h:146: std::vector<ui::LatencyInfo>
latency_infos;
On 2014/07/08 16:49:07, danakj wrote:
> this should be a ScopedPtrVector<SwapPromise>

Done.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:505:
scroll_update_latency_infos_.push_back(latency_info);
On 2014/07/08 16:49:07, danakj wrote:
> Should we be dropping SwapPromises if the list gets too large like we do in
> LayerTreeImpl::QueueSwapPromise?

Good point, I've added the same logic here.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:319: void QueueScrollUpdateLatencyInfo(const
ui::LatencyInfo& latency_info);
On 2014/07/08 16:49:07, danakj wrote:
> this should be QueueSwapPromiseForMainThreadScrollUpdate

Done.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:600: std::vector<ui::LatencyInfo>
scroll_update_latency_infos_;
On 2014/07/08 16:49:07, danakj wrote:
> ScopedPtrVector<SwapPromise> swap_promises_for_main_thread_scroll_update_

Done.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:988:
ASSERT_TRUE(scroll_info->latency_infos[0].FindLatency(
On 2014/07/08 16:49:07, danakj wrote:
> use EXPECT

Done.

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/365463003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:4530: (*set_needs_redraw_count_)++;
On 2014/07/08 16:49:08, danakj wrote:
> Looks like we could do ADD_FAILURE() here too and get rid of this variable.

Well spotted, done.

[danakj, 2014-07-09 15:50:36]

https://codereview.chromium.org/365463003/diff/60001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/365463003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1033:
TRACE_ID_DONT_MANGLE(swap_promise->latency_info().trace_id),
LGTM but if you were to put a TraceId() accessor on SwapPromise, then you could
keep all this general and not need to use the LatencyInfoSwapPromise type
anywhere in LTH/LTHI which might come in handy if we have another SwapPromise
type in the future.

[Sami, 2014-07-09 16:45:23]

On 2014/07/09 15:50:36, danakj wrote:
>
https://codereview.chromium.org/365463003/diff/60001/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/365463003/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:1033:
> TRACE_ID_DONT_MANGLE(swap_promise->latency_info().trace_id),
> LGTM but if you were to put a TraceId() accessor on SwapPromise, then you
could
> keep all this general and not need to use the LatencyInfoSwapPromise type
> anywhere in LTH/LTHI which might come in handy if we have another SwapPromise
> type in the future.

Good idea, that's even cleaner. Done.

[danakj, 2014-07-09 16:58:19]

Thanks!

https://codereview.chromium.org/365463003/diff/100001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/365463003/diff/100001/cc/base/swap_promise.h#...
cc/base/swap_promise.h:47: virtual int64 TraceId() const = 0;
Can you add a comment saying what this is for?

[Sami, 2014-07-09 17:35:21]

On 2014/07/09 16:58:19, danakj wrote:
> Thanks!
> 
> https://codereview.chromium.org/365463003/diff/100001/cc/base/swap_promise.h
> File cc/base/swap_promise.h (right):
> 
>
https://codereview.chromium.org/365463003/diff/100001/cc/base/swap_promise.h#...
> cc/base/swap_promise.h:47: virtual int64 TraceId() const = 0;
> Can you add a comment saying what this is for?

Yup, done!

[danakj, 2014-07-09 17:49:55]

Awesome :) LGTM again in case

[Sami, 2014-07-09 18:18:34]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-07-09 18:20:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/365463003/120001

[commit-bot: I haz the power, 2014-07-09 20:48:45]

Change committed as 282117

[Sami, 2014-07-22 11:11:22]

We reverted this patch since it added a lot of noise to the existing input
latency metric. I've now reworked this following Yufeng's suggestion to branch
off a separate latency info just for the scroll update. That way we can measure
scroll slippage and normal input latency separately.

Yufeng, can you do a sanity check please? Only
cc/base/latency_info_swap_promise_monitor.cc has substantially changed from what
was here before.

[Yufeng Shen (Slow to review), 2014-07-23 20:11:03]

On 2014/07/22 11:11:22, Sami wrote:
> We reverted this patch since it added a lot of noise to the existing input
> latency metric. I've now reworked this following Yufeng's suggestion to branch
> off a separate latency info just for the scroll update. That way we can
measure
> scroll slippage and normal input latency separately.
> 
> Yufeng, can you do a sanity check please? Only
> cc/base/latency_info_swap_promise_monitor.cc has substantially changed from
what
> was here before.

latency part LGTM.

+ sadrul@ for OWNER of ui/events/

[sadrul, 2014-07-23 20:13:32]

events rslgtm

[Sami, 2014-07-24 11:53:26]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-07-24 11:55:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/365463003/220001

[commit-bot: I haz the power, 2014-07-24 12:06:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-24 12:12:59]

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

[commit-bot: I haz the power, 2014-07-24 12:13:00]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[Sami, 2014-07-24 14:03:40]

Thanks! Needed to rebase this a bit, so +kbr@ for content/renderer/gpu approval
please.

[Ken Russell (switch to Gerrit), 2014-07-28 20:15:31]

content/renderer/gpu LGTM

[Sami, 2014-08-04 11:14:00]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-04 11:14:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/365463003/260001

[commit-bot: I haz the power, 2014-08-04 13:09:45]

Change committed as 287325

[tonyg, 2014-08-04 19:26:46]

A revert of this CL has been created in
https://codereview.chromium.org/437163007/ by tonyg@chromium.org.

The reason for reverting is: Breaks smoothness benchmarks on windows:
"ValueError: LatencyInfo has no begin component"

http://chromegw/i/chromium.perf/builders/Win%207%20Intel%20GPU%20Perf/builds/....

[sadrul, 2014-08-05 05:01:35]

On 2014/08/04 19:26:46, tonyg wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/437163007/ by mailto:tonyg@chromium.org.
> 
> The reason for reverting is: Breaks smoothness benchmarks on windows:
> "ValueError: LatencyInfo has no begin component"
> 
>
http://chromegw/i/chromium.perf/builders/Win%207%20Intel%20GPU%20Perf/builds/....

Revert drover-landed in r287458

[Sami, 2014-08-12 10:13:57]

This broke the smoothness metric because it wasn't expecting the new type of
latency data we're adding here. I've now fixed that by adding reporting for the
new metric -- Yufeng, would you mind taking a look at these files:

tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py
tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py
tools/telemetry/telemetry/web_perf/metrics/smoothness.py

The other changes are same as before +/- a rebase.

[Yufeng Shen (Slow to review), 2014-08-12 21:52:22]

On 2014/08/12 10:13:57, Sami wrote:
> This broke the smoothness metric because it wasn't expecting the new type of
> latency data we're adding here. I've now fixed that by adding reporting for
the
> new metric -- Yufeng, would you mind taking a look at these files:
> 
> tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py
> tools/telemetry/telemetry/web_perf/metrics/rendering_stats_unittest.py
> tools/telemetry/telemetry/web_perf/metrics/smoothness.py
> 
> The other changes are same as before +/- a rebase.

tools/telemetry/telemetry/web_perf/metrics/*   lgtm

[Sami, 2014-08-13 10:31:39]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-13 10:35:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/365463003/340001

[commit-bot: I haz the power, 2014-08-13 10:43:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 10:44:50]

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

[commit-bot: I haz the power, 2014-08-13 10:44:51]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Sami, 2014-08-13 11:18:41]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-08-13 11:19:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/365463003/360001

[commit-bot: I haz the power, 2014-08-13 15:25:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-13 16:09:54]

Change committed as 289307