CC: Force push properties for all layers when tracing is started

CC: Force push properties for all layers when tracing is started

... provided categories that cause layer tree snapshots to be traced
are enabled. We need this so that we can show owner nodes for layers
that were not updated on the main thread side for a while.

BUG=

Committed: https://crrev.com/4a4b55844b0cff76d33734555fd3267f0da7826e
Cr-Commit-Position: refs/heads/master@{#320947}

[caseq, 2015-02-20 10:11:56]

Patchset #1 (id:1) has been deleted

[caseq, 2015-02-20 10:21:38]

caseq@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[caseq, 2015-02-20 10:21:39]

We only do this once when tracing is started and only if categories that cause
layer tree snapshots to be dumped are enabled, so this shouldn't incur
noticeable overhead or cause unwanted side effects.

[danakj, 2015-02-20 17:13:18]

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

https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:309: // push properties for all layers to propagate
LayerDebugInfo to
AFAICT LayerDebugInfo is not an actual type name, so we should not make it look
like one in a comment

https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:310: // the impl side.
Can you help fill me in on how this DebugInfo works?

Is the debug info not up to date until a trace starts? Why didn't we just push
it when the layer was created? If not, what generates all the DebugInfo later?
Is there code looking for TRACE_EVENT stuff somewhere else to generate it for
each layer?

https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:314: "devtools.timeline.layers"),
If it's reasonable, I would much prefer we expose this as a method on LTH like
SetNextCommitRequiresAllLayersPushProperties() or something. Then we can write
unit tests in cc for it. Then the TRACE_EVENT snooping code can call that
function..

https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:318: root_layer(),
base::Bind(SetNeedsPushPropertiesForLayer));
I think that walking all the layers to make us walk all the layers is not the
most performant way to go about this. It seems wrong that LayerTreeHost needs to
know this secret knowledge about how Layers trace debug info too. But I have
some other questions above to help tease out our options here..

[caseq, 2015-02-24 15:56:45]

>
https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:309: // push properties for all layers to
propagate
> LayerDebugInfo to
> AFAICT LayerDebugInfo is not an actual type name, so we should not make it
look
> like one in a comment

Why, it's an actual class
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...)

>
https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:310: // the impl side.
> Can you help fill me in on how this DebugInfo works?

Sure -- it originates on the blink side (see GraphicsLayerDebugInfo class for a
good overview of its properties) and is generated on demand (via
WebLayerClient::TakeDebugInfoFor()) and pushed to the impl-side along with other
properties in Layer::PushPropertiesTo() provided tracing is enabled (here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...).

> Is the debug info not up to date until a trace starts?

It actually consists of two parts, one is mostly static (node id, compositing
reasons etc), other varies from frame to frame (invalidations).

> Why didn't we just push it when the layer was created?

I guess this could be done for static part, if we go for splitting
GraphicsLayerDebugInfo into two parts, but current approach slightly cuts the
overhead of propagating these properties to the impl side when tracing is off.

> If not, what generates all the DebugInfo later?

See above.

> Is there code looking for TRACE_EVENT stuff somewhere else to generate it for
> each layer?

Yes -- we emit a snapshot of many layer properties in LayerImpl::AsValueInto()
that is invoked recursively for the entire tree via
LayerTreeHostImpl::AsValueWithFrame() (traced from here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...)

>
https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:314: "devtools.timeline.layers"),
> If it's reasonable, I would much prefer we expose this as a method on LTH like
> SetNextCommitRequiresAllLayersPushProperties() or something. Then we can write
> unit tests in cc for it. Then the TRACE_EVENT snooping code can call that
> function..

This would work for DevTools, as we have an ability to do ad-hoc stuff when
enabling tracing, but this won't work for trace viewer, since its interface to
the backend is quite limited. In general, I think this leaks abstraction -- the
reasonable contract between tracing client and the back-end in my view is
"tracing client enables tracing and the back-end does its best to assure the
client gets all available data according to the trace categories specified", as
opposed to "the client needs to enable tracing and then invoke additional magic
methods depending on what data its interested in, otherwise it gets incomplete
data".

> 
>
https://codereview.chromium.org/924973003/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:318: root_layer(),
> base::Bind(SetNeedsPushPropertiesForLayer));
> I think that walking all the layers to make us walk all the layers is not the
> most performant way to go about this.

