catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 562 forks source link

Find associated events for CSS Animations #1433

Open benshayden opened 9 years ago

benshayden commented 9 years ago

From #1430 I think this will require adding new flow events to blink, though I'm not sure exactly where.

Edit 2015 Dec 8 Currently, RAIL cannot find any associated events for CSS Animations. RAIL does not know how much CPU to attribute to an animation. RAIL does it know what events could affect the performance of an animation. RAIL cannot compute the speed or jank.

There are two possible approaches to computing associated events: following flow events, and context traits (frameblamr). Following flow events requires tracing them in chromium and processing them in catapult, but can yield critical path analysis and nice flow of execution visualizations. Context traits would require logging the LocalFrame in the Animation async events. Then catapult can simply re-use frameblamr and find all of the sync slices during the css animation that were associated with the same frame as the Animation. This would include work that was not caused by the css animation, but would include work that could affect the animation. This tension between "caused by" and "influenced by" is an open question for RAIL associated events. Maybe RAILIR.associatedEvents should be split into "attributedEvents" and "influencingEvents"?

I think we might want to try to do both flow events and context traits. Since context traits would just require adding the LocalFrame pointer to Animation async events, we can do that first while we continue to figure out how to add flow events in chromium. We can add the LocalFrame pointer to Animation async events and objectify Animation async events in catapult even before Frameblamr lands, but catapult won't be able to find CSS Animation associatedEvents until after Frameblamr lands.

Next steps: Ben: add LocalFrame pointer to Animation async events (unblocked) Ben: objectify Animation async events in catapult (unblocked) Ben: schedule a meeting that Nat and Dan can do to talk with Doug about how flow events work Ben: After Frameblamr lands: find CSS Animation associated events!

dstoc commented 9 years ago

As a start I think we can add flow events in the same way that the async spans are generated.

dj2 commented 9 years ago

@dstockwell do you know how would be best suited to add the needed events?

dstoc commented 9 years ago

@dj2 I can add them, what exactly is necessary for the IR finder?

dj2 commented 9 years ago

@benshayden can you let Doug know what you need from him?

benshayden commented 9 years ago

I need flow events from the Animation async slices to whatever trace events are directly caused by the animations. I don't know how CSS Animations work, so I don't know what slices would be caused directly by animations. Basically, when you take a trace of an animation in chrome://tracing, then you should be able to see chains of spline-arrows from the Animation async slices in the Renderer process to the paint and frame events and whatever else is caused directly or indirectly by the animation.

I can make the finder follow the flow events and add the flowed-to slices to the Animation IR's associatedEvents.

For bonus points if you're up for it while you're in there: a flow event from the Document::updateStyle (or whatever else that can cause css animations) to the Animation async slice. The IR finder probably would not use this flow event, but it might be neat to be able to see when a Response causes a CSS Animation.

Thank you in advance!

benshayden commented 9 years ago

Sorry, I meant that the FlowEvents should have the same id as the Animation async slice, like how FlowEvents have the same id as latencyInfo async slices. Of course the flow events should start and end at ThreadSlices. I'm planning on using basically the same logic as InputLatencyAsyncSlice, insofar as it works for CSS Animations. https://github.com/catapult-project/catapult/blob/master/tracing/tracing/extras/chrome/cc/input_latency_async_slice.html So, in tracing/extras/chrome, I'll register an AsyncSlice subclass named Animation to handle async slices titled "Animation" in the category "blink.animations", and override its associatedEvents getter with logic based on InputLatencyAsyncSlice.associatedEvents.

benshayden commented 9 years ago

Maybe like this: https://codereview.chromium.org/1338633004

dstoc commented 9 years ago

Tried to add this, but there's no INTERNAL_TRACE_EVENT_ADD_SCOPED_WITH_FLOW in blink. It failed horribly when I tried to hack it in.

dstoc commented 9 years ago

@dj2, @vmpstr, Any documentation on flow events?

