Skip to content

Remove unused observer and raise the waiting time #46626

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.

Sign up for GitHub

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

Merged
fluttergithubbot merged 1 commit into from
Dec 9, 2019
Merged

Remove unused observer and raise the waiting time #46626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 0 additions & 11 deletions dev/benchmarks/macrobenchmarks/test_memory/large_images.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,9 @@ Future<void> endOfAnimation() async {
} while (SchedulerBinding.instance.hasScheduledFrame);
}

int iteration = 0;

class LifecycleObserver extends WidgetsBindingObserver {
@override
void didChangeAppLifecycleState(AppLifecycleState state) {
debugPrint('==== MEMORY BENCHMARK ==== $state ====');
debugPrint('This was lifecycle event number $iteration in this instance');
Copy link
Contributor

@Hixie Hixie Dec 10, 2019

Choose a reason for hiding this comment

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

fwiw, this was useful debugging information when examining the test

Copy link
Contributor Author

@liyuqian liyuqian Dec 10, 2019

Choose a reason for hiding this comment

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

afaik, this life cycle state is only printed when one try to switch between multiple apps, or go back to the home screen. In this specific memory test, we're not doing any of that so in my test this function ends up never been called.

Copy link
Contributor

@Hixie Hixie Dec 10, 2019

Choose a reason for hiding this comment

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

maybe this code was copied from another test where it was more useful?

Copy link
Contributor Author

@liyuqian liyuqian Dec 10, 2019

Choose a reason for hiding this comment

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

Correct. I copied them from flutter_gallery__back_button_memory test, and later found that the READY message is the only one that I need.

}
}

Future<void> main() async {
runApp(const MacrobenchmarksApp(initialRoute: kLargeImagesRouteName));
await endOfAnimation();
await Future<void>.delayed(const Duration(milliseconds: 50));
debugPrint('==== MEMORY BENCHMARK ==== READY ====');
WidgetsBinding.instance.addObserver(LifecycleObserver());
}
2 changes: 1 addition & 1 deletion dev/devicelab/bin/tasks/fast_scroll_large_images__memory.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class FastScrollLargeImagesMemoryTest extends MemoryTest {
await launchApp();
await recordStart();
await device.shellExec('input', <String>['swipe', '0 1500 0 0 50']);
await Future<void>.delayed(const Duration(milliseconds: 10000));
await Future<void>.delayed(const Duration(milliseconds: 15000));
Copy link
Contributor

@Hixie Hixie Dec 10, 2019

Choose a reason for hiding this comment

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

rather than waiting an arbitrary time, which is super flaky, can we just wait until the test is known to have finished, somehow?

Copy link
Contributor

@Hixie Hixie Dec 10, 2019

Choose a reason for hiding this comment

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

(e.g. watching the logs, like some of the other tests do?)

Copy link
Contributor Author

@liyuqian liyuqian Dec 10, 2019

Choose a reason for hiding this comment

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

Agree that waiting for some actual signal would be much better. Although this would rely on some (to-be-added) signals/logs being generated by the engine in the IO thread, so this would require some engine changes first. There's an additional difficulty that we should wait for the last cleanup signal, but it's unclear to me how to determine whether a signal is the final one as the engine could generate arbitrary number of such signals at arbitrary times. CC @dnfield and @chinmaygarde for more thoughts on how to provide a deterministic signal from the engine.

Copy link
Contributor

@Hixie Hixie Dec 10, 2019

Choose a reason for hiding this comment

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

can't we just print from the dart code in the test?

Copy link
Contributor Author

@liyuqian liyuqian Dec 10, 2019

Choose a reason for hiding this comment

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

When the Dart code released the image, the engine IO thread and Skia would still hold the graphics memory. The actually release won't happen until a Drain task is executed, which could be delayed by up to 3 seconds, or even more if there are some slow IO thread tasks. The test only makes sense to measure the memory when the last engine IO thread Drain task actually finishes. That's why printing something from the Dart code isn't an option here.

await recordEnd();
}
}
Expand Down