Skip to content

perf(agc): reuse immutable buffer copies and batch uploads - #1364

Closed
TibixDev wants to merge 10 commits into
boykopovar:mainfrom
TibixDev:pr/retained-buffer-storage
Closed

TibixDev wants to merge 10 commits into
boykopovar:mainfrom
TibixDev:pr/retained-buffer-storage

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Status after review

Needs topic split and isolated title measurements.

The body now lists the legacy integration dependencies raised in review: #1324, #1357, #1359, #1360 and #1361. #1334 is merged and is not an open prerequisite. Current main's descriptor, snapshot and import-lifetime changes must be preserved; the historical branch copies cannot be treated as current dependencies.

The requested split remains necessary: copied-buffer version retention, batched upload pages, and shader-data streaming are three separate changes. The old combined 25 → 15.5 µs result cannot establish any one's benefit. The new main + experimental NGS Skyrim loading profile does identify expensive uploads, but it neither isolates immutable read-only reuse nor separates page batching from streaming. It is a useful lead, not before/after evidence for this head. No speculative large rebase or claim that its budget fix resolves the architectural/performance review.

What

Reuse immutable copies of read-only guest buffers. Upload small inputs and shader data into aligned batch pages. Keep each slice alive through GPU completion, preserve descriptor offsets, and invalidate cached copies only when their contents or backing change.

Tested

Earlier branch-specific tests and measurements remain in the discussion. They have not been rerun on a redesigned current-main version of this PR. The review audit and shared NGS-assisted title exploration do not establish that this head is merge-ready. No full Linux/Windows platform CI was dispatched in this pass.

Checklist

@TibixDev

TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on 1e1e1390. Preserved the #1113 batch memory budget and included retained draw-upload pages in its byte count. The upload fixture checks each buffer is counted once and that completion removes the bytes. Existing snapshot, mapping, and descriptor coverage remains intact.

Fedora 44 Release: agc_driver_graphics_tests, agc_driver_pm4_tests, guest_mapping_history_tests, agc_driver_recorder_tests, agc_recorder_kept_bytes_tests build; the targeted tests and contribution checks pass. Windows CI is pending.
RADV and lavapipe: --copied-buffers-only, --shader-data-only, --draw-upload-only, --draw-snapshot-only, --draw-descriptors-only pass with core and synchronization validation; no validation errors.

@TibixDev
TibixDev force-pushed the pr/retained-buffer-storage branch from ac6a977 to 9daffd5 Compare October 8, 2026 07:31
Use a locked ordered lookup for one containing readable allocation when the import registry is current. Recheck the registry generation after acquiring the lease, and retain the full reconciliation path when mappings change.

Tests cover bounds, gaps, read-only and inaccessible ranges, address reuse, changed protection, pinning the requested allocation, and releasing an unrelated allocation. agc_driver_graphics passes on Fedora 44. The preceding Skyrim profile attributed 76% of sampled CPU cycles to full allocation leases.

Written with AI assistance.
Bound allocation coverage and availability lookups, stage only changed mapping fragments before host operations, and retain atomic registry updates on host failure. Descriptor uploads acquire a targeted lease when an unchanged registry has no cached import.

Validation: agc_driver_graphics and guest_memory passed, including host-failure rollback, gaps, split mappings and unrelated leases. A 20000-entry mapping microbenchmark dropped from 2354 ms to 0.764 ms for 1000 localized operations; this is not a game benchmark. AI-assisted implementation.
Replace per-draw Vulkan descriptor copies with a single write from retained buffer and image metadata. Keep per-draw guest snapshots, sets and resource ownership unchanged. Baseline RX 6600 profiling attributed 28% of graphics-worker cycles to descriptor copies.

Validation: mixed descriptor and snapshot regression passes on RADV RX 6600 and llvmpipe with Vulkan validation; graphics and guest-memory tests pass. Written with AI assistance.
Extract the recorder's bounded versioned buffer cache and reuse it for
read-only copied shader buffers. Small inputs use existing batch upload
pages; larger watched ranges retain immutable buffer slices. Move recorded
read-only uploads to the recording stage and preserve descriptor offsets
for overlapping and misaligned views. Writers and address-based shaders
keep their existing ownership and publication paths.

Test CPU and driver writes, alias mappings, replacement backing at the same
address, retained GPU reads, writer overlap and buffer eviction. Four AGC
suites and targeted recorder checks pass. Matched ABBA resource batches beat
the previous implementation for 256-byte and reused 16-KiB inputs, both bare
and with eight textures. Helgen completes 441,398 decode comparisons with
zero mismatches and no Vulkan validation errors.

Written with AI assistance.
Defer small shader-data buffers until the recording stage and retain aligned
slices of the existing upload pages. Preserve slice offsets in descriptors,
alignment patches, template updates and debug reads. Large data buffers keep
their independent allocation during preparation.

Add GPU readback coverage for deferred construction, neighboring slices,
misaligned guest-buffer patches and ordered template refreshes. Four AGC
suites and the validated recorder checks pass. A matched ABBA comparison
against the prior commit improves small-data resource batches, both bare
and with eight textures; large-data costs remain comparable.

Written with AI assistance.
Carry the fixture correction already used by the binding-plan branch. Keep byte checks on both paths and require fresh copies when write watching is unavailable.

AI-assisted: yes.
@TibixDev
TibixDev force-pushed the pr/retained-buffer-storage branch from 9daffd5 to 85593a1 Compare October 8, 2026 07:58
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 8, 2026

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the work here. This can't be merged as is:

  • It is stacked on #1324, #1357, #1359, #1360 and #1361. All five are still open and conflict with main, so nothing here can land before them. The Depends on line lists #1334 (already merged) and #1360; please list the whole stack.
  • This head also conflicts with main (ShaderResources.hpp/.cpp, Recorder.cpp, GuestBufferMemory.cpp, GuestAllocations.cpp, tests/Recorder.cpp) after the recent descriptor and snapshot changes, so it needs the author rather than a mechanical rebase.
  • The gain is a component timing (16 KiB resource batches ~25 → 15.5 µs per draw, on your combined fork). Please name a title and scene where this shows up, with before/after frame times on current main; on #1324 @oneandonlydean measured no frame-time change and that question is still open.
  • At +1367 lines with a 694-line test change, please split it once the bases land (copied-buffer retention vs. batched upload pages vs. shader-data streaming) so each PR keeps one topic.

Please rebase once the base PRs are merged and we will review it again.

@TibixDev

Copy link
Copy Markdown
Contributor Author

Review-pass update:

The body now lists the legacy integration dependencies raised in review: #1324, #1357, #1359, #1360 and #1361. #1334 is merged and is not an open prerequisite. Current main's descriptor, snapshot and import-lifetime changes must be preserved; the historical branch copies cannot be treated as current dependencies.

The requested split remains necessary: copied-buffer version retention, batched upload pages, and shader-data streaming are three separate changes. The old combined 25 → 15.5 µs result cannot establish any one's benefit. The new main + experimental NGS Skyrim loading profile does identify expensive uploads, but it neither isolates immutable read-only reuse nor separates page batching from streaming. It is a useful lead, not before/after evidence for this head. No speculative large rebase or claim that its budget fix resolves the architectural/performance review.

I have updated the PR description to match this status. The PR remains open; the outstanding review requirements are not being marked resolved. AI-assisted audit.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing for now: it carries three topics on a stack, and the author said it needs a redesign. With the queue this large we're closing stalled perf PRs that don't show a gain in a title; reopen it with before/after numbers from a title scene (the summary lines from the performance report form) if you pick it up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build libc needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants