-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Memory benchmark for showing large images in succession #64762
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
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| imageIndex = 0; | ||
| } else { | ||
| imageIndex += 1; | ||
| } |
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: line 33-37 can be simplified as imageIndex = (imageIndex + 1) % 6.
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.
Done
| const String kPackageName = 'com.example.macrobenchmarks'; | ||
| const String kActivityName = 'com.example.macrobenchmarks.MainActivity'; | ||
|
|
||
| class LargeImageChangerPerfTest extends MemoryTest { |
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.
I wonder if more memory info (e.g., peak memory, average, percentile, etc.) from DevToolsMemoryTest (see, e.g., this example) or Timeline (see, e.g., profiling_summarizer and flutter/engine#18516) would be helpful here? The DevTools works for Android and the Timeline works for iOS.
|
Updated to use a DevTools memory test. |
|
Is it feasible to measure CPU usage as well? |
|
@liyuqian might have more context on that. What CPU benchmarks do you think this would capture that we're not already capturing though? |
|
specifically the cost of increased GCs to reduce image memory usage |
…o image_page_benchmark
|
Added an iOS variant that does CPU as well. |
| await driver.close(); | ||
| }); | ||
|
|
||
| // test('Measure CPU/GPU/Memory', () async { |
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.
commented out code?
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.
Done
| }, onDone: () { observatoryUri.complete(null); }); | ||
| }, onDone: () { | ||
| if (!observatoryUri.isCompleted) { | ||
| observatoryUri.complete(null); |
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.
unless you need the null, this can be observatoryUri.complete()
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.
Done
jonahwilliams
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
liyuqian
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
Benchmark for #56482
This will be improved when the following finish landing:
Current behavior observed is memory growth until a GC is finally performed after multiple iterations of changing the image. Expected behavior will be that a GC is performed at least once after a large image is disposed of.