Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[metrics] Record the frame target time on the layer tree #17281

Merged
merged 1 commit into from Mar 25, 2020

Conversation

iskakaushik
Copy link
Contributor

This lets us measure stats on when the frame was
scheduled to be rendered vs when it finished rasterizing.

Note: This isn't propagated to the FrameTimings struct yet,
that is to be followed.

This lets us measure stats on when the frame was
scheduled to be rendered vs when it finished rasterizing.

Note: This isn't propagated to the FrameTimings struct yet,
that is to be followed.
@iskakaushik iskakaushik requested a review from flar March 24, 2020 00:08
@auto-assign auto-assign bot requested a review from cbracken March 24, 2020 00:08
@iskakaushik iskakaushik removed the request for review from cbracken March 24, 2020 00:09
@@ -25,7 +25,8 @@ Animator::Animator(Delegate& delegate,
: delegate_(delegate),
task_runners_(std::move(task_runners)),
waiter_(std::move(waiter)),
last_begin_frame_time_(),
last_frame_begin_time_(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've also renamed this from last_begin_frame_time_ to last_frame_begin_time_, I think this reads better.

@flar
Copy link
Contributor

flar commented Mar 24, 2020

I'm not sure how compatible we can be with this, but it would be better if the agent that created the layer_tree passed this information in the constructor. Right now we only pass the target time up and then it is used to compute a tree, but it is not passed down.

@iskakaushik
Copy link
Contributor Author

I've looked at passing it down when constructing the later tree. One of the ways I think we can do it would be to pass it through SceneBuilder. Given that this requires a change in dart:ui which typically requires more complex rolls for our downstream dependents, I was hesitant to do this during the current WFH situation that we are in.

@flar
Copy link
Contributor

flar commented Mar 24, 2020

I'm not sure what the complications are for downstream dependents. Is this a case of them using our API and needing to use the new method signature? Or do some of them implement our dart:ui interfaces and they'd have to update their alternate implementations to match?

I'm still looking at the chain of method calls, but can default constructor values be used to maintain backwards compatibility? It could also be an optional "after construction" call which leaves the properties with default values if they don't call it.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM

@iskakaushik iskakaushik merged commit f1d8026 into flutter:master Mar 25, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2020
@liyuqian liyuqian added perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues. labels Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes perf: speed Performance issues related to (mostly rendering) speed severe: performance Relates to speed or footprint issues.
Projects
None yet
5 participants