[Invalidation Tracking] Add trace events for compositor based invalidations

[Invalidation Tracking] Add trace events for compositor based invalidations

This patch add a new trace event InspectorLayerInvalidationTrackingEvent for invalidation reason tracking. The event tracks when compositor invalidates the whole RenderLayer including its descendents, along with its reason.

BUG=410701
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181933

[kouhei (in TOK), 2014-09-12 00:20:29]

kouhei@chromium.org changed reviewers:
+ caseq@chromium.org, chrishtr@chromium.org, pdr@chromium.org

[kouhei (in TOK), 2014-09-12 00:20:30]

Would you take a look?

[chrishtr, 2014-09-12 00:27:03]

You can extract the paint invalidation container by calling:

layer.renderer()->containerForPaintInvalidation();

Doesn't matter if it is inefficient, because it's not enabled by default.

Also please change the strings into enum values or names of static const*
globals.

[kouhei (in TOK), 2014-09-12 01:14:41]

On 2014/09/12 00:27:03, chrishtr wrote:
> You can extract the paint invalidation container by calling:
> 
> layer.renderer()->containerForPaintInvalidation();
> 
> Doesn't matter if it is inefficient, because it's not enabled by default.
> 
> Also please change the strings into enum values or names of static const*
> globals.

Thanks for your comments! Updated CL.

[chrishtr, 2014-09-12 01:22:46]

lgtm

https://codereview.chromium.org/565793003/diff/40001/Source/core/inspector/In...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/565793003/diff/40001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:262: const char
InspectorLayerInvalidationTrackingEvent::RemovedFromSquashingLayer[] = "Removed
the layer from a squashed layer.";
"Removed the layer from a squashing layer"

[kouhei (in TOK), 2014-09-12 02:29:47]

pdr: Would you take a look?

On 2014/09/12 01:22:46, chrishtr wrote:
> lgtm
> 
>
https://codereview.chromium.org/565793003/diff/40001/Source/core/inspector/In...
> File Source/core/inspector/InspectorTraceEvents.cpp (right):
> 
>
https://codereview.chromium.org/565793003/diff/40001/Source/core/inspector/In...
> Source/core/inspector/InspectorTraceEvents.cpp:262: const char
> InspectorLayerInvalidationTrackingEvent::RemovedFromSquashingLayer[] =
"Removed
> the layer from a squashed layer.";
> "Removed the layer from a squashing layer"

Done.

[pdr., 2014-09-12 04:07:17]

https://codereview.chromium.org/565793003/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/565793003/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:260: const char
InspectorLayerInvalidationTrackingEvent::SquashingLayerGeometryWasUpdated[] =
"Squashing layer geometry was updated.";
I'm a little worried about using the word "Squashing" because only a handful of
folks understand what it means. I think we should move forward with this as it
is, but we may need to revisit it in the future.

https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/GraphicsLayerUpdater.cpp (right):

https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:102:
layersNeedingPaintInvalidation.append(std::make_pair(&layer,
InspectorLayerInvalidationTrackingEvent::AncestorRequiresNewLayer));
Why do we need to track the {layer,reason} pair together? Could we just issue
the trace event here instead?

[kouhei (in TOK), 2014-09-12 05:02:12]

Thanks for your comments!

https://codereview.chromium.org/565793003/diff/60001/Source/core/inspector/In...
File Source/core/inspector/InspectorTraceEvents.cpp (right):

https://codereview.chromium.org/565793003/diff/60001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:260: const char
InspectorLayerInvalidationTrackingEvent::SquashingLayerGeometryWasUpdated[] =
"Squashing layer geometry was updated.";
On 2014/09/12 04:07:17, pdr wrote:
> I'm a little worried about using the word "Squashing" because only a handful
of
> folks understand what it means. I think we should move forward with this as it
> is, but we may need to revisit it in the future.

Understood.

https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
File Source/core/rendering/compositing/GraphicsLayerUpdater.cpp (right):

