Skip to content

perf(graphics): normalize buffer ranges without temporary owners - #1345

Closed
TibixDev wants to merge 2 commits into
boykopovar:mainfrom
TibixDev:pr/buffer-range-normalization
Closed

TibixDev wants to merge 2 commits into
boykopovar:mainfrom
TibixDev:pr/buffer-range-normalization

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What

Coalesce overlapping buffer regions in place instead of allocating a second owner vector. A wholly committed memory range no longer allocates a list of partial ranges. Preserve snapshot contents, conflicting-overlap rejection, sparse heaps, and mirror refresh/writeback.

Rebased directly on main d146ec1b6. Removed the #1330 permission-cache prerequisite and the already-merged #157 copy; this branch now contains only buffer-range normalization and its tests. The newer sparse-mirror callers use CommittedRanges when they need an explicit full range. Main's sparseHeap flag and GuestAllocationsAcquireAll behavior are preserved.

Depends on: none.

Tested

Fedora 44, GCC 16.2.1, Release: built agc_driver_graphics_tests and guest_memory_tests; agc_driver_graphics and guest_memory pass (3.33 seconds total). The graphics suite includes the BDA resources and guest-allocation cases.

Regression coverage includes reordered overlapping snapshots, disjoint views, inconsistent overlapping bytes, empty/full/partial mappings, clipped bounds, read-only pages, unmapped/decommitted gaps and address reuse. The new commitment test has native Linux and Windows paths; Windows runtime was not run in this pass. Contribution checks and git diff --check pass.

Isolated measurements on Ryzen 5 5600, CPU 3, same compiler/Release flags and benchmark source. Baseline uses main d146ec1b6 production files; comparison adds only this PR. Three ABBA cycles (six processes per variant), each reporting the median of nine measured batches after one warm-up:

Workload Main, ns/call This PR, ns/call
DescribeCommitted, 0 bytes 10.82 10.59
DescribeCommitted, fully mapped 4 KiB 669.53 653.14
DescribeCommitted, fully mapped 16 KiB 668.13 651.76
UploadPrepare, 0 snapshot regions 28.00 25.55
UploadPrepare, 1 snapshot region 60.08 42.11
UploadPrepare, 8 overlapping/disjoint snapshot regions 769.82 627.25
UploadPrepare, 64 overlapping/disjoint snapshot regions 5339.56 4690.64

All checksums match. Upload preparation is timed with imports enabled and upload completion outside the measured interval; the harness uses the graphics suite's mock Vulkan functions. These are CPU component measurements, not GPU or title frame times. The 64-region run-median ranges overlap (main 5169–6126 ns, PR 4521–5412 ns); the table reports medians across all six processes, not the best run.

Command for each variant after building the same harness:

taskset -c 3 ./build-fedora/tests/agc_driver_graphics_tests --benchmark-buffer-ranges

For the baseline, use main's Execution/include/GuestMemory.hpp, Execution/src/GuestMemory.cpp and Graphics/src/GuestBufferMemory.cpp under core/libs/prx/libSceAgcDriver, keeping the benchmark source identical. Keep main's tests/GuestAllocations.cpp for that baseline build because the new assertions refer to partialRanges. The benchmark mode does not run those assertions.

Checklist

  • Based on main at the revision above; prior prerequisites removed.
  • One topic.
  • No new code comments.
  • Unsupported paths retain their errors.
  • No notes, logs, or images added to the repository.
  • No new third-party code.
  • General behavior.
  • Depends on: none.
  • AI-assisted: yes.

@github-actions github-actions Bot added libc agc libSceAgc* labels Oct 8, 2026
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 8, 2026
@Mouhand-Kaddo

Copy link
Copy Markdown
Contributor

Additional platform validation on standalone PR head 7e8c2c1c1ca0bb0c3e1cfb42794288e3c1890ed7:

Windows: passed — Windows 11, supported WinLibs GCC 15.2.0 UCRT/POSIX/SEH r7; CMake 4.3.1 / Ninja.

Release / BUILD_TESTING=ON: built the selected test targets; 2 CTest cases, 0 failures, 0 CTest skips.

Tests: agc_driver_graphics, guest_memory.

Reproduce after building the test targets: ctest --test-dir <build> --verbose --no-tests=error --timeout 30 -R "^(agc_driver_graphics|guest_memory)$".

Vulkan: native AMD RX 6900 XT, driver 32.0.21045.5002.

Integration limitation: a local merge against main 948a526c2 has source conflicts; these results validate the standalone PR head, not a resolved merge.

@blawesom

Copy link
Copy Markdown
Contributor

