[ Do not submit ] Prototype for invalidation analysis

[ Do not submit ] Prototype for invalidation analysis

This is a prototype for invalidation root-cause analysis in devtools
using trace events. Our design doc is at:
https://docs.google.com/document/d/1BXT3_gD--YdbIYMlTzNGsyUUMBCZJ7V5qGJhMA-pGrs/edit?usp=sharing

This patch implements a simple prototype of style recalc invalidation
reasons that lead to layout. A trace event has been added to
Node::setNeedsStyleRecalc which is passed through devtools to the
devtools timeline and is listed on the layout event.

Screenshot: http://pr.gg/invalidations4.png

[pdr., 2014-08-13 04:52:56]

Didn't mean to send this to blink-reviews. This is only intended for Kouhei.

[kouhei (in TOK), 2014-08-13 16:32:14]

This is amazing. O_o

https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineModel.js (right):

https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:502: layoutInitator =
this._lastRecalculateStylesEvent.initiator;
This line is interesting. Looks like they are already doing something similar?

https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineModel.js:509:
event.invalidations = this._invalidations[frameId] ?
this._invalidations[frameId].join('<br>') : 'No invalidations';
I'm wondering how well we should track the flow of invalidation events. I'm not
yet sure.

Is this timeline approach enough for most purposes? This approach is very
simple, but if tons of style recalc happen at the same frame (frame as in fps),
it would be impossible to query "what javascript line triggered this specific
invalidationRect". Supporting this kinds of query may significantly save time
minimizing repaint storm bugs, but it may be obvious from the node it touched.

If we track NodeId, RenderObjectId, {Render,Graphics,cc::} layer Id in the
invalidation trace events, we should be able to reconstruct the whole
invalidation flow accurately.

I guess we can do this incrementally and delay decision. These events are only
used by us and devtools UI lives in the same repo, it would not be so hard to
change the event data structure.

I'll use this patch locally and try it on minimizing bugs so we can find real
features we need.

[pdr., 2014-08-14 03:53:14]

On 2014/08/13 16:32:14, kouhei wrote:
> This is amazing. O_o
> 
>
https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
> File Source/devtools/front_end/timeline/TracingTimelineModel.js (right):
> 
>
https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
> Source/devtools/front_end/timeline/TracingTimelineModel.js:502: layoutInitator
=
> this._lastRecalculateStylesEvent.initiator;
> This line is interesting. Looks like they are already doing something similar?

Yeah! This is how they handle layout invalidations. They just do the basic thing
here so it's not very useful, but I'm hopeful that there is an incremental path
forward where we just improve these invalidations at each step of the way until
you can find the root cause of a paint.

> 
>
https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
> Source/devtools/front_end/timeline/TracingTimelineModel.js:509:
> event.invalidations = this._invalidations[frameId] ?
> this._invalidations[frameId].join('<br>') : 'No invalidations';
> I'm wondering how well we should track the flow of invalidation events. I'm
not
> yet sure.
> 
> Is this timeline approach enough for most purposes? This approach is very
> simple, but if tons of style recalc happen at the same frame (frame as in
fps),
> it would be impossible to query "what javascript line triggered this specific
> invalidationRect". Supporting this kinds of query may significantly save time
> minimizing repaint storm bugs, but it may be obvious from the node it touched.

The UI side will be difficult :/ I think we should let real examples drive this.

> 
> If we track NodeId, RenderObjectId, {Render,Graphics,cc::} layer Id in the
> invalidation trace events, we should be able to reconstruct the whole
> invalidation flow accurately.

One of the problems here is that at style recalc time we may only know the node,
and possibly the node that caused the invalidation. Similarly, when a renderer
needs to be laid out we do not know the compositor layer id.

Idea:
When we call setNeedsStyleRecalc, we send a trace event with the nodeid, style
change type, invalidating nodeid, etc.
When we call setNeedsLayout, we send a trace event with the nodeid, renderer,
etc.
When we update the compositing layers, we send a trace event with the
invalidated layers and associated renderobjects

