Repository navigation
fix(agc): bound a journal replay's graphics and compute pipelines apart - #8542
oneandonlydean merged 1 commit into
Conversation
|
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.
|
62171b0 to
de02531
Compare
|
Restacked on #8365's rebased head (30c643b, on AI-assisted: yes (Claude Code) |
|
Windows run of this head (
The new cases write and replay journals past both limits, so the test takes about a second longer here. The 28.4 s run was the second of the first three. It didn't come back in six more runs, and the VM's disk is a qcow2 on NFS, so I take it as I/O on this machine rather than the change. I haven't confirmed that. AI-assisted: yes (Claude Code) |
de02531 to
ff8ec20
Compare
|
GTA V (PPSA04264), Windows 11, RTX 4090, open world: the graphics store overflows on its own. It settles at about 1,270 distinct pipelines after about 3 minutes, so the 1,024 LRU bound keeps evicting pipelines that are rebuilt right after, at about 200 ms each (up to 1.6 s) under the GPU mutex. Last 330 s of one run each:
With So the graphics journal bound is tied to a store bound that is too small for this title. I plan a separate PR that grows the store adaptively, as #7189 does for the resource cache (a ghost list of evicted keys, doubling up to 16384, with a set value still fixing it), and sizes the graphics journal bound to match. It touches the same lines of AI-assisted: yes (Claude Code) |
|
Thanks @tibzejoker, those numbers make the case. At about 1,270 distinct pipelines, a 1,024-entry store evicts pipelines the title rebuilds right away, and An adaptive store with the graphics journal bound sized to match sounds right to me. This PR keeps the graphics and compute bounds separate, so yours only has to move the graphics one. One note for building on it: this PR is stacked on #8365 (compressed journal modules), which is on #8213, so its head carries their commits until they merge. If you base on this branch you'll carry them too. Once they land, rebasing onto AI-assisted: yes (Claude Code) |
|
Follow-up: the store side is now #9401. It replays up to 16384 journal records, lets the replay grow the store instead of skipping once it is full, and grows the bound while evicted pipelines come back. In the GTA V open world, builds in play went from 80-153 to 42. It touches the same journal-bound line as this PR, so I'll rebase it onto yours if yours lands first. AI-assisted: yes (Claude Code) |
|
Thanks @tibzejoker. 42 builds in play, down from 80–153, is a big drop. I trial-merged #9401's head with this PR: they conflict in AI-assisted: yes (Claude Code) |
|
Overlap note: #9440 changes the same bound line in The changes compose: the per-kind bounds of this PR can be applied to the entries #9440 keeps. I will rebase onto whichever lands first. |
|
Thanks @00xDisaster, agreed that they compose: this PR's per-kind bounds apply to the entries #9440 keeps. If #9440 lands first I'll rebase this onto it. I also left a review on #9440 about the record that triggers an eviction. AI-assisted: yes (Claude Code) |
ff8ec20 to
98efc1f
Compare
|
@milescoviello #9401 changes the same line in Proposed order: yours first, since it is older and smaller. I'll rebase #9401 onto it and keep your per-kind counting, so only the graphics default changes. If #9401 lands first, the conflict is that one line plus the AI-assisted: yes (Claude Code) |
|
@tibzejoker that works for me: this one first, then #9401 on top, keeping the per-kind counting. #9401 changes only the graphics default to 16384, and compute keeps Note that @00xDisaster's #9836 (#9440's successor) changes the same replay bound to keep the newest entries instead of the first. We agreed that either order works. If #9836 lands before this, I'll rebase this onto it, and you'd then rebase #9401 onto this as planned. Either way, #9401 only needs this PR's line and the AI-assisted: yes (Claude Code) |
|
@milescoviello main now conflicts with this branch after #9567 (merge c9b2660) rewrote This is a logic conflict, not a mechanical one, so we'd rather you resolve it than guess. Could you merge or rebase onto main at c9b2660 or later and resolve:
The per-kind limit should still hold with main's newest-entries eviction order. We'll re-run CI on the new head. |
98efc1f to
a45877d
Compare
|
@oneandonlydean done. The stack was rebuilt on #8213's refreshed head
Tested (Linux, Ubuntu 26.04.1, GCC 15.2, RTX 4070 Ti, 615.71.09, and the Windows 11 VM, GT 1030, 582.78):
AI-assisted: yes (Claude Code) |
|
@milescoviello thanks for the rebase, and for the correction: the journal rewrite came from #9836, not #9567. We re-reviewed both layers from scratch. #8365 (3ac931e) and #8542 (a45877d) both pass: the compression now sits in main's One thing at landing: both heads use journal |
|
@oneandonlydean thank you. I'll leave both branches as they are while CI and the gate run. When you land them, I can push the one-line AI-assisted: yes (Claude Code) |
|
@milescoviello Thanks. Please push the |
a45877d to
cb2ba99
Compare
|
@oneandonlydean #8542 now conflicts with The conflict is with #9401, which raised the default of
The rebased head ( Should I push it to this PR now, or wait until #8365 lands? AI-assisted: yes (Claude Code) |
|
@milescoviello I still have #8365 first. Please wait for it to land before refreshing this PR, as agreed. The graphics default of 4096 with a separate compute limit of 1024 preserves #9401's change; keep that split when restacking. #8365 needs main+1 for its format at landing, and #8542 should inherit that codec number unless it changes the record format itself. Then we can review and run CI on the final heads. |
|
@oneandonlydean understood. I'll leave this PR as it is until #8365 lands. Then I'll restack it with the 4096 graphics / 1024 compute split and #8365's format number: #8542 doesn't change the record format itself, so it inherits that number. AI-assisted: yes (Claude Code) |
|
An AMD RDNA 4 run on Windows of Windows 11 Pro 10.0.22631, AMD Radeon RX 9070 XT (
AI-assisted: yes (Claude Code) |
|
@bunbeans2-arch thanks for the RX 9070 XT run, and for building the head as is: the separate graphics and compute bounds show in the replay lines there too. The AI-assisted: yes (Claude Code) |
The replay kept the newest APS5_PIPELINE_JOURNAL_ENTRIES entries of either kind, so compute pipelines used up the bound meant for the graphics pipeline store, and the entries left out were not reported. Graphics entries now count against that bound alone, compute entries against APS5_PIPELINE_JOURNAL_COMPUTE_ENTRIES (1024); each kind keeps its newest entries in record order, and a line says how many of each a replay leaves out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cb2ba99 to
ee0d52b
Compare
|
@oneandonlydean #8365 has landed, so this is restacked as promised:
AI-assisted: yes (Claude Code) |
✅ AMD GPU test: passedPR head
Failing on main too (10, not counted against this PR)Validation IDs already present on main (5 IDs in 481 tests)Skipped (54)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validation IDs already present on main (5 IDs in 484 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
A journal replay keeps the last
APS5_PIPELINE_JOURNAL_ENTRIESentries (4096) of the loaded journal, the newest since #9836. That bound is there for the graphics pipeline store, which #9401 sizes to the title's working set. But the replay counts compute entries against it too, and truncates silently. So in a title that records both kinds, compute pipelines take graphics slots, and whatever the bound cuts goes unreported.This PR applies
APS5_PIPELINE_JOURNAL_ENTRIES(4096) to graphics entries only. Compute entries count separately against a newAPS5_PIPELINE_JOURNAL_COMPUTE_ENTRIES(1024), as agreed earlier in this thread. Each kind keeps its newest entries, as #9836 does for both together, and the recorded order is kept. When a replay leaves entries out, it says so, for example:ENVIRONMENT.mddocuments the new variable, and the journal debt entry names both bounds. The record format doesn't change, so the journal keeps #8365's format number.Why
@SleglJan found this with #8365. On PPSA11386, the default-cap seed held 1,322 entries: 628 graphics, 694 compute and their modules. The warm run replayed exactly the first 1,024 (481 graphics, 526 compute, 17 already present), and the load line still said 1,322.
Tested
Restacked onto
main77d536fb38, which now has #8365, asee0d52b98f; it also merges cleanly with currentmain31c727f622. The change is the same commit, replayed alone. The per-kind test from before is unchanged.Linux (Ubuntu 26.04.1, GCC 15.2), RTX 4070 Ti (NVIDIA 615.71.09), Release:
ctest -R pipeline_journal(agc_driver_pipeline_journal, which holds the per-kind replays,agc_pipeline_journaland_long_paths) passes 3 of 3 onee0d52b98f.Khronos validation (
VK_LAYER_KHRONOS_validation1.4.363, LunarG SDK) over the same 3 tests, on the base77d536fb38and onee0d52b98f: 3 of 3 pass on each, the tests created 17 and 20 Vulkan devices that the layer checked, and there is noVUID-message on either side.Windows 11 VM (GT 1030, R580, WinLibs GCC 15.2.0, Release):
agc_driver_pipeline_journal(the per-kind replays),agc_pipeline_journal,_long_pathsand_extended_pathspass 3 of 3 on the base77d536fb38and 3 of 3 onee0d52b98f.Earlier results, on the heads named in them (before the restack):
main's single newest bound fails 3 of 3 on the bounded-replay case (a replay bounded to 1 graphics and the default compute entries replayed 1 graphics and 0 compute pipelines). A bound that keeps the first entries of each kind fails 3 of 3 in the newest-entries test.cb2ba99693.Not tested: a title run.
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