Is it preferred to use the *with_flow over flow_begin/step/end? I ask because there's no ...with_flow_begin/end yet.

What happens when I flow in our out multiple events with the same flow id?

dj2 commented 9 years ago

flow_begin/step/end is deprecated. The with_flow variants are the new flow v2 and provide more flexibility.

The design doc is at: https://docs.google.com/document/d/1La_0PPfsTqHJihazYhff96thhjPtvq1KjAUOJu0dvEg/edit#heading=h.mlpdir2vdooz

dstoc commented 9 years ago

@dj2 OK, I'll prefer with_flow. But in one case I was trying to convert there's an existing TRACE_EVENT_BEGIN/END pair and I can't see how to convert that.

dj2 commented 9 years ago

Link?

dstoc commented 9 years ago

eg. Document::updateLayoutTree: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/dom/Document.cpp#1743

dj2 commented 9 years ago

I'd be cautious modifying any of the devtools.timeline events without talking to them first as they use that for their timeline.

benshayden commented 8 years ago

@dstockwell Still working on this?

In Document::updateLayoutTree, could we add a new trace event like this? TRACE_EVENT_WITH_FLOW0("blink", "Document::updateLayoutTree", animationId, FLOW_IN);

dstoc commented 8 years ago

@benshayden, haven't made any progress here. This was waiting on an explanation of how to use flow events correctly. So we should discuss further.

benshayden commented 8 years ago

We might be able to use @skyostil 's Trace Contexts (frameblamr) to simplify this attribution problem as well as loading. Until frameblamr is sorted out, though, maybe we could keep trying to get flow events working here. Unfortunately, my knowledge of the corner cases of flow events is limited, and the design doc doesn't explain how to use them in this case. For example, how does trace-viewer handle multiple flow events with the same id? For example, many SetNeedsStyleRecalc calls may flow into a single updateStyle slice.

@natduca Should we try to use frameblamr here? Should we schedule a meeting to sort out how to use flow events here?

natduca commented 8 years ago

Mmm good insight. I think you're on the right track, but if you dig deep you'll find that frameblamr isn't the full story. Namely, frameblamr will let us connect a given slice to a given execution context. But, there is no way to connect an async event to a slice... if we had that, then it'd work. How would we do that?

E.g. inside the recalcStyle, the animation event is going to get created. Frameblamr is going to tell us the context for the recalcStyle [hopefully, @skyostil am I batty?] But, the animation engine running is what is going to create the async slice... we'll need to figure out how create a connection in the trace (or some new implicit rule) that says that the async event has the contextid of the enclosing slice...

skyostil commented 8 years ago

E.g. inside the recalcStyle, the animation event is going to get created. Frameblamr is going to tell us the context for the recalcStyle [hopefully, @skyostil am I batty?]

Yeah, we should be able to know which frame's style is being recalc'd.

But, the animation engine running is what is going to create the async slice... we'll need to figure out how create a connection in the trace (or some new implicit rule) that says that the async event has the contextid of the enclosing slice...

The naive idea I had was to make async events inherit the context of the thread that started them (at the time they were started). That wouldn't draw an arrow between them in the trace but at least the context would match.

natduca commented 8 years ago

@dstockwell fyi... do you think when we're recalcing style, we could have a separate trace_event for the work for each frame? or is recalc a big flat thing that doesn't split easily into frames?

dstoc commented 8 years ago

@nduca, I think this is already split. Do you have an example page or trace?

The style recalc event (Document::updateStyle) should strictly belong to the frame referenced by the outer UpdateLayoutTree event.

natduca commented 8 years ago

I'm just making assumptions here! What happens if you've done something that invalidated the universe from mainframe down, but then ask for computed style on something specific in the subframe? Do you see recalcs for each frame visited?

benshayden commented 7 years ago

I have no concrete plans to do this until a benchmark requires it, and then I'll probably ask the benchmark author to implement it, so unassigning, but I'll keep this open for reference.