In a post-processing step (easy in devtools), we can go through the layout
invalidations and update style invalidations with the appropriate renderer.
Similarly, when the compositing update happens we go through both the layout and
style invalidations and add more data. Does that make sense and/or sound
possible?

[kouhei (in TOK), 2014-08-15 02:20:17]

>
https://codereview.chromium.org/465223002/diff/20001/Source/devtools/front_en...
> > Source/devtools/front_end/timeline/TracingTimelineModel.js:509:
> > event.invalidations = this._invalidations[frameId] ?
> > this._invalidations[frameId].join('<br>') : 'No invalidations';
> > I'm wondering how well we should track the flow of invalidation events. I'm
> not
> > yet sure.
> > 
> > Is this timeline approach enough for most purposes? This approach is very
> > simple, but if tons of style recalc happen at the same frame (frame as in
> fps),
> > it would be impossible to query "what javascript line triggered this
specific
> > invalidationRect". Supporting this kinds of query may significantly save
time
> > minimizing repaint storm bugs, but it may be obvious from the node it
touched.
> 
> The UI side will be difficult :/ I think we should let real examples drive
this.

I want this when debugging a real-world website. A lot of things happen in a
single frame, so it is hard to track down what caused this repaint.
I'm not yet sure about the granularity we need. (v8 stacktrace? css line? node?)

> > If we track NodeId, RenderObjectId, {Render,Graphics,cc::} layer Id in the
> > invalidation trace events, we should be able to reconstruct the whole
> > invalidation flow accurately.
> 
> One of the problems here is that at style recalc time we may only know the
node,
> and possibly the node that caused the invalidation. Similarly, when a renderer
> needs to be laid out we do not know the compositor layer id.
> 
> Idea:
> When we call setNeedsStyleRecalc, we send a trace event with the nodeid, style
> change type, invalidating nodeid, etc.
> When we call setNeedsLayout, we send a trace event with the nodeid, renderer,
> etc.
> When we update the compositing layers, we send a trace event with the
> invalidated layers and associated renderobjects
> 
> In a post-processing step (easy in devtools), we can go through the layout
> invalidations and update style invalidations with the appropriate renderer.
> Similarly, when the compositing update happens we go through both the layout
and
> style invalidations and add more data. Does that make sense and/or sound
> possible?
Yes. I think we can do that.

We can dump the set of ids that is available at each trace point, and we can
match the corresponding invalidation events at the post-processing stage.

[caseq, 2014-08-26 12:19:08]

caseq@chromium.org changed reviewers:
+ caseq@chromium.org

[caseq, 2014-08-26 12:19:08]

https://codereview.chromium.org/465223002/diff/80001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/465223002/diff/80001/Source/core/dom/Node.cpp...
Source/core/dom/Node.cpp:780:
TRACE_DISABLED_BY_DEFAULT("devtools.timeline.styleInvalidationTracking"),
nit: I wonder if category needs to be that specific (i.e. can we just have
devtools.timeline.invalidations, or will we have the analysis of different types
of invalidations independently)?

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

https://codereview.chromium.org/465223002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:129:
setGeneratingNodeId(value.get(), "nodeId", renderer);
can we just use the node id of the generating node here?

https://codereview.chromium.org/465223002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:131:
value->setString("callstack", callstack ?
callstack->buildInspectorArray()->toJSONString() : "[]");
we normally expose callstacks via a separate event adjacent to the "payload"
instrumentation event. The reason for that is that they're in a category of
their own, so we may control the instrumentation overhead by toggling checkbox
in the Timeline UI (stack collection is relatively expensive).

https://codereview.chromium.org/465223002/diff/80001/Source/core/inspector/In...
Source/core/inspector/InspectorTraceEvents.cpp:141:
setGeneratingNodeId(value.get(), "nodeId", renderer);
ditto.

