Skip to content

Animated placeholder perf - #50851

Merged
dnfield merged 5 commits into
flutter:masterfrom
dnfield:animated_placeholder_perf
Feb 18, 2020
Merged

Animated placeholder perf#50851
dnfield merged 5 commits into
flutter:masterfrom
dnfield:animated_placeholder_perf

Conversation

@dnfield

@dnfield dnfield commented Feb 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

dnfield commented Feb 15, 2020

Copy link
Copy Markdown
Contributor Author

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

liyuqian commented Feb 16, 2020

Copy link
Copy Markdown
Contributor

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

dnfield commented Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

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
Comment thread dev/devicelab/manifest.yaml Outdated

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

@liyuqian liyuqian Feb 16, 2020

Copy link
Copy Markdown
Contributor

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?

@dnfield dnfield Feb 16, 2020

Copy link
Copy Markdown
Contributor Author

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

@liyuqian liyuqian left a comment

Copy link
Copy Markdown
Contributor

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

@digiter digiter left a comment

Copy link
Copy Markdown
Contributor

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
Jonah Williams
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
Jonah Williams
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