Repository navigation
feat(agc): keep driver pipeline caches and costly compute pipeline binaries across updates - #13931
Merged
adriadam10 merged 6 commits intoNov 1, 2026
Conversation
MarcZX10
suggested changes
Oct 31, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The PR claims a pipeline-creation speed improvement but does not include the matched performance captures required by the repository contribution rules.
What I ran
Reviewed head 8727a380d7a8ff14a8a65b40c31ba3ef83a85864 against base cd087bd12c258cf6dbb82e203bafd20d9fe2596c; the precheck reports a clean merge preview. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0. No build or C++ tests were run locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13931
gh pr diff 13931 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
git diff --check |
Passed |
| Repository conventions and PR template | Passed |
| Merge preview | Clean against main at cd087bd12c258cf6dbb82e203bafd20d9fe2596c |
| ABI audit and test-quality helper | Could not execute: the helper scripts are absent from the isolated PR checkout; this PR changes no exports and no Python tests |
| Hosted CI | Intake, conventions, and labels passed on the exact head at the final refresh |
| Tests | The PR description reports 18/18 focused Vulkan CTests on an RTX 4080 and 3,171 full-suite passes, with some unrelated restricted-runner failures. I did not independently run these tests. |
Unedited precheck output
All conventions checks passed.
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13931-8727a380d7a8/head-8727a380d7a8-ab4a4570/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13931-8727a380d7a8/head-8727a380d7a8-ab4a4570/tools/pr_test_quality.py': [Errno 2] No such file or directory
git diff --check produced no output. The precheck report records both head and base unchanged at completion.
Findings
Blocking
The PR description claims that a 1,988,739-word pipeline took about 32 minutes to build and 202.7 ms to restore. The repository requires a speed claim to include aps5-summary.jsonl lines for main and the change from the same machine and scene, the corresponding traces, and the performance-report evidence. The PR currently provides driver log timings but no matched summary lines, trace attachments, or trace_digest.py output. Please add the required matched artifacts, or remove the unsupported speed claim and describe the measured scope accurately.
Non-blocking
None identified in this source review.
Sources
- Contribution rule for speed claims, pinned to the reviewed base
- Performance report requirements, pinned to the reviewed base
Not verified
I did not build or run the new C++ tests locally. The author-reported test results are not independently confirmed here; other GPUs and console behavior were not tested by me.
To get this to a good state
Add the matched main/PR summaries, traces, and digest output required for the speed claim, or revise the claim to match the evidence provided. I will rerun the relevant checks on the updated head.
Contributor
Author
|
Thanks, @MarcZX10. You are right that the timings were a speed claim without the matched captures. I removed it:
AI-assisted: yes |
Merged
12 tasks done
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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
|
Windows GPU run of head Windows 11 Enterprise 10.0.22621, Ryzen 9 5950X, Radeon RX 6700 XT with AMD proprietary driver 26.10.1 (Vulkan 1.4.315), WinLibs GCC 15.2.0 UCRT r7, Ninja, Release,
AI-assisted: yes (Claude Code). |
Collaborator
|
@liskeee Reviewed at |
MarcZX10
suggested changes
Oct 31, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — new head, changes requested
The prior performance-report gap has been addressed, but the periodic saver now copies the full Vulkan cache under an exclusive lock every ten seconds even when nothing changed.
What I ran
Reviewed head 8727a380d7a8ff14a8a65b40c31ba3ef83a85864 against base cd087bd12c258cf6dbb82e203bafd20d9fe2596c; the live dependency/mergeability check passed and the PR head and base stayed unchanged during the precheck. Environment: Linux x86_64, Python 3.11.2, Git 2.39.5. No C++ compiler or build was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13931
git diff --check cd087bd12c258cf6dbb82e203bafd20d9fe2596c...8727a380d7a8ff14a8a65b40c31ba3ef83a85864Results
| Check | Result |
|---|---|
| Diff check | Passed |
| Conventions and PR template | Passed |
| Live dependency and mergeability | Passed |
| Export ABI and test-quality helpers | Not executable in the isolated precheck checkout: those local helper files are absent there; this is not a PR-source failure. |
| C++ build and tests | Not run; initial review checks do not build. |
Precheck output
Conclusion: checks failed
Head unchanged at completion: True
Base unchanged at completion: True
All conventions checks passed.
The precheck's overall checks failed status is from its inability to find the optional local pr_abi_audit.py and pr_test_quality.py helpers in its isolated checkout; the diff, conventions, and live mergeability checks passed.
Findings
Blocking
PipelineCache::save(false, false) runs on the 10-second saver thread. The new path removes the previous unchanged-size/unchanged-header fast return, then takes PipelineCacheSynchronization::SnapshotLock and copies the entire VkPipelineCache payload with vkGetPipelineCacheData before comparing its hash with the settled header. That exclusive lock blocks LockForPipelineUse, so an unchanged large cache can stall pipeline creation on every saver tick. The later equality check avoids rewriting the file, but it occurs only after the full snapshot copy. Please restore a cheap unchanged check for periodic SaveUnlessBusy saves while retaining full-content comparison for explicit/final saves where same-size changes must be detected.
Non-blocking
None.
Sources
- Periodic save loop and full snapshot path at this head
- Ten-second save interval and shared/exclusive synchronization locks
- Periodic saver and explicit/final save call sites
Not verified
- Runtime cost on a large real driver cache; I have not run matched performance captures.
- C++ build, CTest, GPU behavior, or console behavior.
To get this to a good state
Keep periodic saves from taking a full exclusive snapshot when both the cached size and on-disk header are unchanged, while preserving the same-size-change check for explicit/final saves. I will rerun the same checks on the updated head.
Contributor
❌ NVIDIA GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_pipeline_binary_vulkan: Failed (also failed when re-run alone), assertion and diagnosticsFailing on main too (4, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validated but not compared (4, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 426 tests)Skipped (50)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. |
Contributor
Author
|
@oneandonlydean @MarcZX10 thank you, you are both right: commit 1 dropped the cheap check that kept the 10-second save free while nothing changed. I fixed that and two more problems I found while checking it, in three new commits on top (no force-push):
Validation layer, same selection as in the description: @TechLevelZero thank you for the run. My best explanation for @JJenkx thank you for the RX 6700 XT run. I added it to "GPUs tested", marked as run at 8727a38. I also corrected several sentences in the description that overstated what the code does (cleanup timing, rollback, the threshold wording and the test descriptions). AI-assisted: yes |
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 426 tests)Skipped (51)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. |
MarcZX10
approved these changes
Oct 31, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-checked — new head, looks good
The updated head fixes the periodic cache snapshot regression and retains the driver-cache fallback when pipeline-binary capture fails.
What I ran
Reviewed head e65ab33664a8351420b0e7bf890e87d9c7abee12 against the PR base cd087bd12c258cf6dbb82e203bafd20d9fe2596c; the live merge preview is clean against current main at 474030e58424fefdb65e5df9f3820810fbb4553c. Environment: Linux x86_64, Python 3.11.2, Git 2.39.5. I did not invoke a compiler or build locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13931 --output-dir /tmp/anyps5-13931-update-precheck
gh pr checks 13931 -R boykopovar/AnyPS5I also inspected the complete PR diff and update discussion, including the periodic-save logic, binary-capture failure markers, active-layer check, and added tests. The precheck ran its live dependency/mergeability check against current main.
Results
| Check | Result |
|---|---|
| Diff whitespace check | Passed |
| Repository conventions and PR template | Passed |
| Live dependency and mergeability preview | Passed; clean against current main at 474030e58424fefdb65e5df9f3820810fbb4553c |
| Current GitHub checks | conventions and labels passed on the exact head; no upstream build/test job is present in this check rollup |
| ABI and test-quality helper scripts | Not executable in the isolated precheck checkout because those helper files are absent there; this PR changes no public exports or Python tests. I manually inspected the new C++ assertions. |
| Focused Vulkan tests | Not run locally. The author reports 14/14 selected tests on main (b8f0a6b202) and 19/19 on this head using an RTX 4080 with validation enabled and no VUIDs. |
| NVIDIA GPU CI report | Not run by me. TechLevelZero reports a successful build and 2,432 passed tests on this exact head, with five failures reported as not new (four also fail on its main baseline and one is unstable there), plus 51 skips. Its run merged the PR head onto older main 73f4f7e; it is useful corroborating evidence, not a current-main run. |
| C++ build, local CTest, physical console | Not run by me |
Unedited precheck command output
Conventions stdout:
All conventions checks passed.
ABI helper stderr:
/usr/bin/python3: can't open file '/tmp/anyps5-13931-update-precheck/head-e65ab33664a8-c37535f0/tools/pr_abi_audit.py': [Errno 2] No such file or directory
Test-quality helper stderr:
/usr/bin/python3: can't open file '/tmp/anyps5-13931-update-precheck/head-e65ab33664a8-c37535f0/tools/pr_test_quality.py': [Errno 2] No such file or directory
The precheck's recorded conclusions were diff_check: passed, conventions_and_pr_template: passed, and live_dependency_and_mergeability_check: passed. These are report statuses; the successful diff check produced no output.
Findings
Blocking
None found in the updated head.
Non-blocking
- The periodic-save update restores the cheap unchanged-cache path while retaining full snapshots for explicit/final saves. The new cases cover unchanged reopened caches, same-size changes, and legacy import.
- Capture failures now leave a marker so later runs use the ordinary driver-cache path instead of retrying capture on every run. The added tests cover extraction failure, oversized data, rejected capture, reopening, and a lowered time threshold.
- The layer guard disables pipeline binaries if active-layer enumeration fails or an older validation/gfxreconstruct layer is present; its tests cover these branches. I did not independently reproduce the reported NVIDIA layer crash.
Sources
- Updated pipeline-cache save path
- Capture fallback and layer compatibility logic
- Updated regression tests
- Binary-store and layer-guard tests
Not verified
- I did not independently run the author-reported Vulkan CTests or the NVIDIA GPU CI suite.
- No local C++ build/CTest, macOS/Linux test matrix, or physical-console behavior was run by me.
EagleBearerYS
added a commit
to EagleBearerYS/AnyPS5
that referenced
this pull request
Oct 31, 2026
12 tasks done
liskeee
force-pushed
the
perf/agc-persistent-pipeline-cache-upstream
branch
from
October 31, 2026 20:00
e65ab33 to
6ca7ba4
Compare
Contributor
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validated but not compared (5, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 476 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. |
Keep driver blobs outside source-version cleanup and import compatible legacy caches. Persist equal-size content changes and retry incomplete snapshots without replacing the previous file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Capture costly compute builds and restore matching recipes after checking full recipes, driver fingerprints and Vulkan compatibility keys. Add size and time thresholds, opt-out, atomic persistence and corruption/rejection fallback. Remember below-threshold recipes in separate files so competing decisions cannot overwrite captured binaries. Prefer saved binaries and retry capture when the time threshold is lowered. Cover compatibility, fallback, deterministic competing writers and cross-process capture, restore and corruption. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Restore the cheap size-and-header check for the 10-second worker save, with the size query still under the snapshot lock, so an unchanged cache is no longer copied and hashed on every tick, including the first tick after loading a current cache. A legacy import is still written by the first periodic save. SaveNow and the final save keep the content comparison that writes equal-size changes. Cover periodic copy avoidance after saving and after reopening, legacy migration and equal-size persistence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A costly build whose binaries cannot be extracted or stored, and a rejected capture build, now leave a capture-failure marker, so later runs build that recipe through the driver cache instead of repeating a capture build with a null pipeline cache. Lowering the time threshold does not retry these markers; a driver change does. Existing below-threshold markers stay compatible and retryable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Check the enabled layers before querying pipeline binary features. A validation or gfxreconstruct layer older than Vulkan 1.3.294 wraps handles but passes the extension's commands to the driver unchanged, so keep the driver pipeline cache path there and when enumeration fails. Cover the layer decisions with a fake enumerator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y store; fix pipeline binary skip messages
adriadam10
force-pushed
the
perf/agc-persistent-pipeline-cache-upstream
branch
from
November 1, 2026 11:25
6ca7ba4 to
ad8567f
Compare
Collaborator
|
Checked the content key (driver fingerprint, extension keys, full recipe), the null |
13 tasks done
12 tasks done
12 tasks done
Collaborator
|
@liskeee thank you for the pipeline-cache work. We've had to revert it for now in #14839, because with the default settings it crashes at device creation on our NVIDIA test machine, and we'd like to help it land again. What we see (RTX 4090, NVIDIA driver 610.57.04, Linux): every Vulkan device creation with the shader cache enabled segfaults in Why CI didn't catch it: most AGC tests set When you re-land it, could you add a test that runs with the cache enabled (no |
oneandonlydean
added a commit
that referenced
this pull request
Nov 1, 2026
revert(agc): keep driver pipeline caches and costly compute pipeline binaries across updates (#13931)
winstrex
added a commit
to winstrex/AnyPS5
that referenced
this pull request
Nov 1, 2026
Resolve the include list in tests/Recorder.cpp after the revert of boykopovar#13931: take main's ShaderCacheSourceVersion.hpp and keep WatchedBlock.hpp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
12 tasks done
12 tasks done
12 tasks done
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
A large compute pipeline can take many minutes to build in the driver (#13538 reports 13 minutes on an RTX 4090), and today every AnyPS5 update throws that work away. Two changes keep it, with review fixes on top:
1.
fix(agc): preserve driver pipeline caches across source updates(+fix(agc): skip unchanged periodic pipeline cache snapshots). The driver blob moves out of the source-version directory, which every AnyPS5 update replaces with a new one (the old one is removed after 14 unused days), and a compatible legacy blob is imported once.SaveNowand the final save now write a content change of equal size (it was skipped before), while the 10-second background save keeps its cheap size-and-header check and copies nothing while the cache is unchanged. An incomplete snapshot no longer replaces the previous file.2.
feat(agc): persist costly compute pipeline binaries(+fix(agc): retain driver cache fallback after binary capture failuresandfix(agc): disable pipeline binaries with outdated wrapping layers). Costly compute pipelines are stored withVK_KHR_pipeline_binaryand restored without a rebuild:Anything missing, corrupt, rejected or mismatched falls back to a normal build.
APS5_NO_PIPELINE_BINARIESturns the store off. The store also stays off when an enabledVK_LAYER_KHRONOS_validationorVK_LAYER_LUNARG_gfxreconstructis older than Vulkan 1.3.294, because such a layer wraps handles but does not know the extension's commands. A marker never replaces a captured binary. Lowering the time threshold retries a recipe that was below it, but not one whose capture failed; a driver update retries both, because the key holds the driver fingerprint.Rollback: reverting the PR restores today's behaviour. At run time,
APS5_NO_PIPELINE_BINARIESturns the binary store off (the driver cache stays in its new place), and deleting the cache directory resets both caches. Blast radius: compute pipeline creation and disk use. Nothing is evicted yet; the debt entry and USAGE.md say so first.Overlap: #9508 replaces the compute creation call this touches, #11435 adds a pipeline-cache option beside the new store, and #7622 edits PipelineCache.cpp's includes. The pr-overlap bot also lists open PRs that touch neighbouring lines in VulkanDevice.cpp, Recorder.cpp, USAGE.md and the debt file. These are textual neighbours, not duplicates; I am happy to rebase on whichever lands first.
Tested
Windows 11 Pro 25H2 (build 26200), NVIDIA GeForce RTX 4080, driver 610.88.
[pipeline-binary] built key=bb77041da96ad820 words=1988739), and the next run restored it from the store instead of building it again. This PR makes no frame-time claim, so there are noaps5-summary.jsonllines;maincannot reach that pipeline in this title.VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1), selectionagc_(pipeline_cache|pipeline_binary|pipeline_journal|driver_recorder|driver_pipeline_journal):mainb8f0a6b 14/14, this branch 19/19; zeroVUID-and zero missing-layer messages in both.agc_driver_recorder_tests,agc_pipeline_binary_store_testsandagc_pipeline_cache_allocation_testspass 3 of 3 runs with the validation layer, no VUIDs.guest_wide_formatting(cp1250 zone name, fix(libc): convert Windows time zone names to UTF-8 #10987); none of them touches these files.tools/check_conventions.py --base origin/main: all checks passed.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)