Remove redundant trace events #32812
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| } | ||
|
|
||
| void Engine::BeginFrame(fml::TimePoint frame_time, uint64_t frame_number) { | ||
| TRACE_EVENT0("flutter", "Engine::BeginFrame"); |
There was a problem hiding this comment.
Can you add comments to these deletions to explain why they are okay? Just looking at the PR it's hard for me to know if this is acceptable or not.
There was a problem hiding this comment.
Sure.
This one is redundant with Animator::BeginFrame. It adds no additional information and never really shows anything valuable.
| picture_metrics_ = {}; | ||
| layer_metrics_ = {}; | ||
| { | ||
| TRACE_EVENT0("flutter", "RasterCache::SweepCaches"); |
There was a problem hiding this comment.
This is almost always very fast and I've never used it in tracing. @flar can you comment on whehter you think this is worth keeping?
| } | ||
|
|
||
| void MessageLoopImpl::FlushTasks(FlushType type) { | ||
| TRACE_EVENT0("fml", "MessageLoop::FlushTasks"); |
There was a problem hiding this comment.
This is FML's only direct usage of tracing, and there is always at least one interesting event beneath it. Maybe we could guard this behind a flag, but it would be good to get rid of FML's reliance on the Dart VM here and instead have more descriptive events.
| total_count.c_str()); | ||
| #else | ||
| TRACE_EVENT0("flutter", "VolatilePathTracker::OnFrame"); | ||
| #endif |
There was a problem hiding this comment.
There is a separate benchmark for the volatility tracker. This information was useful to me when first implementing, but I have never used it since and it's a pretty obscure implementation detail to be surfacing in the traces.
Same comment below.
| if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) { | ||
| TRACE_EVENT1("flutter", "RuntimeController::DispatchPlatformMessage", | ||
| "mode", "basic"); | ||
| TRACE_EVENT0("flutter", "RuntimeController::DispatchPlatformMessage"); |
There was a problem hiding this comment.
mode: basic is no longer used.
| if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) { | ||
| TRACE_EVENT1("flutter", "RuntimeController::DispatchPointerDataPacket", | ||
| "mode", "basic"); | ||
| TRACE_EVENT0("flutter", "RuntimeController::DispatchPointerDataPacket"); |
There was a problem hiding this comment.
mode: basic no longer used.
| TRACE_EVENT2("flutter", "Framework Workload", "mode", "basic", "frame", | ||
| FrameParity()); |
There was a problem hiding this comment.
This (and FrameParity) were used by a no longer functioning view in the observatory. See https://github.com/dart-lang/sdk/blob/main/runtime/observatory_2/web/timeline.js#L158 and related lines in there, which also refer to now-deleted timeline events.
There was a problem hiding this comment.
Also, this is redundant with Animator::BeginFrame, which has the frame number in it and is easily detected as even or odd.
| } | ||
|
|
||
| { | ||
| TRACE_EVENT0("flutter", "PipelineConsume"); |
There was a problem hiding this comment.
Duplicated with GPURasterizer::Draw
| RasterStatus Rasterizer::DrawToSurface( | ||
| FrameTimingsRecorder& frame_timings_recorder, | ||
| flutter::LayerTree& layer_tree) { | ||
| TRACE_EVENT0("flutter", "Rasterizer::DrawToSurface"); |
There was a problem hiding this comment.
GPURasterizer::Draw covers this more fully.
| RasterStatus Rasterizer::DrawToSurfaceUnsafe( | ||
| FrameTimingsRecorder& frame_timings_recorder, | ||
| flutter::LayerTree& layer_tree) { | ||
| TRACE_EVENT0("flutter", "Rasterizer::DrawToSurfaceUnsafe"); |
There was a problem hiding this comment.
GPURasterizer::Draw covers this more fully.
| FireNextFrameCallbackIfPresent(); | ||
|
|
||
| if (surface_->GetContext()) { | ||
| TRACE_EVENT0("flutter", "PerformDeferredSkiaCleanup"); |
There was a problem hiding this comment.
IME this event never shows anything interesting. We should instead just use skia tracing when needed to find out what's going on here.
|
@akbiggs instead of me for flutter reviews moving forward :) For this review, a grep in fuchsia's trace_processing library shows that none of the removed tags are being used, so I think you're good to go re: fuchsia |
|
LGTM from DevTools perspective. The two event names we care about are "Animator::BeginFrame" and "GPURasterizer::Draw". |
|
Looks good thanks Dan. I did a search of all the event names in the Fuchsia source tree and didn't find any references to them. |
6 participants
Removes a number of tracing events that are redundant.
Removes all
"mode", "basic"tracing information, which is related to flutter/flutter#101382.Removes the one instance of using tracing from FML, which should make it easier to detangle FML from the Dart VM and resolve issues like flutter/flutter#101866
/cc @arbreng - can you help verify that Fuchsia does not depend on these events for performance tracing?
/cc @kenzieschmoll FYI
/cc @zanderso fyi
I think this makes more sense to request a test exemption for (deleting code) rather than trying to write tests that assert we don't add these back. I anticipate this will make small positive differences in benchmarks upstream.