feat(vk): quick pipelines for draws whose full build is not done #234
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!234
Loading…
Reference in a new issue
No description provided.
Delete branch "TrashPanda/Longshot:feat/quick-pipelines"
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
While a worker is still building a draw's pipeline, Longshot either leaves the draw out or waits for the build. On AMD's proprietary driver the optimizer took 5-13 s per pipeline (Mako's report, by sharkiix): Volvagia's intro showed the boss as a black silhouette, and draws that had to wait froze it. This PR lets such draws use a pipeline from the caches, or a quick one built without the optimizer, until the full build is done. It is off by default on every driver (
RX_VK_QUICK_MSunset) until someone measures it on AMD.Ported from Mako, by sharkiix,
github.com/Shakoquo/Mako@8292d36346("quick pipelines for draws whose full build is not done (boss cutscenes no longer black)").Commits (base
main@2e1e2e8c; #233 is merged, so nothing is stacked any more):9cf8b511, the author's: quick pipelines and the brief wait.4a629554, HumanBean's (review 1124): a busy driver-fault gate is retried, it is off by default everywhere with 8 ms suggested and a real budget,RX_VK_PIPE_SKIP=0makes no quick pipelines, a failed full build that leaves the quick pipeline is a deliberate choice and logged, andst_pipelinesno longer counts quick builds.d49cd033(review 1136): a quick build that met a busy gate no longer uses up the frame's one build. A scope guard keeps a throw from leaving the thread flags ore.quickset. A draw that must wait polls for 5 s by the clock. The docs say how far the retry reaches.What changed
draw()(gpu/vk/vk_draw.cpp), a draw whose pipeline isn't ready and that has nothing to draw with callsquick_pipelinefirst.may_skip: attachment-only, on earlier frames' targets) is held to the budget. Without a quick pipeline it gets the brief wait (RX_VK_PIPE_WAIT_MS, off unless set) and is then left out.RX_VK_PIPE_SKIP=0(always_wait) a draw waits for the full pipeline as on main, and no quick one is made.RX_VK_TRANSLATE_WAIT_MS). Mako's brief wait applied to translations too; here it applies only to pipelines.quick_pipeline(vk_draw.cpp), with its rules ingpu/vk/quick_pipe.h(take,brief,FrameBudget):pipeline_from_cachesdoes (ask_start_first, only where the driver haspipelineCreationCacheControl). If that misses, it builds one at once without the driver's optimizer.job->e->pipeline = full).kNoneand a later draw tries again. A quick pipeline that loses the race to the full build is destroyed, since no draw has seen it.kOtherQuickWaitMs), and stops once that attempt is over.quick_build,ask_start_firstand the test'sslow_full_buildare set by a scope guard (quick_pipe::Raise), andtakeleaves the entrykTriedif the ask or build throws.run_guardedkeeps a worker running after its job throws. Without the guard, a throw would leave the flag set for that thread's later builds, and every must-wait draw of the entry would poll the whole 5 s.build_graphics, throughthread_local bool quick_build:VK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BITand builds into no pipeline cache. It doesn't count towardspcache_built_since_save(an unoptimized pipeline in the saved cache would answer a later run's full build) or inst_pipelines.RX_VK_NOOPT, AMD's task and mesh rule, the AMD Windows cloth pipeline), and for stage libraries (part >= 0).gate_busy, and the entry is tried again later.jobs_hurry(flag)(vk_jobs.cpp) moves a queued job to the front of the queue. The brief wait uses it and then waits (jobs_wait_for) within its budget. It doesn't run the build on the recording thread, as Mako did: once started, a build can't be stopped, and AMD's took seconds.RX_VK_TEST_SLOW_MS(Mako's test switch) holds each worker's full build inside the driver-fault gate, so a quick build meets the busy gate as it would on AMD. Off AMD, addRX_VK_DRIVER_FAULTS=2to make the gate serial.quick_pipe::read_ms) and logged once when on, and are documented indocs/development.md.New
tests/test_quick_pipelines. With fake handles, it checksquick_pipe.h:kTried;It also cuts
vk_draw.cpp's ownquick_budget_ms,pipe_wait_budget_msandquick_pipelineout and builds them against stand-ins. That covers the caches, the flags, a busy gate at either step, a throw out ofbuild_graphicsthat clears the flags, andRX_VK_QUICK_MSoff by default on AMD and NVIDIA. Finally it checks source rules forbuild_graphics, the job anddraw().test_draw_side_effects: the pipeline's quick path and brief wait, a translation still waited out, and no quick path underRX_VK_PIPE_SKIP=0.test_jobs_wait_runs_queued:jobs_hurry.test_driver_faults,test_gpl_library_cacheandtest_fallback_pipeline_statefollowquick_build/gate_busy/slow_full_build. Their checks are kept.Flags and settings
RX_VK_QUICK_MS=<ms>0or unset turns it off; anything that isn't a number is ignored with a warning8to try it (half a 60 fps frame, asRX_VK_TRANSLATE_WAIT_MS). On AMD, where a quick build takes tens of ms, 8 in practice means about one quick build a frame on each recording thread, each a hitch of tens of ms. Turn it on by default for AMD's proprietary driver in a follow-up with numbersRX_VK_PIPE_WAIT_MS=<ms>100(Mako's default) where draws flicker in for a frame or two on first sightRX_VK_TEST_SLOW_MS=<ms>RX_VK_DRIVER_FAULTS=2so the gate is serial as on AMDMeasurements
None in the game: there is no game dump on this PC, and no AMD card with the proprietary driver. The motivating report is Mako's (sharkiix, 8292d363): AMD's optimizer took 5-13 s per pipeline in Volvagia's intro, which drew as a black silhouette. With quick pipelines the boss cutscenes were no longer black, and draws that had to wait no longer froze loading.
main:
2e1e2e8c(this PR's base; main is nowfbd42024, see Testing)this PR: feat/quick-pipelines @
d49cd033Hardware: RTX 4070 Ti, NVIDIA 595.99.02, Linux (game-free checks only)
Scene: none (no game)
Method:
python -I -B tests/run_all.py --jobs 4at each commit, and the touched tests one by oneHow to reproduce (in the game, for a tester): see Testing.
2e1e2e8cd49cd033tests/test_quick_pipelines.pyquick_pipe.h)tests/run_all.py --jobs 4Testing
Tested:
python -I -B tests/run_all.py --jobs 4atd49cd033and at main @2e1e2e8c(Linux, RTX 4070 Ti, 595.99.02): 499 / 498 passed, 30 skipped each. The one failure,test_lift_cache("the lift's scripts import"), fails the same way on main. The skips need Windows, a Vulkan 1.3 device for readback, numpy or the SPIR-V headers, none of which is set up here.fbd42024, #198, which touchesvk_draw.cpp). It merges cleanly, and on the mergetest_quick_pipelines,test_draw_side_effects,test_driver_faults,test_gpl_library_cache,test_jobs_wait_runs_queuedandtest_fallback_pipeline_statepass.d49cd033. With 4a629554's counting (a build that met the busy gate counted as started), the busy-build budget case fails. Without thee.quickguard, both throw cases fail. Withask_start_firstset by hand, the cut-code throw case fails. A 16x longer poll bound fails the clock case. Before4a629554, a quick build sent intopcacheorquick_buildset during the cache ask each made the test fail.Not tested:
Needs other testers (open, but they can't be claimed until a maintainer adds the
testing wantedlabel):RX_VK_QUICK_MS=8and without it, against main. Expectvk: quick pipeline (no optimizer) for ... in X mslines (report the times and the worst frame time), fewer draws left out, and no longer freezes than before. Watch for the known limit (see Risk): a boss whose programs are already building still draws as a silhouette.RX_VK_QUICK_MS=8 RX_VK_TEST_SLOW_MS=8000 RX_VK_DRIVER_FAULTS=2into a new area, and the same withoutRX_VK_QUICK_MS. Expect quick pipeline lines, nothing missing for long while full builds are held, and no 8 s freezes, except for a draw whose own program is already building (that one waits, as on main).RX_VK_PIPE_WAIT_MS=100against unset and main, in a new area. Expect fewer draws flickering in late, and frame-time spikes of at most about 100 ms.gl_Positionis decoratedInvariant(mma_layout.h), which should cover this across optimisation levels. On NVIDIA it isn't decorated, so T-HE7V is where this would show.Screenshots
This change should show on screen (the AMD boss intro), but there is no game here to take them. A tester's before and after from the same spot is wanted.
Risk and rollback
RX_VK_QUICK_MS=8on AMD, a recording thread can make about one quick build a frame (tens of ms). Draws that must wait get one whatever the budget, as before they waited for the whole full build.RX_VK_QUICK_MSorRX_VK_PIPE_WAIT_MSis set (RX_VK_PIPE_SKIP=0is as on main). Unsetting them, or reverting the three commits, is enough.Reviewer notes
Start with
quick_pipe.h(FrameBudget::fits/started,take,Raise), thenquick_pipelineand thequick_build/gate_busylines inbuild_graphics, then the pipeline block indraw()and the job ingraphics_pipeline. Deliberate differences from Mako:Issues
None open; motivated by Mako's report (sharkiix, 8292d363). Builds on #233 (merged).
Checklist
For everyone
type(scope): description).main. (Based on2e1e2e8c. Main is one commit ahead,fbd42024(#198); it merges cleanly and the touched tests pass on the merge.)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.Reviewed
471cb339only (ab7b4950, PR 233, taken as the base). Read-only; no build or tests run.What a quick pipeline is here: the same
build_graphicscall with the same state, shaders, specialisation and dynamic state, plusVK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BIT, built into no cache. Before that it asks the caches for the full pipeline without compiling. It is not a GPL fast-link and not a fallback shader, so no specialisation constant or dynamic state is skipped. Good parts:pcacheand out ofpcache_built_since_saveis right.jobs_hurrywith a bounded wait is better than Mako's synchronous build.Must change
set_serialis on for every AMD card (vendorID 0x1002). The cache ask and the quick build both usebegin(p.key, false), so either getskBusywhile any build of the same program is in the driver. That includes the entry's own full build, whichgraphics_pipelinequeued a moment earlier and which an idle worker may already have taken.takethen storeskTried, so that entry never gets a quick pipeline again. Its skippable draws stay out for the full 5-13 s, and its must draws stall as before. This is the scenario the PR is for. Leave the entry atkNone(retry next frame) when the build answeredkBusy, and keepkTriedfor "made" and for "won't ever be made" (NOOPT, part, faulted). Also,RX_VK_TEST_SLOW_MSsleeps beforebuild_graphicsand so outside the gate, and the gate is serial only on AMD. The NVIDIA/RADV test plan therefore never exercises this path. Hold the sleep inside the gate, or ask testers forRX_VK_DRIVER_FAULTS=2as well.pr ready, or ship it opt-in (RX_VK_QUICK_MSunset = 0 everywhere) and flip it in a follow-up with the numbers. Also, "Rebased on currentmain" is ticked, but the base isfdc2840band main is now1ef93e79. Nothing conflicts (those two commits touch none of these files), so either rebase or untick the box.Should change
takechecks the budget before a build, so the last build always runs past it. Say why 1000 (what Mako ships) and measure the worst hitch on AMD, or start lower.RX_VK_PIPE_SKIP=0no longer means "wait for every pipeline".always_waitdraws aremust, and they now draw with a quick pipeline whenRX_VK_QUICK_MSis on. Either skip the quick path underalways_wait, or update the switch's row.pipelinesis never cleared. On a cold AMD cache that's most new pipelines, as unoptimised code. Log a running count, or retire them once the frames that might hold them are done.SKIP_HERE). That's probably what you want, but it also covers a program the driver later faults on (the "left out" log line is then wrong). State it in the description, and make it a deliberate choice.gl_Positionis decoratedInvariant(mma_layout.h), which should cover this across optimisation levels, but it's off on NVIDIA, where testers are asked to run withRX_VK_QUICK_MS=1000. Add "geometry with a depth pre-pass draws whole, no z-fighting or holes" to the testers' checks.st_pipelinesandst_pipeline_usnow count quick builds too, so the stats line mixes the two.kOtherQuickPollsis 5000 ×sleep_for(1ms), which is about 15 ms a sleep on Windows without a raised timer resolution; waiting ondone_cvorquick.waitwould be tighter.Not checked: game behaviour on any GPU, the Windows build, and the PR's test results (not run, by the brief).
@ -0,0 +11,4 @@namespace quick_pipe {/// RX_VK_QUICK_MS unset: this budget on AMD's proprietary driver, where the slow optimizer was measured; none elsewhereconstexpr double kQuickDefaultMs = 1000;1000 ms a frame on each recording thread is effectively no budget for draws the policy already accepts leaving out: it trades a missing draw for a hitch of up to a second (more, since the budget is checked before a build, not after). This is also the new AMD default, and nothing has been measured. Say where 1000 comes from (Mako's value?), and measure the worst frame on AMD, or start lower / opt-in.
@ -0,0 +73,4 @@for (int i = 0; must && i < kOtherQuickPolls && e.quick.load(std::memory_order_acquire) == kMaking && !there() &&!e.ready.load(std::memory_order_acquire);i++)poll();Nit:
sleep_for(1ms)is about 15.6 ms on Windows unless the timer resolution has been raised, so a must draw polls more coarsely than the comment says. Waiting one.quick(C++20atomic::wait) ordone_cvwould wake on the change.@ -0,0 +88,4 @@} else {got = there() ? Got::kOther : Got::kNothing;}e.quick.store(kTried, std::memory_order_release);This also stores
kTriedwhen the build returned nothing only because the driver-fault gate was busy (kBusy). On AMD the gate is serial on every card, and the entry's own full build, queued a moment earlier, is often already in the driver. That uses up the entry's only try for good: its skippable draws stay out for the full 5-13 s, and its must draws stall. KeepkNone(retry next frame) for a busy gate, and keepkTriedfor made or never-makeable.@ -2252,2 +2264,4 @@}();if (slow_ms > 0) std::this_thread::sleep_for(std::chrono::milliseconds(slow_ms));use_counters.made_how = 0;if (const VkPipeline full = build_graphics(*job->p, job->s, job->at, job->topo)) job->e->pipeline = full;When this build fails (null), the quick pipeline stays as
e->pipelineand the entry is marked ready, so the draw uses the unoptimised pipeline for the session where before it wasSKIP_HERE. That includes a program the driver faults on. Probably fine, but state it, and the "left out" log line is then wrong. The replaced quick pipeline is also never destroyed, so it's one extra pipeline per entry for the session: worth a counter in the log at least.@ -2249,3 +2252,3 @@job->predicted = predicted;jobs_push([job] {// (over a linked one, which is left alone: draws recorded with it may still be on their way)// (over a linked one or a quick one, which are left alone: draws recorded with them may still be on their way)RX_VK_TEST_SLOW_MSsleeps here, outside the driver-fault gate, and the gate is serial only on AMD. So the suggested NVIDIA/RADV test never has a quick build meet a full build in progress, which is the AMD case. Hold the sleep insidebuild_graphicsafterbegin(full builds only), or have testers addRX_VK_DRIVER_FAULTS=2.@ -2488,3 +2507,2 @@if (gated) {const auto gate = fault_quarantine().begin(p.key, !cold);if (gate == driver_faults::Quarantine::kBusy) return *cold = true, VK_NULL_HANDLE;const auto gate = fault_quarantine().begin(p.key, !cold && !quick_build);With
wait=falsethis answerskBusywhile any pipeline ofp.keyis in the driver, not only this entry's full build. In a new area on AMD (serial gate, 5-13 s builds), many quick attempts will get nothing here, and quick_pipe.h:91 then marks the entry tried. The cache ask a few lines up is gated the same way.@ -6924,0 +7024,4 @@// (longshot, from Mako) one to draw with until the full build is done: from the caches, or quick (quick_pipeline). Else a// draw that may be left out first waits briefly for its build, moved to the queue's front (RX_VK_PIPE_WAIT_MS, off unless// set), and one that may not waits for it, running it here if no worker has (jobs_wait)if (!pe->pipeline.load(std::memory_order_acquire) && !quick_pipeline(*pe, p, s, at, topo, !may_skip)) {!may_skipincludesalways_wait(RX_VK_PIPE_SKIP=0). With quick pipelines on, those draws now draw with a quick pipeline instead of waiting for every pipeline, as the switch's row in docs/development.md still says they do. Skip the quick path underalways_wait, or update the row.471cb339294698784874Re-review at
46987848(9cf8b511, the author's, plus46987848, the maintainer's fix). The base is main@origin2e1e2e8c, which is current main. #233 is merged asd5fc892f. I only read the code: no build, no tests, nothing run in the game.Earlier review (1124, at
471cb339)build_graphicssetsgate_busyonkBusy, andtakeleaves the entry atkNone.kTriedstays for "made", for "lost the CAS", and for "won't ever be made" (NOOPT,part >= 0,kFaulted, a failed create). I went through every branch and they all hold.RX_VK_TEST_SLOW_MSslept outside the gatebegin, for the worker's build only, and the row says to addRX_VK_DRIVER_FAULTS=2off AMD.quick_msno longer takes the driver.main" ticked but falsefitshas a bug (Must change 1).RX_VK_PIPE_SKIP=0drew with quick pipelinesalways_waitgoes straight towait_for(pe->ready), as on main, and test_draw_side_effects checks that no quick pipeline is asked for.st_pipelinesmixed quick and full buildssleep_for(1ms)pollWhat's good
beginper draw, after the setup inbuild_graphics. It can't spin. A draw that must wait and polls stops as soon as the other attempt goes back tokNone, and then waits for the full build as on main.gate_busycan't carry a stale value into a quick decision. Both lambdas clear it right before theirbuild_graphicscall and read it right after, andbuild_graphicsdoesn't call itself.kBusyregex,st_pipelines,always_wait. The stubs in test_fallback_pipeline_state (gate_busy,slow_full_build,test_slow_ms,<thread>) cover every new namebuild_graphicsuses. By reading, they should build under-Werror, but nobody has run them.Must change
FrameBudget::fitstreats any time spent in the frame as its first build, so a busy retry starves the quick builds after it.fitsisl > 0 && (used <= 0 || build_ms <= l). A busy retry, or a cache answer, spends a few microseconds, soused > 0. From then on, a build runs only if the thread's last build time (build_ms) fits in what is left of the budget.8,build_msis above 8: the description itself puts AMD's quick builds at tens of ms.buildis called and reset inleft), not onused <= 0. Add a test where a busy ask costs some time and a later entry withbuild_msabove the budget still gets the frame's one build.The description is the PR as it was at
471cb339, not as it is now. These parts are wrong at this head:471cb339belongs to this PR".RX_VK_QUICK_MS=1000 RX_VK_TEST_SLOW_MS=8000, which now needsRX_VK_DRIVER_FAULTS=2and the suggested value.471cb339.The "
run_all.pyrun and passing" box is ticked, but nothing has been run at46987848, and this commit changed test_fallback_pipeline_state's stubs, test_quick_pipelines, test_draw_side_effects and test_driver_faults. Runpython -I -B tests/run_all.py, thenpr updatethe description and the checklist. The description also says "#1573" bare. That is Hookshot's PR and needs its full URL, https://github.com/NotRexed/Hookshot/pull/1573.Should change
p.key), for any pipeline state. While any build of the program is in the driver, every quick attempt for that program is busy, and a waiting worker (wait=true) takes the gate back before a non-blocking retry can. In practice a quick pipeline comes only for entries whose program hasn't reached a worker yet: a backlog, as in a new area. Draws whose program is already building stay out (skippable) or wait (must) for the whole build, as on main. That may be most of Volvagia's intro. The gate can't be skipped (issue 996: a second build hangs behind a faulted one), so say so in the description and have the AMD testers watch for it.8is in practice "one quick build a frame per recording thread". With quick builds taking tens of ms, the first build always runs and nothing after it fits. Each one is then a hitch of tens of ms, and a frame can run past the budget by one full build. Say that next to the suggestion.quick_build,ask_start_firstandslow_full_buildare set and cleared by hand, ande.quickis set tokMakingwith no guard.run_guardedkeeps a thread alive after a job throws. A throw insidebuild_graphicswould leaveslow_full_buildset on that worker, ande.quickstuck atkMaking, so every must draw of that entry would poll all 5000 times. A small scope guard would close both.Not checked: the game on any GPU, the Windows build, and whether the tests pass at this head (not run, by the brief).
@ -222,6 +222,9 @@ Pipeline-cache file ownership, temporary storage and failure behavior are descri| `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`) || `RX_VK_QUICK_MS=<ms>` | Quick pipelines (ported from Mako, by sharkiix): a draw whose pipeline is still being built by a worker first asks the caches for the full one without compiling (where the driver has `pipelineCreationCacheControl`), else gets one made at once without the driver's optimizer, into no pipeline cache, and is drawn with it until the worker's full build replaces it. Up to this many ms of those a frame on each recording thread for draws that could be left out: a build is not started where the thread's last one would not fit in what is left, but a frame's first always is, so a frame runs past the budget by at most one build. Draws that must wait get one whatever the budget. Off by default on every driver until measured in the game (it is for AMD's proprietary driver, where the optimizer took 5-13 s a pipeline: Mako, Volvagia's intro a black silhouette); `8` is the suggested value (half a 60 fps frame, as `RX_VK_TRANSLATE_WAIT_MS`; Mako's 1000 trades a missing draw for a hitch of up to a second); `0` or unset: off. Not made where the full build is made without the optimizer already (`RX_VK_NOOPT`, AMD's task and mesh programs); while the driver-fault gate has another build of the program in the driver (AMD, often the entry's own full build) none is made then, and a later draw tries again. Not with `RX_VK_PIPE_SKIP=0`, which waits for every pipeline. Deliberately, a full build that fails (or that the driver faults on) leaves the quick pipeline drawn with for the session, where the draw was left out before. Memory: a quick pipeline the full build replaced is not destroyed (nothing tells when the recordings that used it are done on the GPU), so each entry that got one keeps one more pipeline, unoptimised, for the session; the log's quick pipeline lines count them. Quick builds are not counted in the stats' pipelines. Anything but a number is ignored with a warning; the value in use is logged. Untested in the game in Longshot |With quick builds taking tens of ms on AMD,
8in practice means one quick build a frame on each recording thread: the frame's first always runs and nothing after it fits. That one is still a hitch of tens of ms. Worth saying next to the suggested value.@ -0,0 +56,4 @@/// frame): a frame runs past the budget by at most one build, not by every build started before it was spentbool fits(uint64_t now, double total) {const double l = left(now, total);return l > 0 && (used <= 0 || build_ms <= l);used <= 0stands in for "the frame's first build", but any time spent sets it: a busy retry (a few µs), a cache answer, a must draw's build. Onceused > 0, a build runs only if the thread's last build time (build_ms) fits in what is left. On AMD at the suggested 8 ms, quick builds take tens of ms (the description's own figure), so they never fit. Draw order is the same every frame, so an entry whose program is in the driver (5-13 s a build, serial across the program's variants) retries at the same point each frame and takes the frame's one slot. Every skippable draw after it on this thread gets nothing for that whole time. Track a build actually started this frame (a flag set whenbuildis called and reset inleft) instead ofused <= 0.@ -2252,2 +2271,3 @@// pipeline stays for the session, one more per entry that got one, counted in quick_pipeline's log line)use_counters.made_how = 0;if (const VkPipeline full = build_graphics(*job->p, job->s, job->at, job->topo)) job->e->pipeline = full;slow_full_build = true; // (RX_VK_TEST_SLOW_MS: held inside the driver-fault gate, build_graphics)Nit:
run_guardedkeeps a worker alive after a job throws. A throw out ofbuild_graphicshere leavesslow_full_buildset on that thread. Likewise a throw insidetakeleavese.quickatkMaking, and every must draw of that entry then polls all 5000 times. A small scope guard for these flags (andquick_build/ask_start_firstin quick_pipeline) would close it.@ -2489,2 +2526,2 @@const auto gate = fault_quarantine().begin(p.key, !cold);if (gate == driver_faults::Quarantine::kBusy) return *cold = true, VK_NULL_HANDLE;const auto gate = fault_quarantine().begin(p.key, !cold && !quick_build);if (gate == driver_faults::Quarantine::kBusy) return gate_busy = true, cold ? (*cold = true, VK_NULL_HANDLE) : VK_NULL_HANDLE;The gate is per program (
p.key), for any state. While any build of the program is in the driver, every quick attempt for it iskBusy, and a worker waiting atbegin(wait=true) takes the gate back before a non-blocking retry can get it. So the retry mostly helps entries whose program hasn't reached a worker yet (a backlog). Draws whose program is already building stay out, or wait, for the whole build, as on main. That's correct, since the gate can't be skipped (issue 996), but the description and the AMD test plan should say so.@ -0,0 +177,4 @@{FakeEntry g;int asks = 0, builds = 0;const auto ask_busy = [&](bool& busy) { return asks++, busy = true, 0; };The busy lambdas don't advance
clock, so a busy retry spends 0 and the starvation inFrameBudget::fits(quick_pipe.h:59) can't show here. Worth a case where the busy ask costs some time, then a second entry withbuild_msabove the budget still gets the frame's one build.46987848744a62955427Since #233 merged, I rebased your quick-pipelines commit onto main @
2e1e2e8c(dropping 233's) and addressed my first review in one commit of mine on top. It's pushed as4a629554, and your commit is otherwise unchanged.My commit, "quick pipelines retry a busy driver-fault gate, opt-in with a real budget":
build_graphicsreportskBusythrough agate_busyflag, andtakethen leaves the entry atkNone, so a later draw asks again.kTriedstays for made, won't be made, faulted, and failed.RX_VK_TEST_SLOW_MSnow holds the full build inside the gate, so a test can meet a busy gate. Off AMD, addRX_VK_DRIVER_FAULTS=2for that.RX_VK_QUICK_MS=<ms>turns it on anywhere. Please flip the AMD default back with numbers.RX_VK_TRANSLATE_WAIT_MSuses. A frame's first quick build always runs. After that, a build starts only if the thread's last one would fit in what is left. The re-review caught that my first version took "first" to mean "no time used yet", so a busy retry early in the frame used up the frame's one build. It now counts builds actually started, with a test for that case.RX_VK_PIPE_SKIP=0waits for every pipeline and makes no quick one, as its docs say.st_pipelines.tests/test_quick_pipelines.pyandtests/test_draw_side_effects.pypass on my side. I didn't run the full suite, so the run_all box isn't settled at this head.Still yours:
471cb339:RX_VK_QUICK_MS=8, plusRX_VK_DRIVER_FAULTS=2withRX_VK_TEST_SLOW_MSoff AMD;slow_full_build,quick_build,ask_start_firstande.quick = kMaking, so a job that throws can't leave them set.Thank you, @HumanBean, for the rebase and for
4a629554. It cleared most of review 1124 in one go and kept my commit intact. I checked it against review 1136's first Must change before building on it. Fixes are ind49cd033(head nowd49cd033, base still2e1e2e8c), and the description has been rewritten for this head.Review 1136
Must change 1,
FrameBudget::fitscounting builds actually started.4a629554fixes the case the review describes.fitsnow asksbuilds == 0, notused <= 0. A busy cache ask returns beforestartedis reached. Your new test (a busy ask that advances the clock, then an entry whose build is above the budget) covers that, and I confirmed it passes.One path was still open:
startedwas called beforebuild. A quick build that met the busy gate itself was therefore counted as the frame's one build. That happens wherever there's no cache ask (nopipelineCreationCacheControl), or when the gate is taken between the ask and the build. An entry whose program is in the driver was then retried at the same point every frame, and starved the draws after it as before.d49cd033counts a build only once the gate let it in (if (!busy) budget.started(...)afterbuild). A build that will never be made (NOOPT) still counts once, since it iskTriedand not retried. The new test: a busy build that costs 1 ms, at abuild_msof 400 against a budget of 8, followed by an entry that still gets the frame's one build. With 4a629554's ordering it fails: "a frame's first build still comes after a build that met the gate busy took some of its time".Must change 2, the description. Rewritten for
d49cd033:471cb339" are gone, and the three commits are listed;RX_VK_QUICK_MS=8suggested;RX_VK_QUICK_MS=8 RX_VK_TEST_SLOW_MS=8000 RX_VK_DRIVER_FAULTS=2off AMD;run_allresults: atd49cd033, 499 passed, 1 failed, 30 skipped; at main @2e1e2e8c, 498 passed, 1 failed, 30 skipped. The one failure,test_lift_cache, fails the same way on main."Rebased on current
main" is unticked. Main moved one commit, tofbd42024(#198, touchesvk_draw.cpp). It merges cleanly, and the touched tests pass on the merge. I didn't rebase, so as not to rewrite your commit without a conflict to resolve. Say if you'd like it rebased.Should change, how far the retry reaches on AMD. Done. The description's risk section, the AMD request (T-H86M) and the
RX_VK_QUICK_MSrow in docs/development.md now say it. The gate is per program, and a waiting worker takes it back first. So quick pipelines mostly come for programs no worker has reached yet. Draws whose program is already building get nothing (they are left out, or wait), as on main. That may be most of Volvagia's intro, and the gate can't be skipped (issue 996).Should change, the suggested 8. Done. On AMD it is in practice about one quick build a frame on each recording thread, each a hitch of tens of ms. This is stated next to the suggestion in the flags table, in docs/development.md and in
kQuickSuggestedMs's comment.Nit, exception paths. Done, in
d49cd033. A smallquick_pipe::Raiseguard setsquick_build,ask_start_firstandslow_full_buildfor a scope and clears them on a throw. Intake, a guard stores the entry's final state,kTriedunless a busy gate leaves itkNone, so a throw out of the ask or the build can't leave itkMaking. Tests: a throw from either lambda leaveskTried, and a throw out of the cutbuild_graphicsclears both flags. Each fails without its guard.fault_quarantine().begin/endaroundgraphics_withhas no guard. That is main's code, so I left it alone.Open from the table, the depth-EQUAL / invariance check for testers. Done. It is in Testing for all three requests, with the reason:
Invariantis on AMD but not on NVIDIA, so T-HE7V is where it would show.Open from the table, the
sleep_for(1ms)poll. Done by making the "5 s" true rather than by waiting on an atomic.kOtherQuickPolls(5000 polls) is nowkOtherQuickWaitMs(5000 ms), measured withnow_ms. A test polls in 1 ms and in 15.6 ms steps and checks that both stop at 5 s by the clock. I didn't usee.quick.waitbecause the loop also ends onready(the full build), which doesn't changequick, and a wait with no timeout could hang a must draw. The coarser wake-up on Windows (a must draw may resume up to about 15 ms late) is stated in the constant's comment.The rest of 1136's table was already resolved by
4a629554. I re-checked each point at this head and they still hold: busy gate retried, the slow-build test inside the gate, opt-in everywhere,always_wait, the kept and failed-build quick pipelines, andst_pipelines.Review 1124
4a629554, and the starvation that followed from it byd49cd033(above).4a629554). The box is unticked with the reason above.4a629554); what 8 means on AMD is documented (d49cd033).RX_VK_PIPE_SKIP=0: no quick path underalways_wait(4a629554).4a629554). Memory isn't measured; it is listed under Not tested.4a629554), and in the description's risk section.st_pipelines: quick builds are no longer counted (4a629554).sleep_for(1ms)poll: bounded by the clock (d49cd033).Tests and requests
d49cd033, and on a merge with current main:test_quick_pipelines,test_draw_side_effects,test_driver_faults,test_fallback_pipeline_state,test_gpl_library_cache,test_jobs_wait_runs_queued.d49cd033and main as above.testing wantedlabel.WIP: feat(vk): quick pipelines for draws whose full build is not doneto feat(vk): quick pipelines for draws whose full build is not done