Remove expensive trace events#36989
Conversation
| void DisplayList::Dispatch(Dispatcher& dispatcher, | ||
| uint8_t* ptr, | ||
| uint8_t* end) const { | ||
| TRACE_EVENT0("flutter", "DisplayList::Dispatch"); |
There was a problem hiding this comment.
Let's delete this and the one on DisplayListDispatcher::EndRecordingAsPicture in their own PR.
fml/trace_event.h
Outdated
| // overhead can easily dwarf the actual raster time. For these, we use | ||
| // a special debug mode only trace macro. | ||
| #if (FLUTTER_RUNTIME_MODE_DEBUG) | ||
| #define DEBUG_TRACE_EVENT0(category_group, name) \ |
There was a problem hiding this comment.
It'd be nice if there was some way to turn these on or off via a shell argument, kind of like how we have --trace-skia to enable extra skia tracing today.
Having these in a trace can be helpful to understand scene complexity, but maybe there are better ways we can tool that (like @iskakaushik 's work around rasterizing leaves and timing that).
There was a problem hiding this comment.
The argument to turn this kind of thing on is --profile.
There was a problem hiding this comment.
Leaving these on by default in profile mode is likely artificially slowing down benchmarks, especially on iOS where tracing is more expensive.
I'm suggesting treating these more like how we treat Skia's helpful but too-verbose tracing in profile mode.
There was a problem hiding this comment.
We could even leave them on by default, but we need to provide a mechanism to turn them off so our benchmarks are representative
|
Trying to summarize a lot of different discussion threads:
I think that the best short term course of action is to remove these trace events. Worst comes to worst, we can add them back. But most of what is being traced here are very trivial operations that do not actually have much overhead at all (at least, not without the trace events running). We already have tools for folks to debug what is in the layer tree. |
jonahwilliams
requested review from
Hixie,
dnfield,
kenzieschmoll and
zanderso
| canvas_(save_mode == SaveMode::kInternalNodesCanvas | ||
| ? *(paint_context.internal_nodes_canvas) | ||
| : *(paint_context.leaf_nodes_canvas)) { | ||
| TRACE_EVENT0("flutter", "Canvas::saveLayer"); |
There was a problem hiding this comment.
removing the saveLayer events will break DevTools. We have logic to count these events and provide a hint to users when we detect this expensive operation in their janky frame.
There was a problem hiding this comment.
So I can certainly leave these in the tree, but I don't think relying on this is correct. You are only going to see saveLayers that are composited by flutter, you will miss all of the saveLayers that are performed directly on cavnas. That could easily be the majority of cases
There was a problem hiding this comment.
Okay gotcha. This was the event I was told to check for at the time these were added - @dnfield what is the best way to catch all the save layer events from flutter and from the canvas?
There was a problem hiding this comment.
Its not going to be possible without instrumenting the display list. that said, given that these trace events are slow we should find a different way to record this information.
There was a problem hiding this comment.
saveLayer opeations on canvas should not happen very often. Finding a different way to instrument this is fine, but we should make sure to not break devtools in the mean time.
There was a problem hiding this comment.
Thanks to this discussion I realized that I had not supported these events in the new LayerStateStack stuff.
Added back in: 7632996
| context.internal_nodes_canvas->clipPath(path_, true); | ||
| break; | ||
| case Clip::antiAliasWithSaveLayer: { | ||
| TRACE_EVENT0("flutter", "Canvas::saveLayer"); |
jonahwilliams
added
the
autosubmit
|
The overrides in all of the clip layers are now obsolete. They existed solely to send the event. I'll update this in the LayersStateStack PR. (Please do not create another PR to address this as I'll have to merge with it) |
6 participants
On applications that have a lot of composited layers, the cost of trace events can easily dwarf the actual raster time. See flutter/flutter#113879 (comment)
This patch attempts to work around this by 1) removing trace events that provide low value or are called two frequently (display list) and 2) moving preroll/paint trace events to debug only.