Do we care for performance of this step? We only do this once per tracing
session, this will force Layer::PushPropertiesTo() for each layer in the tree
once, but most importantly, when the categories that we check here are enabled,
we do Layer::AsValueInto() for each layer in the tree upon every frame,
producing huge trees of base::Values.

> It seems wrong that LayerTreeHost needs to know this secret knowledge about
how Layers trace debug info too.

One way to get around that would be to extract that into
Layer::DidBeginTracing(), as impl side already does
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...).
I originally thought this to be a bit of overkill, but if encapsulation remains
the only concern here, I think it's reasonable.

[caseq, 2015-03-03 07:05:40]

ping

[danakj, 2015-03-06 18:50:12]

Thanks for the explanations. I do like the DidBeginTracing change and it makes
things more clear what's going on, a few comments, but I'm mostly okay with
this.

https://codereview.chromium.org/924973003/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/924973003/diff/40001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1353: SetNeedsPushProperties();
Can you leave a comment explaining why we do SNPP() here? basically what you've
explained to me in the comments.

https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:309: // push properties for all layers to propagate
LayerDebugInfo to
I see, LayerDebugInfo is a type, up in ui/compositor/ but that only is relevant
for the ui compositor, not for the renderer, so referring to it here is a
layering violation. Can you make this comment more general so it's accurate for
the renderer too?

