Add impl/comp scrolling instrumentation.

Add impl/comp scrolling instrumentation.

While developing universal overflow scroll, I added a number of trace events
that will, I think, be of general use. I've created this patch to add them on
their own since they're not really related to the rest of that work.

The goal is to allow someone to run chrome with the flag --trace-to-console=impl-scroll and see, at a glance, how the impl-scroll code path works. It's intended to be educational.

R=danakj@chromium.org,enne@chromium.org
BUG=None

[danakj, 2013-07-29 17:16:07]

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:259: "scrollX", sent_scroll_delta.x(),
scroll_x?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:260: "scrollY", sent_scroll_delta.y());
scroll_y?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:268: "scrollX", scroll.x(),
same.

consider making a base::Value containing the data and passing that to the event,
instead of passing values directly, so we can add more things in the future more
easily.

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:286: TRACE_EVENT_INSTANT0("cc,impl-scroll",
Should we drop these from the "cc" category then?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:293: TRACE_EVENT_INSTANT0("cc,impl-scroll",
i think "impl" as a name is something to avoid and not as
discoverable/understandable to people in general. what about just calling these
"scroll[ing?]"? wdyt?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:352: "layerId", id());
layer_id?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:1138: #ifndef NDEBUG
Why only in not NDEBUG?

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1765: "layerId", layer_impl->id());
layer_id ?

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1789: "layerId", scroll_layer_impl->id());
layer_id?

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1812: "layerId", scroll_layer_impl->id());
layer_id?

https://codereview.chromium.org/20994007/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/20994007/diff/1/cc/trees/thread_proxy.cc#newc...
cc/trees/thread_proxy.cc:698: "ThreadProxy::BeginFrameOnMainThread",
You have a trace event in ApplyScrollAndScale, and that's the only thing
happening here, so I'm not sure that you need both?

[enne (OOO), 2013-07-29 17:46:31]

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:268: "scrollX", scroll.x(),
On 2013/07/29 17:16:08, danakj wrote:
> same.
> 
> consider making a base::Value containing the data and passing that to the
event,
> instead of passing values directly, so we can add more things in the future
more
> easily.

Even better, add a MathUtil::AsValue for gfx::Vector2d.

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:293: TRACE_EVENT_INSTANT0("cc,impl-scroll",
On 2013/07/29 17:16:08, danakj wrote:
> i think "impl" as a name is something to avoid and not as
> discoverable/understandable to people in general. what about just calling
these
> "scroll[ing?]"? wdyt?

Do you think there'll ever be a blink scrolling trace? What about cc-scroll?

https://codereview.chromium.org/20994007/diff/1/cc/layers/layer_impl.cc#newco...
cc/layers/layer_impl.cc:1138: #ifndef NDEBUG
On 2013/07/29 17:16:08, danakj wrote:
> Why only in not NDEBUG?

Layers only get their names set in debug builds.  You're welcome to land this,
but there's some work in progress being done to pull names from Blink but only
when tracing is on, rather than always pushing names from Blink only for debug
builds.

See: https://codereview.chromium.org/18473002/ or
https://codereview.chromium.org/16903005/.

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

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host.cc#n...
cc/trees/layer_tree_host.cc:968: const bool has_scrolls = !info.scrolls.empty();
...or page scale delta != 1?

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl.cc
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/20994007/diff/1/cc/trees/layer_tree_host_impl...
cc/trees/layer_tree_host_impl.cc:1833: "impl-scroll",
"LayerTreeHostImpl::ScrollBegin: start impl-scroll.",
Would it make sense to do something with trace event async begin and step for
scrolls like we do with the pending tree in LTHI?

https://codereview.chromium.org/20994007/diff/1/content/renderer/gpu/render_w...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/20994007/diff/1/content/renderer/gpu/render_w...
content/renderer/gpu/render_widget_compositor.cc:538:
"RenderWidgetCompositor::ApplyScrollAndScale");
Does the other apply scroll and scale trace not cover this?

[Ian Vollick, 2013-07-30 19:37:26]

This latest patch has the AsValue stuff you'd both mentioned, the
layerId->layer_id renames Dana mentioned as well as switching to async
begin/events for scrolls.

I'd be happy to discuss the category names, but I should explain my motivation
first (I'll update the description to include this motivation -- it should have
been there in the first place).

This came out of wanting to see, at a glance, the entire impl-scrolling code
path. As I was spelunking through the code refamiliarizing myself with all of
this, I thought it would be nice if someone was able to run chrome with
--trace-to-console=impl-scroll and see exactly how the feature worked under the
hood. I thought this might be a nice thing to do as an educational aid for other
large features, too.

So I don't want to alter the cc category, I think all of those catergorizations
are accurate, I just want to tag the entire impl-scroll code path as well. Parts
of this code path live on the blink side, and I hope to add trace events there,
too, for the same purpose.

comp-scroll is a bit different. That's stuff that's relate entirely with
composited overflow scrolling, unlike impl-scroll, which refers to the code run
to scroll anything off the main thread, be it the main frame, an overflow
scrolling div or an iframe.

[dsinclair, 2013-08-08 17:30:14]

https://codereview.chromium.org/20994007/diff/25001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/20994007/diff/25001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1004: TRACE_EVENT_ASYNC_BEGIN0(
Why not TRACE_EVENT_BEGIN0 and END0 below?

[danakj, 2013-09-26 19:21:59]

ping, is this still meant to land?

[Ian Vollick, 2013-09-27 02:43:17]

On 2013/09/26 19:21:59, danakj wrote:
> ping, is this still meant to land?

nah, I'll close.