-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Measure iOS CPU/GPU percentage #39439
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
Conversation
digiter
left a comment
There was a problem hiding this 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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
dnfield
left a comment
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
dev/devicelab/manifest.yaml
Outdated
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
57774b0 to
82c6aa7
Compare
82c6aa7 to
634d0dd
Compare
|
@dnfield @goderbauer @digiter : ready to review again. |
dnfield
left a comment
There was a problem hiding this 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!
digiter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This reverts commit 652be88. This fix is in flutter/packages#37
This reverts commit 652be88. This fix is in flutter/packages#37
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.
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
This reverts commit 652be88. This fix is in flutter/packages#37
For #33899
Test added:
Test modified:
We'll add the CPU/GPU measurement to more iOS tests
once it's proven to be non-flaky.