fix(vk): RX_VK_JOB_STEAL=true no longer turns off a draw's pipeline build where it waits #233
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!233
Loading…
Reference in a new issue
No description provided.
Delete branch "TrashPanda/Longshot:fix/pipeline-claim"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
A draw that has to wait for its pipeline should never stand behind the whole build queue: on Mako an RX 6600 froze 118 s on one draw behind 26 queued jobs. Longshot already avoids that by running the queued build on the waiting thread. But its switch was read with
atoi, soRX_VK_JOB_STEAL=true(oron,yes) quietly turned the protection off. There was also no log line to show it working.Ported from Mako, by sharkiix,
github.com/Shakoquo/Mako@0d93fb1972("a draw that must wait for its pipeline builds it itself when no worker has taken it").What changed
Mako's commit adds
JobClaim,jobs_push_claimandjobs_claim_run. Longshot has had the same behaviour sinceaafbaf5d:jobs_waittakes the job pushed with the flag it waits on out of the queue and runs it on the waiting thread.graphics_pipeline's build (mesh programs included) and both translation jobs are pushed with their flags. A second, claim-based mechanism for the same wait would duplicate it, so this PR keeps the steal and closes what was left:jobs:
jobs_waitingpu/vk/vk_jobs.cppreadsRX_VK_JOB_STEALbylongshot_env.h's on/off rule (env_value_off): only0,false,offandnoturn it off. It now returns whether it ran the job on this thread. The job is still taken out of the queue under the lock workers take jobs under, so it runs exactly once and no leftover is queued for a worker; this is the equivalent of Mako'stakenflag.draw: the
wait_forlambda indraw()(gpu/vk/vk_draw.cpp) logs the first 10 times a pipeline build or a translation ran where it waited, with how many other jobs were queued or running and how long it took. This is Mako'sa draw's pipeline built where it waitedline.docs:
RX_VK_JOB_STEALadded todocs/development.md's switch table; it was not documented.tests:
tests/test_jobs_wait_runs_queuednow checks:RX_VK_JOB_STEAL=trueand" 1 "keep the steal on, while0,offandNoturn it off (the job then runs on a worker);queue.erase,env_value_offanddraw()'s pipeline wait going throughjobs_wait.test_draw_side_effectsandtest_jobs_failurefollow the new signature and include path.Checked and left as they are: compute pipelines (
dispatch()) and the mesh fallback's compute pipelines (fallback_pipelines_ready). These are not waited for behind the queue: they are pushed urgent, and the dispatch or draw waits 50 ms and is then left out. Running them on the recording thread would bring back the 15-23 s AMD stalls those paths were moved off it for.Flags and settings
RX_VK_JOB_STEAL0/false/off/no: a draw waits for a worker to reach its build, behind the queue, as beforeaafbaf5d. Before this PR any valueatoiread as 0 (true,on,yes) also turned it offMeasurements
None possible: there is no game dump on this PC. The change in behaviour is the switch's reading and a log line; the steal itself is unchanged since
aafbaf5d. The motivating report is Mako's (sharkiix): on an RX 6600, one draw waited 118 s with "26 jobs queued".main:
fdc2840b(2026-11-13)this PR: fix/pipeline-claim @
ab7b4950Hardware: RTX 4070 Ti, NVIDIA 595.99.02, Linux (game-free checks only)
Scene: none (no game)
Method:
tests/test_jobs_wait_runs_queued.pyandpython -I -B tests/run_all.py --jobs 3, each at its commitHow to reproduce:
python -I -B tests/test_jobs_wait_runs_queued.py; on main, its own test binary run withRX_VK_JOB_STEAL=truehangsfdc2840bab7b4950RX_VK_JOB_STEAL=true, every worker busy: the waited buildtests/run_all.pyTesting
Tested:
python -I -B tests/run_all.py --jobs 3on main @fdc2840band this PR @ab7b4950(Linux, RTX 4070 Ti, 595.99.02): 503 passed, 3 failed, 18 skipped on both. The three failures also fail on main, in the same way:test_display_pointer(undefinedwin::rx_option),test_texture_staging_publication(undeclaredlog_debug) andtest_lift_cache.test_jobs_wait_runs_queued,test_jobs_failure,test_draw_side_effects,test_ub_threads) pass. The new test fails on main.vk_jobs.cppandvk_draw.cppcompile with the back end's flags (clang++ -fsyntax-only, nx2/build.py's include folders).Not tested: Not run in the game: no game dump on this PC. Not built for Windows (llvm-mingw). The change is portable C++ with no platform calls, but it is not compiled there.
Needs other testers:
vk: a draw's pipeline build ran where it waitedlines and no multi-second freezes withjobs queued. Expect the same frame pacing as main (request to open).Screenshots
Nothing on screen.
Risk and rollback
Low. The steal's behaviour is unchanged unless the switch was set to something like
true, which now keeps it on as the name says. The log line is capped at 10.RX_VK_JOB_STEAL=0turns the steal off; reverting the commit is enough.Reviewer notes
Start with
jobs_waitingpu/vk/vk_jobs.cpp, thenwait_forindraw(). Mako'sJobClaimwas not ported on purpose: Longshot's flag-based steal takes the job out of the queue, so no no-op copy is left behind for a worker and the run-once guarantee comes from the queue's lock. #234 (quick pipelines, Mako 8292d363) is stacked on this one.Issues
See Mako's report in 0d93fb19 (an RX 6600, one draw 118 s behind 26 queued jobs).
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.tests/that reads changed source updated in this PR, not weakened.Read the diff,
jobs_wait/jobs_push/workerinvk_jobs.cpp, everyjobs_waitcaller (draw()'swait_for,dispatch(),program_wait), the jobs pushed with a flag (graphics_pipeline, both translation jobs) and the compute and fallback builds. I did not build or run anything.What's good
env_value_offturns the steal off for0,false,off,noand an empty value. The oldatoiread also turned it off for an empty value and for0, so=0and an empty value mean the same as before.true,onandyes(atoi 0, so off before) now keep the steal on, which is the point of the PR. The only other values that change are odd ones like00,nordisable, which are on now. Nothing in the repo (docs, scripts, presets) sets the switch, so no shipped config changes meaning.mu, andjobs_waiterases undermubefore it unlocks, so a job a worker has started can't be taken again. Each job runs exactly once and itsstd::functionis moved out once, so nothing is built or freed twice. A second waiter on the same flag finds no entry and sleeps ondone_cv, and the steal path'sjobs_notify()wakes it. The job runs withmureleased, so workers and nestedjobs_pushcalls (the cold GPL libraries) aren't blocked.draw()holds noParLockorshader_muat eitherwait_for.On "is the freeze fixed at every wait site"
In Longshot's default the 118 s case could not happen before this PR either: the steal has been on when the switch is unset since
aafbaf5d. This PR fixes it only for someone who setRX_VK_JOB_STEALto a word. The wait sites:jobs_waitand steal. WithRX_VK_TRANSLATE_WAIT_MSset,jobs_wait_fordoesn't steal, but it is bounded and then leaves the draw out.program_pick, sodispatch()'sjobs_wait(cp->ready)rarely waits.programs_load_wait) have their own CAS claim, separate from this switch.The longest wait left is one build a worker has already started (one pipeline, not a queue), which is the design. Precompile before the title doesn't record draws, so it can't hold a draw.
Must change
Should change
RX_VK_JOB_STEAL=<word>and adds the log line. The in-game check this PR needs is only that theran where it waitedline appears.(RX_VK_JOB_STEAL=0: waited in turn)suffix reads as if the switch were set, and the count isn't the queue it would have waited behind.dispatch()'sjobs_wait(cp->ready)ignores the new return value and isn't logged. That's fine given compute translation is synchronous; just say so if it's on purpose.fdc2840band main is at1ef93e79(two commits, #54 and #78). They touch none of these files, so there's no conflict, but rebase or untick the box.Not checked: Mako's licence. As far as I can tell no Mako code was copied, only the idea and a log wording, and Mako's commit is credited. Not built for Windows.
@ -216,6 +216,7 @@ Pipeline-cache file ownership, temporary storage and failure behavior are descri| `RX_VK_SKIP_PROG=<prefix>,...` | Leaves out the draws of these programs (default `53509044401d78d2`; `-` leaves out none) || `RX_VK_PIPE_SKIP=0`, `1` or `2` | Default/`1`: wait for shader translation (with `RX_VK_TRANSLATE_WAIT_MS` set, a draw whose targets hold an earlier frame waits at most that long a frame in all, then is left out). A pending pipeline can be skipped only for attachment-only draws whose targets hold an earlier frame. `0`: wait for every translation and pipeline. `2`: restore the old skip for untranslated draws with persistent targets at once; this can omit writes before the shader is known. Translated memory-writing draws still wait || `RX_VK_TRANSLATE_WAIT_MS=<ms>` | Off by default: each shader still translating is waited out, as #1573 does. Set (`8` is the tested value): how long, in all, each recording thread waits in a frame for shaders still translating before leaving their draws on earlier frames' targets out. For first-visit pauses of 0.2-3 s on a cold or changed program bank; a translation slower than the bound can lose a memory write, since its writes are not known before it is translated. A negative value is off; anything but a number is ignored with a warning; the value in use is logged. Compute dispatches still wait their translation out || `RX_VK_JOB_STEAL=0` | A draw that must wait for its shader translation or pipeline build waits for a worker to reach it, behind every job queued before it, as before. By default a job it waits for that no worker has started yet is taken out of the queue and run on the waiting thread (it runs once either way; one a worker has started is waited for). The log's `vk: a draw's ... ran where it waited` lines (the first 10) say when it happened. Off follows the on/off rule above (`0`, `false`, `off`, `no`) |Nit: an empty value (
RX_VK_JOB_STEAL=) also turns it off underenv_value_off, as it did underatoi. The general rule at line 108 covers that, but the list in brackets here leaves it out.@ -6840,3 +6840,3 @@PROF_EXACT();const auto t0 = std::chrono::steady_clock::now();jobs_wait(ready);const int queued = jobs_pending();jobs_pending()counts every job queued or running, including the ones queued after this one, not the jobs this wait would have stood behind. The comment on 6848 ("the queue the wait would have stood behind") and the log's wording overstate it a little. Either say "jobs pending", or havejobs_waitreport the job's position inqueuewhen it erases it, which is the number that explains a freeze.@ -6845,0 +6847,4 @@static std::atomic<int> told{0};if (here && told++ < 10) // (longshot: that the steal ran, with the queue the wait would have stood behind)logf("vk: a draw's %s ran where it waited, ahead of the queue (%d other jobs queued or running), in %.1f ms ""(RX_VK_JOB_STEAL=0: waited in turn)",The suffix
(RX_VK_JOB_STEAL=0: waited in turn)is printed on every steal and reads as if the switch were set to 0. Something like(with RX_VK_JOB_STEAL=0 it would have waited in turn)says what is meant.WIP: fix(vk): RX_VK_JOB_STEAL=true no longer turns off a draw's pipeline build where it waitsto fix(vk): RX_VK_JOB_STEAL=true no longer turns off a draw's pipeline build where it waits