Skip to content

fix(agc): release pipeline cache save temporaries after use - #9558

Merged
adriadam10 merged 6 commits into
boykopovar:mainfrom
Izamaru:fix/pipeline-cache-save-temporaries
Oct 24, 2026
Merged

adriadam10 merged 6 commits into
boykopovar:mainfrom
Izamaru:fix/pipeline-cache-save-temporaries

Conversation

@Izamaru

@Izamaru Izamaru commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

What

When PipelineCache::save merges a cache already on disk, it keeps the input file buffer and the temporary Vulkan cache alive until the function returns. Both overlap the serialized output buffer, its hash and the atomic file write. Large cache files therefore add avoidable host-memory pressure during a save.

Release disk.contents after vkCreatePipelineCache consumes its payload, before merging/serializing. Destroy the temporary merged cache immediately after vkGetPipelineCacheData copies its output. Its guard still destroys it on earlier returns and clears the handle to prevent a second destruction. Keep the merge diagnostic in a separate boolean.

One production file changes. Cache contents, format, source namespace, save policy, snapshot locking, size floor and error paths are preserved. No cache budget or title-specific policy is added. This follows Vulkan's cache data retrieval and cache destruction lifetimes.

Tested

Current-integration paired verification, 2026-10-22

Refreshed in response to the exact-head/current-main review request. The main branch advanced during preparation; the final comparison uses these exact snapshots:

  • Upstream main: 83462cc57ddecac5e5f3cdafafbe38d406e55953.
  • Final PR head: 4e28be4c6cc659d17d6d310422a043845ed35075.
  • Full final-head tree: 267120c401ad69a803bb8152e3f9f5649038db20.
  • Parents: previous PR head c6f2cd2db104ccb59b061a55a970a4fcb717af81 and the pinned main above.

The exact final commit was created in the fork and fetched by SHA before moving the PR branch. Its complete tree was checked against the clean local merge; that exact commit was rebuilt and tested, then the branch was moved to it without rewriting history. Only core/libs/prx/libSceAgcDriver/Graphics/src/PipelineCache.cpp differs from the pinned main. Its production patch is unchanged from the earlier tested head ae1c84b0b.

Same machine/GPU for both selections: Windows 11 build 26200, WinLibs MinGW-w64 GCC 15.2.0 r7, Release; NVIDIA GeForce RTX 4090, driver 617.14; Khronos validation 1.4.363.0.

The eight cases are:
agc_pipeline_cache, agc_pipeline_cache_file, agc_pipeline_library, agc_pipeline_library_off, agc_pipeline_library_fast_only, agc_pipeline_library_clear_waits_for_build, agc_pipeline_store_lazy_buffer_pool, agc_driver_graphics.

Paired selection Result Khronos instance-layer insertions VUIDs Missing layer / skipped tests
main 83462cc57 8/8 5 0 none
exact final head 4e28be4c6 8/8 5 0 none

Both run serially with ctest -V --timeout 30, ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation and loader layer diagnostics. Both logs confirm actual Khronos loading and the RTX 4090 device, contain no requested-layer-not-found message, and have empty VUID sets. The change adds no validation errors in this selection. The cache-file fixture explicitly enables its private persistence cache to exercise saves. agc_driver_graphics is a CPU fixture; native Vulkan coverage comes from the cache/library fixtures.

git diff --check and conventions against the pinned upstream main pass. The literal origin/main conventions check was also run; it reports unrelated upstream changes because that ref tracks our older fork main.

Limitations: focused Windows/NVIDIA validation only. No new Linux, other-vendor GPU, full-suite, title-runtime or combined #7622/#9558 runtime test. No new save-time memory benchmark was run for this refresh. These results do not establish completion of full Windows/Linux project CI; that build workflow still requires collaborator dispatch for the updated head. Subsequent main commits are outside these pinned runs.

Earlier save-time memory measurement — not rerun for this refresh

Four fresh-process runs in main/change/change/main order, comparing main 3ca07d086d5f44fbe3c00370e7f03953ab46ca1b with local integration c75625764251d4db13cfa66a9f6594555eee1ebc (tree 2701b67ddb91a67f4b30eec8269d12021a5b6250, published as ae1c84b0b2ec2a4bb6858451bbd82adccb1ccd69). A private probe built from the then-current main device fixture uses the real PipelineCache and alternates only the production driver DLL. It imports and merges the same two opaque NVIDIA cache files (338,090,706 and 361,513,974 bytes). No shader dispatch or title is run. This persistence probe deliberately enables its private cache (ANYPS5_NO_SHADER_CACHE=0); the paired focused regression suites use 1.

Run Process peak commit (MiB) Save duration (ms)
main A 1639.617 507.201
PR integration A 1315.285 486.029
PR integration B 1314.789 505.575
main B 1638.875 500.355
median main 1639.246 503.778
median PR integration 1315.037 495.802

Median peak reduction: 339,957,760 bytes (324.209 MiB). The peak is Windows PROCESS_MEMORY_COUNTERS_EX::PeakPagefileUsage (process peak commit, not physical pagefile writes). All four outputs are byte-identical: 361,513,974 bytes, SHA-256 11f8fd160edfb5241330d4097f3fccd707bf77a1d8c3c6564fd9c7fb6d02ac9e. Input hashes remained unchanged. Khronos was active in every probe process, with no missing-layer messages or VUIDs. The tested candidate DLL was restored afterward. This benchmark was not repeated on the new main or final head; its numbers apply only to the earlier pinned comparison.

This is a save-time peak-memory result. No FPS, speed, whole-game memory or title-unblock claim. The probe and opaque inputs remain local; no captured game data is committed. No Linux, full-suite or title runtime validation; full Windows/Linux CI still requires a collaborator to dispatch the project build workflow.

Earlier #7622 overlap check — retained with its original base

On main 3ca07d086d5f44fbe3c00370e7f03953ab46ca1b and the corresponding PR tree 2701b67ddb91a67f4b30eec8269d12021a5b6250, inspected #7622 at 778f35ea5fd168bc5b8dc1523acc5d1d7b4181aa. In the shared PipelineCache.cpp, #7622 adds the host-thread helper include and changes the constructor's saver launch from std::thread to Libc::StartHostThread. This PR changes temporary lifetimes in save; it does not implement that worker/MXCSR change.

Three-way merges of the shared file succeed in both orders, producing identical bytes and preserving both changes. This holds against that pinned main alone and that pinned main with this PR. Whole-tree merge checks show that #7622 already conflicts with that pinned main in Execution/src/VulkanDevice.cpp and core/shader/recompiler/ShaderDiskCache.cpp; adding this PR leaves exactly that same conflict-file set. PipelineCache.cpp is clean. No ordering dependency or duplicate implementation was found for this PR's change. A combined #7622/#9558 runtime build was not tested.

This overlap experiment was not rerun for the current refresh.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source — none added
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: NVIDIA GeForce RTX 4090, driver 617.14, Windows 11 build 26200
  • AI-assisted: yes, OpenAI Codex; author reviewed and is responsible for the submitted change

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lifetime change is narrow and the paired validation/memory evidence is strong, but the PR is based on 8c26eac3 while current upstream main is dce41a89478876a8e3ea9c346b1dac00bb31c8cd (23 commits ahead). The required AGC comparison therefore does not cover current main. Please rebase/update to current main, rerun the same focused validation-layer tests on that main and the updated head on the same machine/GPU, confirm the layer loaded in both logs and compare VUIDs, and refresh the comparison evidence/checklist accordingly. I checked the intervening upstream commits touching PipelineCache.cpp; none are present, but that does not replace the repository’s current-main test requirement. I did not run tests locally. Existing hosted eligibility/conventions/intake/labels/report checks pass; product Linux/Windows jobs are skipped.

@fogydes

fogydes commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Reviewed; the lifetime fix is correct. vkCreatePipelineCache copies pInitialData synchronously, so freeing disk.contents right after the create is spec-legal, and the only other reads of disk afterwards use disk.header — a byte-copied FileHeader member, not a view into contents. GetPipelineCacheData (both the size query and the payload copy) runs strictly before destroyMerged.Reset(), so nothing holds a reference into the released bytes across the free. The added Reset() (and the destructor calling it) also makes the double-destroy impossible, and every early-return path still runs the RAII destructor.

One detail caught my eye and holds up: the new mergedWithFile bool is required, not cosmetic. Reset() nulls merged before the final "saved ... KiB" log line, so the old merged != VK_NULL_HANDLE ? " (merged with the file)" : "" condition would have flipped to false after the change — the bool preserves the old message semantics exactly (non-NULL only ever meant a successful create above). Diff-scope-wise fix(agc) reads fine; this trades a small peak-commit holding for identical cache format and output, so no perf evidence is owed.

@github-actions github-actions Bot added the agc libSceAgc* label Oct 22, 2026
@MarcZX10

Copy link
Copy Markdown
Contributor

The lifetime review and independent correctness comment address the ownership sequence: the create call consumes the payload, the temporary cache is reset after data retrieval, and early returns retain RAII cleanup. The hosted eligibility, conventions, intake, labels and report checks passed; product and relinker jobs were skipped. Current upstream main is 4777cac, while this branch is 1 ahead/111 behind (merge base 8c26eac). Although the three-way merge is clean and no intervening main commit changed PipelineCache.cpp, the required AGC evidence must use current main: please update the branch and rerun the same focused tests on current main and the updated head on the same machine/GPU with validation enabled, no shader cache, required Vulkan, loaded-layer confirmation, and VUID comparison. Refresh the Tested section with those commits. I did not run these tests locally; this environment has a full filesystem.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation and independent review look sound, but this changes the AGC driver and the repository requires paired focused validation-layer tests on current main and the PR head using the same machine/GPU. The detailed existing Windows/RTX 4090 report is based on 8c26eac, while current upstream main is 4777cac and the branch is 111 commits behind. Please update and rerun the paired suite with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, VK_LAYER_KHRONOS_validation, verify layer loading and compare VUIDs, then refresh Tested. I did not run tests locally due to the full filesystem.

@Izamaru

Izamaru commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

@MarcZX10 the branch has now been updated with the tested main integration. The new head is ae1c84b0b2ec2a4bb6858451bbd82adccb1ccd69, incorporating main 3ca07d086d5f44fbe3c00370e7f03953ab46ca1b. Its complete tree (2701b67ddb91a67f4b30eec8269d12021a5b6250) is identical to the locally tested integration. I fetched and rebuilt the published head and reran the focused tests.

On the same Windows 11 / RTX 4090 / driver 617.14 machine, main and the updated head pass 8/8 cache/library/graphics cases with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1 and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. Both logs confirm five Khronos instance-layer insertions, no missing-layer messages, no skipped cases and empty VUID sets. The description now records the exact commits, cases and comparisons; agc_driver_graphics is a CPU fixture, with native Vulkan coverage supplied by the cache/library fixtures.

The repeated four-process ABBA save probe on main versus the identical candidate tree reduced median process peak commit from 1639.246 to 1315.037 MiB, a 324.209 MiB reduction. All four serialized outputs were byte-identical. This measures save-time committed memory; it does not establish FPS, whole-game memory savings or a Yōtei loading improvement. The persistence probe enables a private cache (ANYPS5_NO_SHADER_CACHE=0) to exercise saving; the regression suite above uses 1.

I also checked #7622 at 778f35ea5fd168bc5b8dc1523acc5d1d7b4181aa: its saver-thread/MXCSR change and this save-lifetime fix merge cleanly in the shared file in both orders. Its existing main conflicts in VulkanDevice.cpp and ShaderDiskCache.cpp remain unchanged by this PR.

Limitations: focused Windows/NVIDIA validation only; no Linux, full-suite, other-vendor GPU or title runtime testing, and no runtime test of a combined #7622/#9558 build. Full Windows/Linux project CI still needs a collaborator to dispatch the Build workflow for this updated head.

@Izamaru
Izamaru requested a review from MarcZX10 October 22, 2026 13:25
@MarcZX10

Copy link
Copy Markdown
Contributor

The new paired report is strong and the measured PipelineCache.cpp patch is unchanged on the published head. I checked the live branch state: its current head is c6f2cd2db104ccb59b061a55a970a4fcb717af81, not the tested/published head ae1c84b0b2ec2a4bb6858451bbd82adccb1ccd69; the trees differ, though PipelineCache.cpp itself is identical. Current upstream main is now caa089fd735d0a5d615279e0f567cb638ad65690; the PR is 4 commits ahead/184 behind (merge base a6646f8464140180351e4c08694dccdd55b2814d). Main has not changed PipelineCache.cpp since that merge base, but the repository still requires the branch to stay current. Please sync to current main and rerun the paired eight cases on current main and the exact final head, then update the Tested section with that head/tree. This is a current-integration request; I did not run the GPU tests locally because the filesystem is full.

Celegans12 added a commit to Celegans12/AnyPS5 that referenced this pull request Oct 23, 2026
…r#9558, hritikutekar's four committer changes)

AI-assisted: yes
Celegans12 added a commit to Celegans12/AnyPS5 that referenced this pull request Oct 23, 2026
…kutekar's committer changes)

AI-assisted: yes
@adriadam10 adriadam10 self-assigned this Oct 23, 2026
@github-actions

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 10420 core/libs/prx/libSceAgcDriver/Graphics/src/PipelineCache.cpp

@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 24, 2026 10:21

Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit bafe9f6 into boykopovar:main Oct 24, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants