Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[tracing] SceneDisplayLag is a synchronous event #17878

Merged
iskakaushik merged 1 commit into from
Apr 23, 2020
Merged

[tracing] SceneDisplayLag is a synchronous event #17878

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions shell/common/rasterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ RasterStatus Rasterizer::DoDraw(
timing.Set(FrameTiming::kRasterFinish, raster_finish_time);
delegate_.OnFrameRasterized(timing);

std::string vsync_transitions_missed = "0";
if (raster_finish_time > frame_target_time) {
fml::TimePoint latest_frame_target_time =
delegate_.GetLatestFrameTargetTime();
Expand All @@ -297,21 +298,16 @@ RasterStatus Rasterizer::DoDraw(
}
const auto frame_lag =
(latest_frame_target_time - frame_target_time).ToMillisecondsF();
const int vsync_transitions_missed = round(frame_lag / frame_budget_millis);
fml::tracing::TraceEventAsyncComplete(
"flutter", // category
"SceneDisplayLag", // name
raster_finish_time, // begin_time
latest_frame_target_time, // end_time
Copy link
Contributor

@flar flar Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the begin/end be frame_target_time, raster_finish_time?

"frame_target_time", // arg_key_1
frame_target_time, // arg_val_1
"current_frame_target_time", // arg_key_2
latest_frame_target_time, // arg_val_2
"vsync_transitions_missed", // arg_key_3
vsync_transitions_missed // arg_val_3
);
vsync_transitions_missed =
std::to_string(round(frame_lag / frame_budget_millis));
Copy link
Contributor

@liyuqian liyuqian Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use ceiling instead of round here? Say if a frame has a lag of 0.1 frame, it seems to still cause 1 vsync transition to be missed.

Copy link
Contributor Author

@iskakaushik iskakaushik Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see both sides to the argument here. If there is 0.1 frame, there might be too much variance to do much to avoid this. It might be "too much" penalty on the app developer to charge a full frame worth of lag. That was the intent in using round.

One other alternative is to keep this as a float instead of int, we can retain say .2f precision. WDYT?

Copy link
Contributor

@liyuqian liyuqian Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... maybe I misunderstood the definition of vsync_transitions_missed as it was round before (and I'm Ok to just use round in this PR as this PR is just fixing the async events).

To better understand the definition of vsync_transitions_missed, I went back and checked the examples in #17384 (comment). That example says frame_lag = 17 (ms), missed = 2. Since a frame is roughly 16ms, the 17 (ms) frame_lag is a lag of 1.0625 frame. Then only a ceil would make it 2 instead of 1. It seems that we intended to give "too much" penalty even for a little lag above a single frame time?

}

TRACE_EVENT1("flutter", // cateogry
"SceneDisplayLag", // name
"vsync_transitions_missed", // arg1_key
vsync_transitions_missed.c_str() // arg1_val
);

// Pipeline pressure is applied from a couple of places:
// rasterizer: When there are more items as of the time of Consume.
// animator (via shell): Frame gets produces every vsync.
Expand Down