Repository navigation
fix(windows): use precise waits in sub-millisecond polling loops - #1551
Conversation
|
Rebased directly onto |
874a70f to
6795498
Compare
6795498 to
f51ea2c
Compare
f51ea2c to
0999ed5
Compare
|
Rebased this PR onto current upstream |
|
The current-head Build, Conventions and Progress report runs were created as |
0999ed5 to
0812c0b
Compare
|
Rebased onto current upstream |
|
The Build/Conventions workflow runs for current head |
0812c0b to
b439aa2
Compare
|
The fork Build on the previous head found a Windows link failure in |
b439aa2 to
1f8a2cc
Compare
|
The second Windows failure on head |
1f8a2cc to
adc6a7e
Compare
|
The re-scoped test-only head is |
adc6a7e to
87b2633
Compare
|
The test-only PR is now rebased onto current |
87b2633 to
ccf07c9
Compare
|
Rebased the isolated test-only change onto current upstream |
822c179 to
46c8f5a
Compare
|
Rebased PR #1551 linearly onto current |
f0a4b00 to
d6c7e2f
Compare
|
I expanded this PR to include the four sub-millisecond polling call-site changes from #1576 and credited Shaan as co-author. On exact head |
|
Windows run: Windows 11 Pro 25H2 (10.0.26200.9457), RTX 5060 Ti (NVIDIA 617.42), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release,
AI-assisted: yes |
|
Thanks for the Windows report: it covers the exact PR head |
d6c7e2f to
a62e5f4
Compare
|
Rebased the existing PR linearly onto current |
a62e5f4 to
46a77cc
Compare
Validation on the updated headI rebased the existing branch linearly onto upstream The matching production call-site work from #1576 is integrated here with Shaan credited as co-author. #1551 remains the fuller candidate because it includes the same production changes plus the helper regression test. The required upstream Build and relinker matrix is still pending for this head. |
46a77cc to
e777d26
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the Windows evidence and the helper test. One blocker:
- The four production changes (
Pm4.cpp:816,Apr.cpp:589,Aio.cpp:256/:281,State.cpp:60) are the same edits as the open #1576, which CONTRIBUTING doesn't allow (no open PR implementing the same change). Co-authoring the commit doesn't change that: two PRs can't both land them.- Fix: drop the call-site changes from this PR and keep only
core/libs/tests/PreciseWait.cpp+ its CMake registration (the scope it had before 2026-10-09), with #1576 in Depends on if you want the test to cover it. Or, if you and the author of #1576 prefer this one to carry the change, agree that there and close the other.
- Fix: drop the call-site changes from this PR and keep only
- Minor:
precise_waitpasses onmainwithout any of these changes (it only exercises the helper), so it won't catch a call site reverting tosleep_for. That's fine as a helper test, just don't claim it covers the call sites.
Checked: Linux build, precise_wait, agc_driver_pm4, guest_aio, guest_font* pass on this head.
e777d26 to
0bb0b7c
Compare
|
Thanks for the review. I agree that the two PRs cannot both land the same production changes, and that the helper test does not prove the call sites use the helper. I rebased #1551 onto current I have asked Shaan on #1576 to confirm one carrier. My preference is #1551 because it preserves the four production changes with Shaan credited and includes the helper regression test. If Shaan prefers #1576, I will remove the production changes from #1551. I will not ask for both to merge; the overlap remains unresolved until Shaan replies. |
0bb0b7c to
6c240d5
Compare
Follow-up to the review
The current GitHub Build has passed eligibility and Ubuntu register-context jobs. Its Linux and Windows full-build jobs are skipped by the fork eligibility gate; macOS and relinker jobs are still running. The PR description records these limits. Please re-review this updated scope when convenient. |
Current-head CI resultBuild run 38034401344 completed successfully on head
The local focused Linux CTests pass 5/5 as recorded in the description. The full CI build matrix did not run on this fork PR; Windows reports in the body are from earlier heads. Please re-review this updated scope and CI result when convenient. |
Co-authored-by: Shaan <shaansahai2007@gmail.com>
6c240d5 to
54a9018
Compare
|
Rebased #1551 onto upstream The maintainer closed #1576 as a duplicate of this PR, so the three production call sites remain consolidated here with Shaan credited as co-author. The current Build and Conventions runs are queued (Build, Conventions); full Linux and Windows jobs are skipped by the fork eligibility gate. Please re-review when the current checks finish. |
Follow-up on the review blockerThe duplicate has now been resolved: maintainer adriadam10 closed #1576 as a duplicate of #1551, noting that this PR is the single carrier for the shared call-site changes and includes the helper regression test. Shaan remains credited as co-author. The current posted head is
The PR description also clarifies that Could you revisit the |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the three call sites (Pm4.cpp WAIT_REG_MEM, Apr.cpp counter/address wait, State.cpp font backoff) against the existing PreciseSleepUs helper on main; they build and precise_wait passes on main + this PR (Linux). Windows evidence from @rmz-24 (full ctest 511/511) and @popcoder565's measurements in #1471 cover the behaviour we can't run here. Note the helper test only pins the minimum delay, not the call sites. Merging.
What
Replace sub-millisecond
std::this_thread::sleep_forpolling withPreciseSleepUs()in three host-side loops. On Windows, winpthreads truncates thesenanosleeprequests to whole milliseconds, so requests below 1 ms return immediately and spin. This addresses the Windows busy-spin behavior documented in #1471.WAIT_REG_MEM: 50 µsKeep the focused
precise_waitCTest for the helper.Relationship to #1576
The production call-site changes originated in #1576 and are retained here with Shaan credited as co-author. The maintainer closed #1576 as a duplicate of this PR on 2026-10-10, identifying #1551 as the single carrier. This PR also includes the focused
PreciseSleepUshelper regression test.The AIO substitutions were removed after Windows measurements showed that the AIO wait loops return on their first check and the change had no measurable benefit. The same report found each precise wait takes about 0.5 ms instead of the requested 10–50 µs, so no AIO performance claim is made.
Overlap audit
The automated overlap notice for #1360 is only the insertion point in
core/libs/prx/libc/CMakeLists.txt. #1360 registers its mapping-history test; this PR registers the precise-wait helper test. They add separate targets and test behavior, with no production-file overlap or dependency.Tested
Current PR head is
54a90185147ea49d1913bccfed53fb477e619b9f, rebased linearly onto upstreammainat6cc33583326dd86c35f14710b984d767405d203e. The five-file diff contains three production call sites, the helper test, and its CMake registration.libc,libSceAgcDriver,libSceFont,libkernel,precise_wait_tests,agc_driver_pm4_tests,guest_aio_tests,guest_font_tests, andguest_font_writing_line_tests.precise_wait,agc_driver_pm4,guest_aio,guest_font,guest_font_writing_line; 0.08 s total).python3 tools/check_conventions.py --base upstream/mainandgit diff --check upstream/main...HEADpass locally.sleep_for(10/50/200 µs)returning immediately andPreciseSleepUstaking about 0.50 ms; see the report. An independent Windows 11 Pro 25H2 report on earlier headd6c7e2f6, cherry-picked ontomaind70b8998, used RTX 5060 Ti / NVIDIA 617.42, MinGW-w64 GCC 15.2.0, Release and a full build pluslibs:precise_waitpassed and full CTest passed (511/511) (report). These Windows runs predate this rebase.sleep_for.Checklist
main; one topic: use the precise wait helper at sub-millisecond host polling sites and test the helper