Skip to content

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented Aug 27, 2020

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.

@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Aug 27, 2020
@flutter-dashboard
Copy link

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.

@dnfield dnfield mentioned this pull request Aug 27, 2020
13 tasks
imageIndex = 0;
} else {
imageIndex += 1;
}
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

@dnfield dnfield requested a review from liyuqian August 28, 2020 17:59
@dnfield
Copy link
Contributor Author

dnfield commented Aug 28, 2020

Updated to use a DevTools memory test.

@jonahwilliams
Copy link
Contributor

Is it feasible to measure CPU usage as well?

@dnfield
Copy link
Contributor Author

dnfield commented Aug 28, 2020

@liyuqian might have more context on that.

What CPU benchmarks do you think this would capture that we're not already capturing though?

@jonahwilliams
Copy link
Contributor

specifically the cost of increased GCs to reduce image memory usage

@dnfield
Copy link
Contributor Author

dnfield commented Aug 28, 2020

Added an iOS variant that does CPU as well.

await driver.close();
});

// test('Measure CPU/GPU/Memory', () async {
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code?

Copy link
Contributor Author

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);
Copy link
Contributor

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()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

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

@liyuqian liyuqian added perf: energy Performance issues related to energy use (power, watts) perf: memory Performance issues related to memory c: performance Relates to speed or footprint issues (see "perf:" labels) labels Aug 29, 2020
@dnfield dnfield merged commit b5ed913 into flutter:master Aug 31, 2020
@dnfield dnfield deleted the image_page_benchmark branch August 31, 2020 17:04
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 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) perf: energy Performance issues related to energy use (power, watts) perf: memory Performance issues related to memory

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants