Remove unused observer and raise the waiting time #46626
Conversation
The waiting time would give our cleanup more room to finish the job, and thus to make our result more accurate. See also flutter/engine#14265
liyuqian
added
a: tests
fluttergithubbot
added
the
c: contributor-productivity
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
CC @terrylucas @cobblest : this test currently only captures the memory difference between the start and the end of the app. It would be nice to capture the max memory usage during the middle of the app (#44013), and I believe that there's more to be improved there (e.g., the IO thread should be able to abandon jobs that are no longer relevant). |
|
Agreed on @liyuqian -ideally we'd know average/median memory usage, high and low point. |
| @override | ||
| void didChangeAppLifecycleState(AppLifecycleState state) { | ||
| debugPrint('==== MEMORY BENCHMARK ==== $state ===='); | ||
| debugPrint('This was lifecycle event number $iteration in this instance'); |
There was a problem hiding this comment.
fwiw, this was useful debugging information when examining the test
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
maybe this code was copied from another test where it was more useful?
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
rather than waiting an arbitrary time, which is super flaky, can we just wait until the test is known to have finished, somehow?
There was a problem hiding this comment.
(e.g. watching the logs, like some of the other tests do?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
can't we just print from the dart code in the test?
There was a problem hiding this comment.
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.
5 participants
The waiting time would give our cleanup more room to finish the job, and
thus make our result more accurate.
See also flutter/engine#14265