[CP-stable][win32] Fix overflow in TaskRunnerWindow.#182864
Conversation
Fixes flutter#182501 Right now empty `TaskRunner::ProcessTasks()` returns `TaskTimePoint::max() - now` (relative time). This causes problems, because the value is later used in `TaskRunnerWindow` to schedule timer at `std::chrono::high_resolution_clock::now() + relative_time`). This of course overflows, because the new `now()` in `TaskRunnerWindow` is higher than the value used to calculate original delta inside `TaskRunner`. `TaskRunnerWindow` already had special case where it checks for `std::chrono::nanoseconds::max()`. In this PR the behavior of `TaskRunnerWindow::SetTimer` has been updated to check for overflow rather than excepcting a special value. *If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].* ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
@loic-sharma please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
github-actions
bot
added
engine
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a potential overflow issue in the Windows task runner. The changes involve modifying task_runner.cc to handle empty task queues more safely and updating task_runner_window.cc to prevent overflow when scheduling timers with large delay values. A corresponding unit test is also added to verify the fix for the empty task queue scenario. My review includes a suggestion to improve the robustness and portability of the fix in task_runner_window.cc.
| auto now = std::chrono::high_resolution_clock::now(); | ||
| auto remaining_to_max = | ||
| std::chrono::nanoseconds::max() - now.time_since_epoch(); | ||
| when = std::min(when, remaining_to_max); | ||
| timer_thread_.ScheduleAt(now + when); |
There was a problem hiding this comment.
While this fix prevents the overflow, its correctness relies on implementation-defined behavior of std::chrono::high_resolution_clock. The calculation std::chrono::nanoseconds::max() - now.time_since_epoch() assumes that now.time_since_epoch() can be safely converted to nanoseconds and that std::chrono::nanoseconds::max() is a reasonable proxy for the maximum duration of high_resolution_clock. This might not be true on all platforms and could lead to precision issues.
A more portable and robust approach is to directly use std::chrono::high_resolution_clock::time_point::max() to determine the scheduling limit. This avoids assumptions about clock implementation details and makes the intent clearer.
Consider this alternative implementation:
auto now = std::chrono::high_resolution_clock::now();
auto max_time = std::chrono::high_resolution_clock::time_point::max();
if (when > max_time - now) {
timer_thread_.ScheduleAt(max_time);
} else {
timer_thread_.ScheduleAt(now + when);
}
There was a problem hiding this comment.
@knopp This seems like a reasonable follow-up improvement, but since you verified this works as expected on Windows I'm fine skipping this for the cherrypick.
|
I have rerun the failing test. If this can get an approval and have passing tests I can land this today midday otherwise it will be in next weeks release. |
|
Hey @knopp are you able to give this a lgtm as another domain expert? |
|
@reidbaker FYI, @knopp created the pull request, I'm the reviewer. (I suspect wires got crossed since I requested the cherry-pick 😄). |
reidbaker
added
the
autosubmit
|
Yeah I didnt catch that Autosubmit added |
auto-submit
bot
merged commit 802fc7d
into
flutter:flutter-3.41-candidate.0
Only 2 non test cherry picks for stable since 3.41.2 https://github.com/flutter/flutter/pulls?q=is%3Apr+base%3Aflutter-3.41-candidate.0+is%3Aclosed #182864 #182691
4 participants
Issue Link:
#182501
Impact Description:
Flutter Windows apps have high CPU even when idle.
Changelog Description:
flutter/182501 Reduce CPU utilization of idle Flutter Windows apps
Workaround:
None
Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
Run the app on a Windows device and check CPU utilization. This issue reproduces especially on low-end devices, or, devices with high CPU utilization.