Repository navigation
fix(agc): start the shader preparation threads at the first job - #5196
Closed
nikowolf123 wants to merge 2 commits into
Closed
nikowolf123 wants to merge 2 commits into
nikowolf123 wants to merge 2 commits into
Conversation
Driver::stop calls CloseRegistrationPreparation at every shutdown, and it reached the pool through RegistrationPreparePool::Instance(). A process that never prepared a shader therefore created the pool during shutdown and started hardware_concurrency() / 2 threads (at least 2) only to close it. On Windows the exit that follows ends those threads wherever they are. When it ends one that holds winpthreads' global cond_locked spinlock, which guards the first use of a statically initialized condition variable, the Driver singleton's destructor spins forever in pthread_cond_destroy on a condition variable it never used. The process keeps a core busy and cannot be terminated, and ctest reports agc_driver_resolution_shutdown or agc_driver_wait_shutdown as timed out. The pool now starts its threads at its first submission, so closing a pool that never ran a job starts none. A submission after the pool is closed still runs on the caller's thread. agc_driver_registration_prepare_idle_shutdown checks that a shutdown without preparations leaves no thread running that it started. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t a device The Windows job builds only the execution tests named in ANYPS5_AGC_EXECUTION_TESTS, so agc_driver_registration_prepare_idle_shutdown only reported a skip there. The hang it guards against happens on Windows, and the test needs no Vulkan device, so the job now builds it next to agc_driver_registration_prepare_shutdown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
10 tasks done
Collaborator
|
Closing: no title is known to reach this. With the queue at 700+ open PRs, please hold off on new PRs that don't name a title needing the change until it's down. If a title stops on this on current main, reopen with the title ID and the log line. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
On Windows,
agc_driver_resolution_shutdownandagc_driver_wait_shutdownsometimes time out because the test process never finishes exiting. It spins on one core, andtaskkill /Fcannot end it. The Windows runners show this in run 38198064871 (agc_driver_resolution_shutdown) and run 38186720056 (agc_driver_wait_shutdown). Both reportTimeout 10.01 secafter the test had printed everything it prints when it passes:agc_driver_wait_shutdownits last line,unsatisfied label waits cancelled cleanly at shutdown, andagc_driver_resolution_shutdownonly the libc startup line.I attached gdb to stuck processes. Apart from the thread gdb adds, one thread is left, with this stack (the
libwinpthread-1.dllandlibstdc++-6.dllframes are named from the DLLs' export tables):The
Drivermember being destroyed was never waited on, so it still holds winpthreads' static initializer. For such a condition variablepthread_cond_destroytakes winpthreads' globalcond_lockedspinlock, the same lock that guards the first wait on every statically initialized condition variable. In every stuck process the spinlock was held (0) and no other thread was left to release it. Writing the unlocked value (-1) back from gdb let every stuck process finish exiting.The lock is left held because of how the process exits.
ExitProcessends every other thread wherever it is and then runs the modules' detach routines.Driver::stopcallsCloseRegistrationPreparation(), which reached the preparation pool throughRegistrationPreparePool::Instance(). In a process that never prepared a shader, this created the pool during shutdown and startedhardware_concurrency() / 2threads (at least 2) only to close it. The first of the new threads to wait on the pool's condition variable initializes it undercond_locked. If the exit ends that thread at that point, the spinlock is never released. Without these threads, no run got stuck (see Tested).This PR makes the pool start its threads at its first submission instead of in its constructor. Closing a pool that never ran a job then starts no threads. A submission after the pool is closed still runs on the caller's thread, which
agc_driver_registration_prepare_shutdownchecks. #4152 (d809e6d) kept shutdown from creating the prebuild pool by closing it only if it exists. The preparation pool now hasExisting(), but a slot alone would let a submission after shutdown start threads, so this PR starts the threads lazily instead.The new
agc_driver_registration_prepare_idle_shutdownconstructs the driver, records the process's threads, runs the libc shutdown and requires that no new thread is still running. On Windows it ignores threads whose start address is inntdll.dll. Those are Windows thread-pool workers, which Windows starts on its own (a work item submitted withTrySubmitThreadpoolCallbackruns on a thread with the same start address). The test fails onmain(Windows and Linux) and passes with this PR. The Windows job builds only the execution tests listed inANYPS5_AGC_EXECUTION_TESTS, so the second commit adds the new test to that list. Likeagc_driver_registration_prepare_shutdown, it needs no Vulkan device.docs/dev/ARCHITECTURE.mdnow says when the pool starts its threads.This PR does not change the broader hazard of detach-time destructors running after
ExitProcesshas ended the other threads; #2612 handled that for_Exit. #5007 changes the same class (Submit,Mainand the members), so whichever lands second needs a small rebase.Tested
Native Windows 11 build (WinLibs GCC 15.2.0, Release) on an RTX 5080 (NVIDIA 616.92). No title.
A harness ran each test binary repeatedly with its ctest environment, 16 runs at a time. The harness and the tests were pinned to logical CPUs 8 to 11 to load them like a 4-vCPU runner. Any run that had not exited after 15 s got
taskkill /F, and if it was still alive 5 s later, gdb. Themainbuild is 0f2683c with only the new test added.mainand this PR ran in alternating blocks of 400 runs (200 for the new test).main0f2683cagc_driver_wait_shutdownagc_driver_resolution_shutdownagc_driver_registration_prepare_idle_shutdowncond_lockedat 0, were still alive 5 s aftertaskkill /F, and exited oncecond_lockedwas set to -1. In a separate probe,taskkill /Freported for each of 11 stuck runs that the process could not be terminated, and each used 2.0 to 2.9 s of CPU time in 3 s while up to six of them shared the 4 CPUs.maingot stuck in 6 of 200 runs ofagc_driver_wait_shutdownand 8 of 200 ofagc_driver_resolution_shutdown. This PR got stuck in 0 of 200 of each.ntdll.dllthreads failed 132 of 800 runs under the same load. The test as committed passed 1600 of 1600 runs (the 800 above and 800 more).main(50 of 50 runs) and passes with this PR (200 of 200 repeats).agc_driver_wait_shutdown,agc_driver_resolution_shutdown,agc_driver_registration_prepare_shutdownandagc_driver_prepared_publicationpass.ctestof this branch (ANYPS5_REQUIRE_VULKAN=1,--parallel 4, two rounds): 1070 tests, 0 failed. Both rounds skip the same 7 tests, whichtools/check_skips.py --job windowsaccepts, andtools/check_test_registration.pypasses.ANYPS5_AGC_EXECUTION_TESTSlist buildsagc_driver_registration_prepare_idle_shutdownfrom its source instead of the skip stub.maine152ba8, andtools/check_conventions.pypasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code