Hi! This PR currently conflicts with main, so I rebased its 3 commits onto the latest main. Beyond the mechanical conflicts, main has evolved the same code since, so the rebased branch adapts a few things:

  • GuestAllocations: main added GuestAllocationsAcquireAll where this PR adds TryDescribeRange/AcquireRange — both kept.
  • Commitment::ranges → partialRanges (this PR's rename) now also applies to main's newer refreshSparseMirror and two other DescribeCommitted(...) call sites.
  • GuestBufferMemory: main's new sparseHeap region flag is kept alongside the PR's partialRanges backing.
  • The test calls were updated for main's new gpu parameter on Mutation::Add/Protect (passed false for the plain test pages).

It builds, and agc_driver_bda_device_tests, agc_driver_graphics_tests, agc_driver_mesh_tests, agc_shader_memory_tests and agc_write_tracking_tests all pass locally.

The rebased branch is available to pull:

git fetch https://github.com/blawesom/AnyPS5 rebase/pr-1345
git reset --hard FETCH_HEAD

Feel free to ignore this if you'd rather rebase yourself.

@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. This is stacked on #1330, and both conflict with main now. main changed the same code since (GuestAllocationsAcquireAll, the sparseHeap region flag, refreshSparseMirror), so the rebase isn't mechanical.

  • Please rebase on main once #1330 is in (the #157 commit is already merged, drop its copy). @blawesom mentions a rebased branch; it isn't pushed here, so the head I looked at is still 7e8c2c1.
  • Re-run agc_driver_graphics/guest_memory after the rebase, including the new BdaResources and GuestAllocations cases.
  • The description says "perf" but has no numbers. Please add before/after timing for the UploadPrepare/DescribeCommitted path (same tree, several runs), as in #1330.

The change itself reads fine: storeChanged returns early on whole, and every other partialRanges use is guarded by !whole.

Keep full commitment checks allocation-free and coalesce owned regions in place while preserving partial mappings and overlapping snapshot validation.

Written with AI assistance.
@TibixDev
TibixDev force-pushed the pr/buffer-range-normalization branch from 7e8c2c1 to d44adcb Compare October 12, 2026 03:44
@TibixDev

Copy link
Copy Markdown
Contributor Author

@adriadam10 Updated to d44adcb4e, based directly on main d146ec1b6. The #1330 permission cache and the already-merged #157 copy are no longer included; Depends on: none.

Preserved main's sparseHeap and allocation-lease handling. The newer sparse-mirror refresh/writeback callers now use CommittedRanges where they need an explicit full range. Added independent commitment tests for empty/full/partial mappings, clipped bounds, protection changes, holes and address reuse, alongside the overlapping-snapshot regressions.

Fedora 44 Release: agc_driver_graphics and guest_memory pass, including the BDA/guest-allocation cases; contribution checks pass. Windows runtime was not rerun.

Added a reproducible --benchmark-buffer-ranges mode. Comparing main production code with only this PR, using identical benchmark source and flags, six pinned processes per variant in three ABBA cycles: 4 KiB/16 KiB DescribeCommitted medians went 669.53/668.13 → 653.14/651.76 ns; UploadPrepare for 1/8/64 snapshot regions went 60.08/769.82/5339.56 → 42.11/627.25/4690.64 ns. Checksums match. The description includes empty-input results, run variability, the command and baseline setup. These are CPU component timings with mock Vulkan, not title frame times.

Ready for another review.

@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 update. d44adcb4e still conflicts with main (GuestBufferMemory.cpp, tests/Graphics.cpp, tests/GuestAllocations.cpp), and it isn't mechanical:

  • GuestBufferMemory::UploadPrepare on main now sorts an order index vector and walks regions[order[i]] (the merge loop starts at GuestBufferMemory.cpp:2759). The in-place merged iterator compaction needs redoing against that loop, with the mergeBacked sparse handling kept.
  • addDescriptorRegion (~GuestBufferMemory.cpp:2413) has a new committed.ranges use that has to move to partialRanges.
  • --benchmark-buffer-ranges collides with main's --sparse-buffer-holes entry in tests/Graphics.cpp; keep both.
  • GuestAllocations.cpp: RunGuestAllocationTests now calls leaseSnapshotTests(); keep it next to committedRangeTests().

After the rebase, please re-run agc_driver_graphics and guest_memory and redo the before/after table against the current main, since the merge loop you measured has changed. Consider keeping the benchmark harness out of the test binary (a separate target or the PR description), as it is ~50 lines of non-test code in Graphics.cpp.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing for now: no title or measured gain is shown. 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* libc needs rebase Conflicts with main, rebase needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants