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

Conversation

@liyuqian
Copy link
Contributor

@liyuqian liyuqian commented Jan 10, 2019

This change follows our trend to move all our performance metrics to frame time instead of FPS. Computing FPS from max ms/frame is misleading and we're no longer just using 60 FPS displays.

Tested on iOS devices. Android devices currently don't show any text because of flutter/flutter#26387

We will add a gold test to the framework for performance overlay to both test this change and prevent regressions like flutter/flutter#26387

Here's a screenshot of the new performance overlay:
image

Computing FPS from max ms/frame is misleading and we're no longer
just using 60 FPS displays.
@Hixie
Copy link
Contributor

Hixie commented Jan 10, 2019

Can you post a screenshot of what it looks like?


// DEPRECATED
// The frame per second FPS could be different than 60 (e.g., 120).
static const double kOneFrameMS = 1e3 / 60.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

are there consumers that still exist that use this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used for drawing 3 horizontal lines: 16ms, 32ms, 48ms (https://github.com/flutter/engine/blob/master/flow/instrumentation.cc#L92)

Eventually, we should remove this altogether and get the refresh rate from #7002

I feel that it's better to do it in another PR to keep each one small and focused.

<< "ms/frame";
stream << label_prefix << " "
<< "max " << max_ms_per_frame << " ms/frame, "
<< "avg " << average_ms_per_frame << " ms/frame";
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm a little conflicted about showing the average...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are often a few big spikes to make the max very different from the average (e.g., max is 300ms while the average is 11ms). One way of thinking is that average is more about the power consumption while max is more about janks.

@Hixie
Copy link
Contributor

Hixie commented Jan 10, 2019

test?

Copy link
Contributor Author

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

@Hixie : regarding tests, I'll add a golden test to the framework to both guard this feature and other performance overlay regression (e.g., the missing text issue mentioned in this PR description).


// DEPRECATED
// The frame per second FPS could be different than 60 (e.g., 120).
static const double kOneFrameMS = 1e3 / 60.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used for drawing 3 horizontal lines: 16ms, 32ms, 48ms (https://github.com/flutter/engine/blob/master/flow/instrumentation.cc#L92)

Eventually, we should remove this altogether and get the refresh rate from #7002

I feel that it's better to do it in another PR to keep each one small and focused.

<< "ms/frame";
stream << label_prefix << " "
<< "max " << max_ms_per_frame << " ms/frame, "
<< "avg " << average_ms_per_frame << " ms/frame";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are often a few big spikes to make the max very different from the average (e.g., max is 300ms while the average is 11ms). One way of thinking is that average is more about the power consumption while max is more about janks.

@liyuqian
Copy link
Contributor Author

See the edited PR description for a screenshot.

@liyuqian
Copy link
Contributor Author

Ping...

@liyuqian liyuqian merged commit 5ce5ace into flutter:master Jan 14, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 15, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants