Skip to content

fix(libSceJobManager): spin idle workers without taking the manager lock - #12085

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
JJenkx:windows/jobmgr-spin-without-lock
Oct 29, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
JJenkx:windows/jobmgr-spin-without-lock

Conversation

@JJenkx

@JJenkx JJenkx commented Oct 27, 2026

Copy link
Copy Markdown
Contributor

What

A JobManager worker that spins, whether inside its spin window or because it is persistent, unlocked and relocked the manager mutex around every _mm_pause(). With more than one spinning worker, they contend on that mutex continuously. On Windows the winpthreads mutex then parks them in the kernel, so "spinning" workers mostly sleep.

guest_job_manager_worker_count shows this on current main. With 4 workers and 2 set persistent, a local trace confirms that both workers enter the spin path. Yet over the test's 2-second window they used 125 ms and 0 ms of CPU, so the check active == persistent fails. The test fails in every run here.

Workers now pause on a workEpoch atomic instead of the mutex. WakeWorkers (every publication of work), setPersistentThreadCount and shutdown advance it. A spinner retakes the lock as soon as the epoch changes, or after 1024 pauses, so the spin-window deadline and the other conditions are still re-checked regularly. Sleeping workers and the wake-token logic are unchanged.

Tested

Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X (16 cores), WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, on main d13b3d7c55. Vulkan is hidden (VK_DRIVER_FILES pointing at a missing manifest), so guest_job_manager_profiler_gpu skips.

  • Before: guest_job_manager_worker_count fails at Require(active == persistent) in each of 4 runs on main 14332cd8d0. It was found in a full Windows CTest run of that head.
  • After: all 29 guest_job_manager_* tests pass in each of 3 runs on the head f25e58c8cc, from all five test executables. profiler_gpu skips, as above.
  • Single worker: guest_job_manager_worker_spin measures one worker's idle CPU with spin 50. It is about the same: 91.8 to 92.1 M cycles on main against 98.3 to 100.5 M with this change, 3 runs each. One spinner has no contention, so the old loop already spun there.
  • python tools/check_conventions.py --base origin/main and git diff --check pass.

Not tested: Linux and macOS, whose futex-based mutexes may have hidden the effect; game runs; a frame-time comparison. This PR claims only the worker behaviour above, not a performance change.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source: none
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

AI-assisted (Claude Code).

🤖 Generated with Claude Code

A spinning or persistent worker unlocked and relocked the manager mutex
around every pause. Two such workers contend on that mutex all the time,
and on Windows the winpthreads mutex then puts them to sleep in the
kernel: in guest_job_manager_worker_count the two persistent workers used
125 ms and 0 ms of CPU in 2 s and the check failed. Workers now pause on
a work epoch that WakeWorkers, persistence changes and shutdown advance,
and retake the lock when it changes or after a bounded spin, so spinning
reacts to new work without contending on the mutex.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JJenkx

JJenkx commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

Linux run at this PR's exact head f25e58c8cc (f25e58c8cc07da5c12e96e559d8d6d9d0c0e018e), not merged onto anything else; its base on main is d13b3d7c55.

Arch Linux, kernel 7.2.9, Ryzen 9 7950X, g++ (GCC) 16.2.1 20260810, glibc 2.44, x86-64. Release, BUILD_TESTING=ON.

Passed in each of 3 runs, exit code 0 each time, with no failed, skipped or not-run tests:

  • guest_job_manager_failures
  • guest_job_manager_registration

Only the CPU-only job manager tests were run (guest_job_manager_failures, guest_job_manager_registration); guest_job_manager_profiler_gpu and guest_job_manager_compute_progress were not, and no title or frame-time comparison was made.

These are the focused tests only; no title or console was used.

AI-assisted: yes (Claude Code).

@github-actions

Copy link
Copy Markdown

Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.

Pull request Shared files not compared
PR 10249 core/libs/prx/libSceJobManager/Export.cpp

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the current head f25e58c8cc07da5c12e96e559d8d6d9d0c0e018e.

The change keeps manager state under manager.lock and uses workEpoch only to bound how long a worker spins before rechecking that state. The Release guest_job_manager_tests target built successfully from this exact head in my isolated Linux checkout. I could not get a local runtime pass: all 29 registered guest_job_manager_* tests abort on this host because its Xeon E5-1620 v2 lacks the AVX2/BMI/LZCNT features required by the project. I therefore do not count that run as a test failure in the change. The PR reports 29 Windows test cases passing three times, and the author later supplied focused Linux failure/registration test runs.

There is an integration blocker with open PR #10249. Its current Export.cpp implementation replaces WorkerEntry's spin loop with condition-variable waiting, while this PR changes that same loop to poll workEpoch; a pairwise merge preview conflicts in Export.cpp. These are competing worker-loop behaviors, not just unrelated edits to the same file. Please coordinate with #10249's author on which behavior should land first or whether one supersedes the other, then declare the agreed order with Depends on if applicable. The overlap check also cannot compare #10249 against main while #10249 itself conflicts with the base, so this needs an explicit resolution before the stack can be integrated confidently.

I found no separate defect in the epoch signaling on this head. I’m requesting changes for the unresolved integration order, not for a demonstrated runtime regression.

@github-actions github-actions Bot added the prx label Oct 28, 2026
@babyhuey

Copy link
Copy Markdown
Contributor

Windows run of this head f25e58c8cc0, merged onto main f759e165d9c (clean merge, local only), with the selection CI's skipped Windows product job uses: Windows 11, Ryzen 7 5800X, RTX 3080, WinLibs GCC 15.2, Release, BUILD_TESTING=ON. The tree was configured fresh after the merge, then select_tests.py select / ninja -f select.ninja selected / --target libs / select_tests.py ctest --parallel 4 --timeout 120.

  • Selected 496 tests: 488 pass, 0 fail, 8 skip. No failures.

