Skip to content

Conversation

@liyuqian
Copy link
Contributor

For #33899

Test added:

  • simple_animation_perf_ios

Test modified:

  • backdrop_filter_perf_ios__timeline_summary

We'll add the CPU/GPU measurement to more iOS tests
once it's proven to be non-flaky.

@liyuqian liyuqian requested review from digiter, dnfield and flar August 28, 2019 21:24
@fluttergithubbot fluttergithubbot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Aug 28, 2019
@liyuqian liyuqian added the c: performance Relates to speed or footprint issues (see "perf:" labels) label Aug 28, 2019
@liyuqian liyuqian requested review from goderbauer and yjbanov August 28, 2019 21:25
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.

Where will the result show up? As new graphs on the performance dashboard?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why this cannot be part of flutter/flutter/dev/devicelab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there will be two new graphs (one for CPU, one for GPU) per test. I didn't directly put them under flutter/flutter/dev/devicelab because (1) I'm not sure the implication of licenses (of using another MIT licensed project); (2) the code there might not be held to the same standard as flutter repo so I can do some quick and hacky jobs; (3) the tool might be useful outside of Flutter.

(1) is the major reason and I started a discussion in the general discord chat room. I'm happy to move my code to flutter repo if licenses aren't an issue.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

We should come up with a better home for the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we upstream this to flutter/packages, and probably publish it on pub? Then we could do a pub global activate with a specific version. As written, it seems likely to possibly break if you push new changes - it's also in a repo that none of the rest of the team can work on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I'll definitely move the code there if license isn't a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the param name feels strange to me, maybe just measureCpuAndGpuUsage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

measureCpuAndGpuUsage was my original name and then I realized a name conflict with measureCpuAndGpuUsage method...

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment why this was marked as flaky (e.g. because its new and unproven).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed flaky.

For flutter#33899

Test added:
- simple_animation_perf_ios

Test modified:
- backdrop_filter_perf_ios__timeline_summary

We'll add the CPU/GPU measurement to more iOS tests
once it's proven to be non-flaky.
@liyuqian
Copy link
Contributor Author

@dnfield @goderbauer @digiter : ready to review again.

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for doing the work to move the package where it belongs - this is much cleaner now!

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

@liyuqian liyuqian merged commit af9f424 into flutter:master Sep 16, 2019
liyuqian added a commit that referenced this pull request Sep 16, 2019
liyuqian added a commit that referenced this pull request Sep 16, 2019
This reverts commit af9f424.

Reverts #39439

Reason: this passed the local device lab test on my Macbook but it failed in the actual device lab. Will investigate, fix, and reland.

TBR: @dnfield @goderbauer @tvolkert @Hixie
liyuqian added a commit to liyuqian/flutter that referenced this pull request Sep 24, 2019
liyuqian added a commit that referenced this pull request Sep 26, 2019
jonahwilliams pushed a commit that referenced this pull request Sep 26, 2019
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
For flutter#33899

Test added:
- simple_animation_perf_ios

Test modified:
- backdrop_filter_perf_ios__timeline_summary

We'll add the CPU/GPU measurement to more iOS tests
once it's proven to be non-flaky.
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
This reverts commit af9f424.

Reverts flutter#39439

Reason: this passed the local device lab test on my Macbook but it failed in the actual device lab. Will investigate, fix, and reland.

TBR: @dnfield @goderbauer @tvolkert @Hixie
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. c: performance Relates to speed or footprint issues (see "perf:" labels)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants