Repository navigation
fix(agc): look up and attach draw recipes on the relocated donor entry - #7369
Open
Flamefusion wants to merge 3 commits into
Open
Flamefusion wants to merge 3 commits into
Flamefusion wants to merge 3 commits into
Conversation
MarcZX10
reviewed
Oct 18, 2026
| std::shared_ptr<DrawEntry> entry; | ||
| { | ||
| void Driver::attachDrawRecipe(std::uint64_t key, const std::vector<std::shared_ptr<DispatchVariant>>& stages, std::shared_ptr<const DrawRecipe> recipe, const std::shared_ptr<DrawEntry>& donor) { | ||
| std::shared_ptr<DrawEntry> entry = donor; |
Contributor
There was a problem hiding this comment.
This donor path lets relocated entries from different draw keys attach recipes to the same DrawEntry. The recipe update below is a load-copy-store sequence: if two completions load the same old vector, each builds a vector with only its own new record, and the later store drops the other attachment. AtomicSharedPtr makes each operation safe, but not this compound update. Could this use a compare-and-retry loop (or per-entry serialization) so parallel relocated draws do not discard each other’s cached recipes?
Contributor
Author
There was a problem hiding this comment.
Good catch, thanks. The race was already possible on one key, but sharing the donor entry across keys makes it much more likely, and pipelined commits can attach in parallel.
Fixed in 090d66e: attachDrawRecipe now uses a compare_exchange_strong loop. If another attach replaced the list after the load, the new list is rebuilt from the list now stored and the exchange is retried, so both records are kept (still capped at dispatchVariants()). Both the AtomicSharedPtr fallback and std::atomic<std::shared_ptr> provide compare_exchange_strong.
The draw cache relocation and reuse tests (all variants), address passes, mesh draw relocation and linear target resident tests still pass.
|
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.
|
Contributor
|
Thanks for the fix. The compare-and-retry rebuilds from the latest recipe list on contention, which addresses the lost-attachment race I flagged. The updated head is clean; I have not run the draw-cache tests locally. |
github-actions
Bot
added
agc
Contributor
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, To pick it up: Head is |
adriadam10
force-pushed
the
fix/agc-relocated-draw-recipes
branch
from
October 19, 2026 14:22
090d66e to
403651e
Compare
Contributor
Author
|
@adriadam10 do you need anything from my side for this pr ? |
github-actions
Bot
added
needs rebase
A draw that misses on its own register key and is served by a relocated draw entry (relocateDrawEntry, boykopovar#4943) never got a draw recipe: - relocateDrawEntry builds a fresh DrawEntry with per-draw copies of the donor's variants. That entry is not stored in drawCache under the draw's key. - Driver::draw still passes the draw's own key to findDrawRecipe and, after recording, to attachDrawRecipe. Both look the key up in drawCache, find nothing and return, so a recipe built while recording such a draw is dropped and the next relocated draw of the same shape records in full again ("attaches 0" with APS5_TRACE_DRAW_CACHE). - The recipe records also compare stage identity against the per-draw copies, which are new objects on every draw, so even a stored record could never match. Changes: - DrawEntry and DispatchVariant gain a donor pointer. relocateDrawEntry sets it on the relocated entry and on every copied variant, always to the original cached entry/variant (a copy of a copy points at the root), so the chain never grows. - Driver::draw keys the recipe stages on the donor variant when a matched variant is a relocation copy, and hands the donor entry to findDrawRecipe/attachDrawRecipe (also through commitDraw on the pipelined path). With a donor, both read and update the donor entry's recipe list directly instead of looking the key up; without one they behave as before. - DrawWithRecipe rebuilds the push constant block from the current shaders and patches it through the recorded resources when the recipe has push stages, instead of replaying the bytes captured with the recipe. A relocated draw moves user words that end up in push constants, so the captured block would carry the first draw's values. Draws that are not relocated take the same path as before: their matched variants have no donor and recipe lookup still goes through drawCache. Measured on Hades (PPSA03355, discussion boykopovar#3234), where nearly every sprite draw is served through a relocated entry: lookups and attaches now succeed, but Graphics::Draw builds a recipe for only ~15 of ~100,000 draws per 10 s because the draws' ShaderResources are not Reusable(), so the frame rate does not change yet. The fix is what lets recipes apply to relocated draws once that is addressed.
Adds RelocatedDrawsReuseTheirRecipe to the draw cache relocation test. It covers the case the previous commit fixes: a draw that misses on its own key, is served by a relocated draw entry and should then be recorded from the recipe of the cached draw it was relocated from. - The draws read a constant through a user pointer that moves on every draw to identical contents, like MovedDataHitsTheCache, so after four teaching draws every further draw is a relocated hit. - They render into a 64 KiB guest block with the SW_64KB_R_X swizzle mode. The linear 1 KiB target the other cases use is never resident, and a draw into a non-resident target is not recorded, so it never builds a recipe and could not show the bug. - The case checks that all six relocated hits are draw-cache hits and that all six were recorded through DrawFromRecipe. VulkanDevice::DrawRecipeHits() returns the draw kind's recipe hit counter that the [recipe] profile line already keeps, so the test can read it without APS5_PROFILE_DRAW. Without the previous commit the case fails with "0 of 6 draws served by a relocated entry were recorded from the cached draw's recipe": the lookup uses the draw's own key, which has no draw cache entry. With it, agc_driver_draw_cache_relocation, agc_driver_draw_cache_reuse (and its _off, _verify and _inline variants), agc_driver_address_passes, agc_driver_mesh_draw_relocation and agc_driver_linear_target_resident pass.
attachDrawRecipe builds a new recipe list from the entry's current one and stores it: it loads the old list, copies the still-valid records behind the new one and stores the result. Each AtomicSharedPtr operation is atomic, but the sequence is not. If two attaches load the same old list, each stores a list holding only its own new record, and the later store drops the earlier attachment. With recipes attached through the relocated donor entry, draws with different register keys that relocate from the same cached draw attach to one DrawEntry, and on the pipelined path their commits run apart from the worker. Concurrent attaches to the same entry are therefore no longer limited to one key. The update is now a compare_exchange_strong loop. When another attach has replaced the list since it was loaded, the new list is rebuilt from the list now stored and the exchange is retried, so both records are kept, within the dispatchVariants() limit as before. The AtomicSharedPtr fallback and std::atomic<std::shared_ptr> both provide compare_exchange_strong. agc_driver_draw_cache_relocation, agc_driver_draw_cache_reuse (and its _off, _verify and _inline variants), agc_driver_address_passes, agc_driver_mesh_draw_relocation and agc_driver_linear_target_resident pass.
Flamefusion
force-pushed
the
fix/agc-relocated-draw-recipes
branch
from
October 20, 2026 05:44
403651e to
562f910
Compare
Contributor
Author
|
Rebased onto current Thanks @blawesom for the earlier rebase handoff.
Verified on Windows 11, MinGW GCC 15.2, Ninja, Release,
|
12 tasks done
This was referenced Oct 21, 2026
12 tasks done
This branch has not been deployed
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
4 participants
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
A draw that misses on its own register key and is served by a relocated draw entry (
relocateDrawEntry, #4943) never uses or keeps a draw recipe, so every such draw is recorded in full (Graphics::Draw) instead of from the cached draw's recipe (DrawFromRecipe).In
Driver::draw(Execution/src/Driver/Draw/Draw.cpp):relocateDrawEntrybuilds a freshDrawEntrywith per-draw copies of the donor's variants. That entry is not stored indrawCacheunder the draw's key.findDrawRecipeand, after recording,attachDrawRecipestill look up the draw's own key indrawCache. They find nothing and return, so a relocated hit never finds the donor's recipe, and a recipe built while recording it is dropped. WithAPS5_TRACE_DRAW_CACHEthis shows as "attaches 0".Fix:
DrawEntryandDispatchVariantgain adonorpointer.relocateDrawEntrysets it on the relocated entry and on every copied variant, always to the original cached entry or variant (a copy of a copy points at the root), so the chain never grows.Driver::drawkeys the recipe stages on the donor variant when a matched variant is a relocation copy. It hands the donor entry tofindDrawRecipeandattachDrawRecipe, also throughcommitDrawon the pipelined path. With a donor, both read and update the donor entry's recipe list directly instead of looking up the key; without one they behave as before.Graphics::DrawWithReciperebuilds the push constant block from the current shaders (AssemblePushConstantsplus the recorded patches) when the recipe has push constants. Relocation moves user words that land in push constants, so the donor's block must not be replayed as-is.Found while profiling Hades (PPSA03355, discussion #3234), whose sprite draws move per-draw pointers on almost every draw. There, recipe attaches now succeed, but few of its draws build a recipe at all: most hold an address-based build's lease, so they are not reusable. This PR therefore claims no frame-rate change; it makes the relocated path consistent with the exact-key path.
Test:
agc_driver_draw_cache_relocationgainsRelocatedDrawsReuseTheirRecipe.VulkanDevice::DrawRecipeHits()exposes the draw recipe hit counter the[recipe]profile line already keeps, so the test can read it withoutAPS5_PROFILE_DRAW.Tested
Windows 11 (build 26300), WinLibs GCC 15.2.0 (posix-seh, ucrt), Release,
-DBUILD_TESTING=ON, Ryzen 7 7435HS / 16 GB, NVIDIA GeForce RTX 3050 Laptop GPU (driver 617.42, graphics pipeline libraries available), rebased onmainb39d90bf1:agc_driver_draw_cache_relocationpasses, as doagc_driver_draw_cache_reuseand its_reuse_off,_reuse_verifyand_reuse_inlinevariants.agc_driver_address_passes,agc_driver_address_passes_targets_split,agc_driver_mesh_draw_relocationandagc_driver_linear_target_residentalso pass.0 of 6 draws served by a relocated entry were recorded from the cached draw's recipe, expected all.[recipe]profile line go from 0 to non-zero, though only about 15 recipes are built per ~100,000 draws (see above). No frame-rate change.tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.py --branchreports four line overlaps, none implementing the same thing: perf(agc): share one image table per heap and write epoch through capture, compare, resolution and descriptors #6454 (test includes and theMovedDataHitsTheCacheend inDrawCacheRelocation.cpp), fix(agc): order deferred write-backs after queued draws' index reads #6914 (the recipe block inExecution/.../Draw.cpp), and feat(agc): render layered draws into 2D array color views #7254 and feat(agc): render layered draws into a depth view of several slices #7256 (DrawWithRecipeinGraphics/src/Draw.cpp). Happy to rebase on whichever goes first.APS5_NO_GPLis set.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)