[tracing] SceneDisplayLag is a synchronous event #17878
Conversation
|
Context: I'm running into multiple issues with the way the benchmarks treat async events, see: flutter/flutter#54629 (comment). Also for now, I'm only relying on |
This change converts it from an events that spans a time interval to an event that occurs at an instant. We also emit this trace event when there is no lag as opposed to only when there was a lag to make it monotonous.
iskakaushik
force-pushed
the
scene-lag-sync-event
branch
from
26647ae to
5887587
Compare
liyuqian
left a comment
liyuqian
left a comment
There was a problem hiding this comment.
In general, I think it would be nice to have unit tests for our tracing events. Not sure how much work it needs so it might be out of scope for this PR to fix flutter/flutter#54629 quickly. Just bring this up to solicit more discussions from folks.
| vsync_transitions_missed // arg_val_3 | ||
| ); | ||
| vsync_transitions_missed = | ||
| std::to_string(round(frame_lag / frame_budget_millis)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| vsync_transitions_missed // arg_val_3 | ||
| ); | ||
| vsync_transitions_missed = | ||
| std::to_string(round(frame_lag / frame_budget_millis)); |
There was a problem hiding this comment.
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?
|
@liyuqian , I'm still debating what I want the behavior to be. I will see how some benchmarks play out and I will revise this metric as needed. You're right though, the original intent seems more inline with |
This change converts it from an events that spans a time interval to an event that occurs at an instant. We also emit this trace event when there is no lag as opposed to only when there was a lag to make it monotonous. Co-authored-by: Kaushik Iska <kaushikiska@google.com>
This change converts it from an events that spans a time interval to an event that occurs at an instant. We also emit this trace event when there is no lag as opposed to only when there was a lag to make it monotonous. Co-authored-by: Kaushik Iska <kaushikiska@google.com>
This change converts it from an events that spans a time interval to an event that occurs at an instant. We also emit this trace event when there is no lag as opposed to only when there was a lag to make it monotonous. Co-authored-by: Kaushik Iska <kaushikiska@google.com>
This change converts it from an events that spans a time interval to an event that occurs at an instant. We also emit this trace event when there is no lag as opposed to only when there was a lag to make it monotonous. Co-authored-by: Kaushik Iska <kaushikiska@google.com>
| "flutter", // category | ||
| "SceneDisplayLag", // name | ||
| raster_finish_time, // begin_time | ||
| latest_frame_target_time, // end_time |
There was a problem hiding this comment.
Shouldn't the begin/end be frame_target_time, raster_finish_time?
4 participants
This change converts it from an events that spans a time interval
to an event that occurs at an instant.
We also emit this trace event when there is no lag as opposed to
only when there was a lag to make it monotonous.
fixes: flutter/flutter#54629