https://codereview.chromium.org/465223002/diff/80001/Source/core/rendering/Gr...
File Source/core/rendering/GraphicsContextAnnotator.cpp (right):

https://codereview.chromium.org/465223002/diff/80001/Source/core/rendering/Gr...
Source/core/rendering/GraphicsContextAnnotator.cpp:97:
InspectorPaintInvalidationTrackingEvent::data(renderer));
Why do we need this? We already have nodeId in the annotation...

https://codereview.chromium.org/465223002/diff/80001/Source/devtools/front_en...
File Source/devtools/front_end/timeline/TracingTimelineUIUtils.js (right):

https://codereview.chromium.org/465223002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:703:
this._buildInvalidationDetailsNode("Style invalidations", linkifier, event,
event.styleInvalidationTrackingEvents));
Display strings should be passed via WebInspector.UIString()

https://codereview.chromium.org/465223002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:833:
invalidationEvents.forEach(function(invalidationEvent, idx) {
Does this produce readable output for large invalidations?

https://codereview.chromium.org/465223002/diff/80001/Source/devtools/front_en...
Source/devtools/front_end/timeline/TracingTimelineUIUtils.js:840: var node =
target.domModel.nodeForId(invalidationEvent.nodeId);
Does this actually work? It looks like it shouldn't...

[kouhei (in TOK), 2014-09-11 18:03:36]

https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
File Source/core/inspector/InspectorTraceEvents.h (right):

https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
Source/core/inspector/InspectorTraceEvents.h:38: class
InspectorStyleInvalidationTrackingEvent {
Can we rename this to InspectorStyleRecalcInvalidationTrackingEvent? I want to
use this for StyleInvalidator invalidation (a invalidation flag which is queried
before StyleInvalidator::invaldiate). StyleInvalidator then sets
setNeedsStyleRecalc on those nodes.

[pdr., 2014-09-11 18:06:04]

On 2014/09/11 18:03:36, kouhei wrote:
>
https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
> File Source/core/inspector/InspectorTraceEvents.h (right):
> 
>
https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
> Source/core/inspector/InspectorTraceEvents.h:38: class
> InspectorStyleInvalidationTrackingEvent {
> Can we rename this to InspectorStyleRecalcInvalidationTrackingEvent? I want to
> use this for StyleInvalidator invalidation (a invalidation flag which is
queried
> before StyleInvalidator::invaldiate). StyleInvalidator then sets
> setNeedsStyleRecalc on those nodes.

Yeah, that sounds reasonable.

Do we need both InspectorStyleInvalidationTrackingEvent and
InspectorStyleRecalcInvalidationTrackingEvent for cases when setNeedsStyleRecalc
isn't called through style invalidator, or do all style invalidations pass
through the style invalidator?

[kouhei (in TOK), 2014-09-11 18:09:46]

On 2014/09/11 18:06:04, pdr wrote:
> On 2014/09/11 18:03:36, kouhei wrote:
> >
>
https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
> > File Source/core/inspector/InspectorTraceEvents.h (right):
> > 
> >
>
https://codereview.chromium.org/465223002/diff/140001/Source/core/inspector/I...
> > Source/core/inspector/InspectorTraceEvents.h:38: class
> > InspectorStyleInvalidationTrackingEvent {
> > Can we rename this to InspectorStyleRecalcInvalidationTrackingEvent? I want
to
> > use this for StyleInvalidator invalidation (a invalidation flag which is
> queried
> > before StyleInvalidator::invaldiate). StyleInvalidator then sets
> > setNeedsStyleRecalc on those nodes.
> 
> Yeah, that sounds reasonable.
> 
> Do we need both InspectorStyleInvalidationTrackingEvent and
> InspectorStyleRecalcInvalidationTrackingEvent for cases when
setNeedsStyleRecalc
> isn't called through style invalidator, or do all style invalidations pass
> through the style invalidator?
We need both. Not all the "recalcStyle" invalidations pass through style
invalidator, like "webfont load complete".