fix: delay exiting microbenchmark #176477
Conversation
Give the collecting process a chance to catch Done
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue where the microbenchmark process might exit before the collecting process has a chance to read the 'Done' message from stdout. It introduces an await stdout.flush() call followed by a 5-second delay before exiting. While flushing stdout is correct, the hardcoded 5-second delay is long and brittle. My feedback suggests reducing this delay significantly to avoid slowing down benchmark runs, or making it configurable if a long delay is unavoidable.
| // Now we're just being paranoid here and letting the process churn through | ||
| // log lines before handling the exit code. | ||
| await Future<void>.delayed(const Duration(seconds: 5)); |
There was a problem hiding this comment.
While adding a delay can help prevent race conditions where the parent process doesn't see the 'Done' message before this process exits, a hardcoded 5-second delay is quite long and will slow down every benchmark run. This also feels like a magic number and might be brittle if the collecting process is under heavy load.1
A much shorter delay, e.g., const Duration(milliseconds: 100), would likely be sufficient to allow the OS to schedule the reading process without introducing a significant overhead. If a longer delay is truly necessary, consider making it configurable via an environment variable with a comment explaining the reasoning.
The existing comment is also a bit informal and could be more descriptive.2
| // Now we're just being paranoid here and letting the process churn through | |
| // log lines before handling the exit code. | |
| await Future<void>.delayed(const Duration(seconds: 5)); | |
| // A short delay to give the collecting process a chance to read the | |
| // "Done" message from stdout before this process exits. | |
| await Future<void>.delayed(const Duration(milliseconds: 100)); |
Style Guide References
Footnotes
-
The style guide advises to 'Write what you need and no more'. A 5-second delay seems excessive and adds unnecessary overhead to every benchmark run. (link) ↩
-
The style guide emphasizes optimizing for readability. The comment '// Now we're just being paranoid here...' is informal and could be replaced with a more descriptive explanation of the delay's purpose. (link) ↩
There was a problem hiding this comment.
A 5 second delay is not excessive on a benchmark run that takes 4 or 5 minutes to run. 100ms might not be enough given that the data is heading over a stream and we don't know if the monitoring process is still processing something already.
But, 100ms is probably enough? It's just that why pin our hopes on probably when the time taken here is not going to impact anything in practice...
There was a problem hiding this comment.
All the benchmarks are batched together, which is also why it takes 5 minutes to run. Bad gem gem. But also good gem gem for calling out my magic number. I don't care.
jtmcdole
added
the
autosubmit
flutter-dashboard
bot
removed
the
autosubmit
flutter/flutter@5c0c9e9...908012d 2025-10-05 [email protected] Roll Skia from 5479115ef5bf to 1fd0ca1f2120 (1 revision) (flutter/flutter#176541) 2025-10-05 [email protected] Roll Fuchsia Linux SDK from oWcBvgdpdlGvaqiDg... to Zm6K_3gP3VCaMy9rH... (flutter/flutter#176538) 2025-10-05 [email protected] Roll Dart SDK from 53aeaeb2454c to 016a8c0045fd (1 revision) (flutter/flutter#176531) 2025-10-04 [email protected] Roll Skia from f316de3d47b4 to 5479115ef5bf (4 revisions) (flutter/flutter#176529) 2025-10-04 [email protected] Roll Dart SDK from 9bc52df78b67 to 53aeaeb2454c (1 revision) (flutter/flutter#176525) 2025-10-04 [email protected] Roll Fuchsia Linux SDK from HUhTcRn-LUXa2Salu... to oWcBvgdpdlGvaqiDg... (flutter/flutter#176515) 2025-10-04 [email protected] Fix TextFormField does not inherit local InputDecorationTheme (flutter/flutter#176397) 2025-10-04 [email protected] Roll Dart SDK from 0009748aed50 to 9bc52df78b67 (4 revisions) (flutter/flutter#176506) 2025-10-04 [email protected] Roll Skia from 9cda1a2050c4 to f316de3d47b4 (2 revisions) (flutter/flutter#176504) 2025-10-04 [email protected] fix: support older git (ubuntu 22.04) in content hash (flutter/flutter#176321) 2025-10-04 [email protected] Roll Skia from a454242c3934 to 9cda1a2050c4 (2 revisions) (flutter/flutter#176499) 2025-10-03 [email protected] [material/menu_anchor.dart] Check for reserved padding updates on layout delegate. (flutter/flutter#176457) 2025-10-03 [email protected] Roll Skia from b842026480e0 to a454242c3934 (3 revisions) (flutter/flutter#176484) 2025-10-03 [email protected] Starts updating the DEPS in preupload. (flutter/flutter#176485) 2025-10-03 [email protected] Align flutter dependencies with ones coming from dart. (flutter/flutter#176475) 2025-10-03 [email protected] fix: delay exiting microbenchmark (flutter/flutter#176477) 2025-10-03 [email protected] Add state restoration for UIScene migration (flutter/flutter#176305) 2025-10-03 [email protected] Fix Voiceover traversal for OutlinedButton.icon (flutter/flutter#175810) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Give the collecting process a chance to catch Done
Give the collecting process a chance to catch Done
Give the collecting process a chance to catch Done
2 participants
Give the collecting process a chance to catch Done