AI-assisted: yes (Claude Code)

@JJenkx

JJenkx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

On the #10249 blocker: I checked its current head, a4d01ff96f, and it doesn't change the worker loop. Its only libSceJobManager/Export.cpp hunk is in Sequence::SubmitCompute. There it loads the shader's register and specials pointers through std::uintptr_t instead of memcpy-ing straight into the pointers. Its diff has no WorkerEntry, condition variable or notify change. Perhaps an earlier head of #10249 had one, or the loop change you saw is in another PR; a link would help if so.

A pairwise git merge-tree --write-tree refs/review/pr12085 refs/review/pr10249 does conflict in Export.cpp, but only in that SubmitCompute block (and in State.hpp, which this PR doesn't touch):

<<<<<<< #12085 (= main's text here)
    const auto* bytes = static_cast<const std::uint8_t*>(item.gpu.shader);
    const ShaderRegister* registers = nullptr;
    ...
=======
    const auto* bytes = static_cast<const std::uint8_t*>(item.shader);
    std::uintptr_t registerAddress = 0;
    ...
>>>>>>> #10249

This PR doesn't modify those lines. The #12085 side is just main's current text: item.shader became item.gpu.shader after #10249's base (4997726a5f). So #10249 conflicts with main in exactly the same place, and rebasing it resolves the conflict with this PR too. The two PRs edit disjoint parts of the file, and neither needs to land first, so I've left Depends on: none. I'll also note this on #10249 so its author sees it.

AI-assisted: yes (Claude Code).

@JJenkx

JJenkx commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Fresh Windows evidence on current main. I ran a full CPU-only CTest of main f8f5b24b77 (2,840 tests, Vulkan drivers hidden) on Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Release. Its only failure is guest_job_manager_worker_count, at Require(active == persistent) (GuestJobManager.cpp:747). It also fails when run alone, in each of 3 runs.

With this PR's diff applied on that main (git apply, clean; head f25e58c8cc), all 29 guest_job_manager* tests pass in each of 3 runs. Restoring main's Export.cpp brings the failure back. So on Windows this PR is what's left between main and a fully green CPU suite here.

AI-assisted: yes (Claude Code).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Revalidated on current main 3a2b919816 (Windows 11, Ryzen 9 5950X, WinLibs GCC 15.2.0 UCRT r7, Release):

  • main alone: guest_job_manager_worker_count fails in a full ctest -j32 run and in 3 of 3 runs on its own. In every run it stops at Require(active == persistent). A temporary print shows the worker CPU over the 2-second window: with 2 set persistent, 0 1031 0 0 ms in one run and 0 0 437 0 ms in another. That is one spinning worker where two are expected. It also fails at db93735278, the commit before today's fix(libc): return the CPU count of the default thread mask from std::thread::hardware_concurrency #11785 (hardware_concurrency) merge, so that change isn't involved.
  • This head (f25e58c8cc) merged onto 3a2b919816: clean merge. All 29 guest_job_manager_* tests pass in each of 5 runs (--repeat until-fail:5); profiler_gpu skips, as before.

AI-assisted: yes (Claude Code).

@JJenkx

JJenkx commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed on current main. Full Windows suite on main edf6804cf5 (Windows 11 Enterprise 10.0.22621, WinLibs GCC 15.2.0 UCRT r7, Ninja, Release, BUILD_TESTING=ON, no Vulkan driver visible): 2 of 2980 fail, guest_job_manager_worker_count and guest_freebsd_clocks, the same two as on 3a2b919816.

With this head (f25e58c8cc) merged onto edf6804cf5 (clean merge), guest_job_manager_worker_count passes 3 of 3 (ctest --repeat until-fail:3).

AI-assisted: yes (Claude Code).

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head f25e58c8cc07da5c12e96e559d8d6d9d0c0e018e. The Windows evidence closes the requested worker-count check: guest_job_manager_worker_count passes 3/3 on a clean merge with main edf6804cf5; the author also reports all 29 guest_job_manager_* tests passing in five repeated runs on the earlier merged tree. Current-main merge preview is clean and the effective change remains the worker spin/epoch signaling edit. I checked the overlap note: #10249’s conflict with main is in its stale SubmitCompute context, not this PR’s worker-loop hunk; that PR still needs its own main rebase. No dependency on #10249 is required for this independent worker change. Hosted Windows and conventions pass; product Linux is skipped. I did not run local tests.

@bunbeans2-arch

Copy link
Copy Markdown
Contributor

Tested this on a second host, where guest_job_manager_worker_count fails on main (reported in #13026): Windows 11 Pro 10.0.22631, i9-9900K (8 cores, 16 threads), MSYS2 UCRT64 GCC 16.1.0, Release, BUILD_TESTING=ON. The tree is main 58f6cff with this PR's diff (f25e58c8cc) applied, built in the same build directory as the baseline.

  • main 58f6cff: ctest -R '^guest_job_manager_worker_count$' failed 13 of 20 runs, each at GuestJobManager.cpp:747 (Require(active == persistent)).
  • With this PR: the same command passed 20 of 20.
  • With this PR: all 29 guest_job_manager_* tests passed in each of 20 serial rounds (ctest -R '^guest_job_manager_').

This PR looks like the fix for #13026, so its Fixes: line could name it.

AI-assisted: yes (Claude Code)

@adriadam10
adriadam10 merged commit 9a9332a into boykopovar:main Oct 29, 2026
11 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Rechecked the head: the earlier change request is met (@MarcZX10 approved this exact head after the Windows guest_job_manager_worker_count evidence on current main), #10249 does not touch the worker loop, and manager state stays under manager.lock; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants