Skip to content

Animated placeholder perf #50851

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.

Sign up for GitHub

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

Jump to bottom
Merged
dnfield merged 5 commits into from
Feb 18, 2020
Merged

Animated placeholder perf #50851

dnfield merged 5 commits into from
Feb 18, 2020

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Feb 15, 2020 •
edited
Loading

Benchmark for #35592 and #50842

Would be ideal to land this before #50842 to measure perf impact more clearly.

I'm running this on Android under the assumption that we have more Android devices available, and there's nothing special about this particular issue to any particular platform.

I also added the macos project to macrobenchmarks because I found it helpful when locally developing the test. The test itself is all in 0fa054a

@dnfield dnfield requested review from digiter and liyuqian February 15, 2020 03:50
@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Feb 15, 2020
@dnfield dnfield mentioned this pull request Feb 15, 2020
Closed
@dnfield dnfield force-pushed the animated_placeholder_perf branch from 0fa054a to 48609d8 Compare February 15, 2020 04:26
@dnfield
Copy link
Contributor Author

dnfield commented Feb 15, 2020

Without the change in #50842:

    "average_frame_build_time_millis": 1.4874661654135357,
    "90th_percentile_frame_build_time_millis": 3.148,
    "99th_percentile_frame_build_time_millis": 4.874,
    "worst_frame_build_time_millis": 28.459,
    "missed_frame_build_budget_count": 1,
    "average_frame_rasterizer_time_millis": 6.210055451127813,
    "90th_percentile_frame_rasterizer_time_millis": 7.408,
    "99th_percentile_frame_rasterizer_time_millis": 9.47,
    "worst_frame_rasterizer_time_millis": 13.314,
    "missed_frame_rasterizer_budget_count": 0,
    "frame_count": 1064,

With it:

    "average_frame_build_time_millis": 1.3924977426636573,
    "90th_percentile_frame_build_time_millis": 1.699,
    "99th_percentile_frame_build_time_millis": 5.492,
    "worst_frame_build_time_millis": 7.912,
    "missed_frame_build_budget_count": 0,
    "average_frame_rasterizer_time_millis": 6.566285553047407,
    "90th_percentile_frame_rasterizer_time_millis": 7.617,
    "99th_percentile_frame_rasterizer_time_millis": 11.362,
    "worst_frame_rasterizer_time_millis": 14.406,
    "missed_frame_rasterizer_budget_count": 0,
    "frame_count": 886,

Numbers from a Pixel 4 so they'll be more exaggerated in the lab.

@liyuqian
Copy link
Contributor

liyuqian commented Feb 16, 2020

I wonder why macos directory need to be added. Is it because that we need to treat Flutter for Desktop MacOS the same as Android/iOS? If so, I wonder if we can separate the addition of macos and the addition of this perf test in two PRs. Otherwise it's a little difficult to see relevant changes and revert.

@dnfield
Copy link
Contributor Author

dnfield commented Feb 16, 2020

I split the commits. We don't have to do it in one pr but I added it to make it easier to test changes locally without another device.

liyuqian
liyuqian reviewed Feb 16, 2020
View reviewed changes
dev/devicelab/manifest.yaml Outdated

animated_placeholder_perf:
description: >
Measures CPU usage of a grid of images that use a FadeInImage with an
Copy link
Contributor

@liyuqian liyuqian Feb 16, 2020

Choose a reason for hiding this comment

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

Measures CPU usage => Measures frame build and raster times as needsMeasureCpuGpu = false?

Copy link
Contributor Author

@dnfield dnfield Feb 16, 2020

Choose a reason for hiding this comment

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

Updated the langauge - I think the most important feature of this test is acutally the number of frames it pumps rather than the times. The reason I put CPU to begin with was because that stands as a proxy for CPU usage, but there's more going on for CPU usage than just building frames in the case we're trying to improve.

liyuqian
liyuqian approved these changes Feb 16, 2020
View reviewed changes
Copy link
Contributor

@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.

LGTM.

digiter
digiter approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@digiter digiter left a comment

Choose a reason for hiding this comment

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

LGTM

@dnfield dnfield merged commit e4fbb1a into flutter:master Feb 18, 2020
@dnfield dnfield deleted the animated_placeholder_perf branch February 18, 2020 19:29
jonahwilliams pushed a commit that referenced this pull request Feb 18, 2020
Revert "Animated placeholder perf (#50851)"
ce95413 
This reverts commit e4fbb1a.
@jonahwilliams jonahwilliams mentioned this pull request Feb 18, 2020
Merged
dnfield pushed a commit that referenced this pull request Feb 18, 2020
Revert "Animated placeholder perf (#50851)" (#50984)
d0b7f0c 
This reverts commit e4fbb1a.
dnfield added a commit to dnfield/flutter that referenced this pull request Feb 18, 2020
@dnfield
Revert "Revert "Animated placeholder perf (flutter#50851)" (flutter#5…
442fcb5 
…0984)"

This reverts commit d0b7f0c.
@liyuqian liyuqian mentioned this pull request Feb 18, 2020
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@digiter digiter digiter approved these changes

@liyuqian liyuqian liyuqian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dnfield @liyuqian @digiter @googlebot @fluttergithubbot