https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:102:
layersNeedingPaintInvalidation.append(std::make_pair(&layer,
InspectorLayerInvalidationTrackingEvent::AncestorRequiresNewLayer));
On 2014/09/12 04:07:17, pdr wrote:
> Why do we need to track the {layer,reason} pair together? Could we just issue
> the trace event here instead?
The string is put into vector to avoid code duplication.
It was originally written in that way (Patch Set #1), but the 5 lines are mostly
copy&paste as chrishtr@ pointed out.

[pdr., 2014-09-12 05:09:25]

On 2014/09/12 05:02:12, kouhei wrote:
>
https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
> Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:102:
> layersNeedingPaintInvalidation.append(std::make_pair(&layer,
> InspectorLayerInvalidationTrackingEvent::AncestorRequiresNewLayer));
> On 2014/09/12 04:07:17, pdr wrote:
> > Why do we need to track the {layer,reason} pair together? Could we just
issue
> > the trace event here instead?
> The string is put into vector to avoid code duplication.
> It was originally written in that way (Patch Set #1), but the 5 lines are
mostly
> copy&paste as chrishtr@ pointed out.

What about a macro in InspectorTraceEvents.h like
TRACE_LAYER_INVALIDATION(layer,
InspectorLayerInvalidationTrackingEvent::reason)? I'd like to avoid functional
changes for traces (where possible) because they add a maintenance cost.

[kouhei (in TOK), 2014-09-12 17:33:32]

On 2014/09/12 05:09:25, pdr wrote:
> On 2014/09/12 05:02:12, kouhei wrote:
> >
>
https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
> > Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:102:
> > layersNeedingPaintInvalidation.append(std::make_pair(&layer,
> > InspectorLayerInvalidationTrackingEvent::AncestorRequiresNewLayer));
> > On 2014/09/12 04:07:17, pdr wrote:
> > > Why do we need to track the {layer,reason} pair together? Could we just
> issue
> > > the trace event here instead?
> > The string is put into vector to avoid code duplication.
> > It was originally written in that way (Patch Set #1), but the 5 lines are
> mostly
> > copy&paste as chrishtr@ pointed out.
> 
> What about a macro in InspectorTraceEvents.h like
> TRACE_LAYER_INVALIDATION(layer,
> InspectorLayerInvalidationTrackingEvent::reason)? I'd like to avoid functional
> changes for traces (where possible) because they add a maintenance cost.

Updated patch to use the C macro approach. PTAL.

[pdr., 2014-09-12 20:12:16]

On 2014/09/12 17:33:32, kouhei wrote:
> On 2014/09/12 05:09:25, pdr wrote:
> > On 2014/09/12 05:02:12, kouhei wrote:
> > >
> >
>
https://codereview.chromium.org/565793003/diff/60001/Source/core/rendering/co...
> > > Source/core/rendering/compositing/GraphicsLayerUpdater.cpp:102:
> > > layersNeedingPaintInvalidation.append(std::make_pair(&layer,
> > > InspectorLayerInvalidationTrackingEvent::AncestorRequiresNewLayer));
> > > On 2014/09/12 04:07:17, pdr wrote:
> > > > Why do we need to track the {layer,reason} pair together? Could we just
> > issue
> > > > the trace event here instead?
> > > The string is put into vector to avoid code duplication.
> > > It was originally written in that way (Patch Set #1), but the 5 lines are
> > mostly
> > > copy&paste as chrishtr@ pointed out.
> > 
> > What about a macro in InspectorTraceEvents.h like
> > TRACE_LAYER_INVALIDATION(layer,
> > InspectorLayerInvalidationTrackingEvent::reason)? I'd like to avoid
functional
> > changes for traces (where possible) because they add a maintenance cost.
> 
> Updated patch to use the C macro approach. PTAL.

Thanks! I think this looks great. LGTM

@chrishtr, could you please take a second look?

[chrishtr, 2014-09-12 21:12:53]

lgtm

[kouhei (in TOK), 2014-09-12 21:15:13]

Thanks for review!

[kouhei (in TOK), 2014-09-12 21:15:18]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-09-12 21:16:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565793003/80001

[commit-bot: I haz the power, 2014-09-12 21:21:18]

Committed patchset #5 (id:80001) as 181933