Add FrameTiming delay to watchPerformance #64780
Conversation
It allows (1) old FrameTimings to be flushed and not interfering our measurements here, and (2) new FrameTimings to be all reported. Fixes flutter#64778
flutter-dashboard
bot
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 on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| final TimingsCallback watcher = frameTimings.addAll; | ||
| binding.addTimingsCallback(watcher); | ||
| await action(); | ||
| await delayForFrameTimings; |
There was a problem hiding this comment.
It looks like we only need one of L43 and L44?
And there's also a copy of watchPerformance in macrobenchmarks.
2s is not a very short time (vs ~10s sampling time), maybe we should also increase the timeout settings in macrobenchmarks.
There was a problem hiding this comment.
Good catch. I forgot to delete L44 during refactor.
| // Delay for a sufficient time so either old FrameTimings are flushed and not | ||
| // interfering our measurements here, or new FrameTimings are all reported. | ||
| final Future<void> delayForFrameTimings = | ||
| Future<void>.delayed(const Duration(seconds: 2)); |
There was a problem hiding this comment.
This sounds potentially flaky. Is there any way to improve coordination with the engine to avoid magic delays like this?
There was a problem hiding this comment.
Yes, it's flaky in terms of benchmark numbers (still have a chance to be bimodal and measuring the wrong thing), but not in terms of success/failure. It will be a net gain compared to our old version as the old ones have a higher chance of measuring the wrong thing.
We shall definitely have a more proper fix to solve this reliably. Created #64808 to track that.
| final TimingsCallback watcher = frameTimings.addAll; | ||
| binding.addTimingsCallback(watcher); | ||
| await action(); | ||
| await delayForFrameTimings; // make sure all FrameTimings are reported |
There was a problem hiding this comment.
If the intention here is to wait for another 2 seconds, then I think you will need a fresh instance of Future.
5 participants
It allows (1) old FrameTimings to be flushed and not interfering our
measurements here, and (2) new FrameTimings to be all reported.
Fixes #64778