https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:313: TRACE_DISABLED_BY_DEFAULT("cc.debug") ","
TRACE_DISABLED_BY_DEFAULT(
I don't like that these magic categories appear here and just kinda "know" what
will get traced later. How can we ensure this isn't going to get out of sync at
some point and cause bugs?

(I don't even see the devtools.timeline.layers mentioned here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...)

https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:318: root_layer(),
base::Bind(LayerTreeHostDidBeginTracingCallback));
This will need a rebase.

[caseq, 2015-03-16 18:04:11]

On 2015/03/06 18:50:12, danakj wrote:

> https://codereview.chromium.org/924973003/diff/40001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/924973003/diff/40001/cc/layers/layer.cc#newco...
> cc/layers/layer.cc:1353: SetNeedsPushProperties();
> Can you leave a comment explaining why we do SNPP() here? basically what
you've
> explained to me in the comments.

Done.

> 
>
https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:309: // push properties for all layers to
propagate
> LayerDebugInfo to
> I see, LayerDebugInfo is a type, up in ui/compositor/ but that only is
relevant
> for the ui compositor, not for the renderer, so referring to it here is a
> layering violation. Can you make this comment more general so it's accurate
for
> the renderer too?

Replaced with plain-text "layer debug info".

>
https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:313: TRACE_DISABLED_BY_DEFAULT("cc.debug") ","
> TRACE_DISABLED_BY_DEFAULT(
> I don't like that these magic categories appear here and just kinda "know"
what
> will get traced later. How can we ensure this isn't going to get out of sync
at
> some point and cause bugs?

This is now encapsulated in frame_viewer_instrumentation. Unfortunately, since
it now
calls back to LayerTreeHostImpl::AsValueWithFrame() and I thought it would be
bad idea to
include layer_tree_host_impl.h from frame_viewer_instrumentation.h, I had to
extract
LayerTreeHostImpl::FrameData to the top-level class so I can forward-declare it.
Hope it's ok :)

> (I don't even see the devtools.timeline.layers mentioned here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...)
> 
>
https://codereview.chromium.org/924973003/diff/40001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:318: root_layer(),
> base::Bind(LayerTreeHostDidBeginTracingCallback));
> This will need a rebase.

Done.

[danakj, 2015-03-16 19:06:49]

On 2015/03/16 18:04:11, caseq wrote:
> On 2015/03/06 18:50:12, danakj wrote:
> > cc/trees/layer_tree_host.cc:313: TRACE_DISABLED_BY_DEFAULT("cc.debug") ","
> > TRACE_DISABLED_BY_DEFAULT(
> > I don't like that these magic categories appear here and just kinda "know"
> what
> > will get traced later. How can we ensure this isn't going to get out of sync
> at
> > some point and cause bugs?
> 
> This is now encapsulated in frame_viewer_instrumentation. Unfortunately, since
> it now
> calls back to LayerTreeHostImpl::AsValueWithFrame() and I thought it would be
> bad idea to
> include layer_tree_host_impl.h from frame_viewer_instrumentation.h, I had to
> extract
> LayerTreeHostImpl::FrameData to the top-level class so I can forward-declare
it.
> Hope it's ok :)

Hm! Thanks, this is a good idea how to make this consistent.

If we're going to make FrameData a top level class we should really give it a
better name, but I'm not sure it's needed. Can we maybe just expose that new
constant and make use of it in both places?

https://codereview.chromium.org/924973003/diff/60001/cc/debug/frame_viewer_in...
File cc/debug/frame_viewer_instrumentation.cc (right):

https://codereview.chromium.org/924973003/diff/60001/cc/debug/frame_viewer_in...
cc/debug/frame_viewer_instrumentation.cc:78: TRACE_EVENT0("cc",
"DrawLayers.FrameViewerTracing");
Can you make this trace event name more like the function name it's found in?

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

https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:298: bool is_new_trace;
Can you move this up above the |sync_tree| variable? It's kinda in the middle of
"update the sync tree" here

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

https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1528: TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(
Did you mean to call TraceLayerTreeSnapshot here?

[caseq, 2015-03-17 12:21:29]

On 2015/03/16 19:06:49, danakj wrote:
> On 2015/03/16 18:04:11, caseq wrote:
> > On 2015/03/06 18:50:12, danakj wrote:
> > > cc/trees/layer_tree_host.cc:313: TRACE_DISABLED_BY_DEFAULT("cc.debug") ","
> > > TRACE_DISABLED_BY_DEFAULT(
> > > I don't like that these magic categories appear here and just kinda "know"
> > what
> > > will get traced later. How can we ensure this isn't going to get out of
sync
> > at
> > > some point and cause bugs?
> > 
> > This is now encapsulated in frame_viewer_instrumentation. Unfortunately,
since
> > it now
> > calls back to LayerTreeHostImpl::AsValueWithFrame() and I thought it would
be
> > bad idea to
> > include layer_tree_host_impl.h from frame_viewer_instrumentation.h, I had to
> > extract
> > LayerTreeHostImpl::FrameData to the top-level class so I can forward-declare
> it.
> > Hope it's ok :)
> 
> Hm! Thanks, this is a good idea how to make this consistent.
> 
> If we're going to make FrameData a top level class we should really give it a
> better name, but I'm not sure it's needed. Can we maybe just expose that new
> constant and make use of it in both places?

Sure, done! (I still left IsTracingLayerTreeSnapshots() to check the category
for
brevity).

>
https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host.cc:298: bool is_new_trace;
> Can you move this up above the |sync_tree| variable? It's kinda in the middle
of
> "update the sync tree" here

Done.

>
https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/924973003/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:1528: TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(
> Did you mean to call TraceLayerTreeSnapshot here?

Ouch! I did, but leaving this as is now per your suggestion above.

[danakj, 2015-03-17 17:08:53]

LGTM

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:274: if (is_new_trace &&
I think you need if (root_layer()) as well here.

[danakj, 2015-03-17 17:09:15]

> BUG=

Either point at a bug or just remove this from the patch description :)

[caseq, 2015-03-17 17:44:55]

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:274: if (is_new_trace &&
On 2015/03/17 17:08:53, danakj wrote:
> I think you need if (root_layer()) as well here.

Thanks, done!
FWIW we seem to unconditionally arrive to a call to
TreeSynchronizer::PushProperties() at the end which does a DCHECK() on its
source root layer (sure, there's still pretty important difference in case of
release :))

[caseq, 2015-03-17 17:45:31]

The CQ bit was checked by caseq@chromium.org

[caseq, 2015-03-17 17:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/924973003/#ps100001
(title: "added root_layer() check")

[commit-bot: I haz the power, 2015-03-17 17:45:48]

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

[danakj, 2015-03-17 18:36:05]

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/924973003/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:274: if (is_new_trace &&
On 2015/03/17 17:44:55, caseq wrote:
> On 2015/03/17 17:08:53, danakj wrote:
> > I think you need if (root_layer()) as well here.
> 
> Thanks, done!
> FWIW we seem to unconditionally arrive to a call to
> TreeSynchronizer::PushProperties() at the end which does a DCHECK() on its
> source root layer (sure, there's still pretty important difference in case of
> release :))

I think it does an early out:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/tree_sync...

We can definitely commit without a root layer.

[commit-bot: I haz the power, 2015-03-17 18:39:45]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-03-17 18:40:34]

Patchset 5 (id:??) landed as
https://crrev.com/4a4b55844b0cff76d33734555fd3267f0da7826e
Cr-Commit-Position: refs/heads/master@{#320947}