Skip to content

perf(agc): note an address space's in-place reads by reference - #4978

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
AlxcBTW:perf/bda-read-lists-by-reference
Oct 14, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
AlxcBTW:perf/bda-read-lists-by-reference

Conversation

@AlxcBTW

@AlxcBTW AlxcBTW commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

What

An address-based build now notes the cached address space's in-place reads by reference instead of copying them into its batch.

  • GuestBufferMemory::AddressSpace builds directReads once when the space is published: the ranges its imports serve in place, sorted and disjoint. It is shared by every build that takes the space and dies with it, so it follows the registry generation, the import epoch and the device exactly as the space does.
  • GuestBufferMemory::SplitInPlaceReads() returns that list plus the build's own in-place ranges. Their concatenation is InPlaceReads(); a build that does not hold the space (or is committed) returns nothing and keeps the old path.
  • Recorder::NotePendingReadList() notes a shared list on the open batch in O(1). readOverlap walks plain reads and lists in note order (a list by bisection), so PendingReadOverlaps and DescribePendingRead answer and report the reader kind exactly as the ranges noted one by one. The same list noted again in a batch is dropped, because an earlier note is always found first. A capture trace or APS5_FLIP_READ_CHECK notes the ranges one by one as before, so trace lines and the submit-time collect do not change.
  • ShaderResources::MarkGpuWrites notes the space's list, then the build's own ranges.

No environment switch: the path is always on and exact.

Why

Every address-based build copied the space's imported ranges (about 9000 in Sword of the Sea gameplay) into its batch, roughly 44 million range notes per 10 s, only for the copy HLE's reader check to scan them. On a lab line (a fork of this code with the same hooks) the change, together with the BDA table memo that is not part of this PR, measured:

  • Sword of the Sea gameplay, RTX 5070: q0 -11.5 ms per frame, +8-9 % FPS (6.33 -> 6.89 FPS over settled segments; two more runs agree within 1 %).
  • record: marks fell from 15-19 us to 3-6 us per dispatch.
  • With only this part switched on, FPS was +5.6 % against the neighbouring base segment.
  • A verify mode that rebuilt InPlaceReads() on sampled notes and compared found 0 mismatches in 517 comparisons.

These numbers come from the lab line, not from main, and I did not capture the Performance report summary lines of main against this branch, so treat them as the motivation and not as the report the protocol asks for.

Tested

  • tests/Recorder.cpp readListTests, own ctest agc_recorder_read_list: random lists noted by reference answer every PendingReadOverlaps and DescribePendingRead query like the same ranges noted one by one, for the open batch and for the batch in flight, with plain reads of other kinds before, between and after the lists, the same list noted twice, and empty and null lists. It also checks that the queries cover several reader kinds, so the comparison is not vacuous, and that the noted-read counter adds the list's size.
  • tests/BdaResources.cpp spaceReadListTests (in agc_driver_graphics): the space's list is shared by builds while the registry stands, rebuilt when a range is added or removed, is valid (sorted, disjoint), and space + own == InPlaceReads(); a committed build and a build without the space offer no split.
  • tools/check_conventions.py --base origin/main: passed.
  • CI on the fork (Linux and Windows, same steps as build.yml): Linux and Windows green, 0 failures (https://github.com/AlxcBTW/AnyPS5/actions/runs/38358855402); the new agc_recorder_read_list passed on Linux (lavapipe) and is skipped on Windows, which has no Vulkan on the runner. Run on the commit before a rebase onto a main that only added tests/ShaderMemory.cpp
  • Not tested on hardware by me beyond the lab measurements above; no game was started for this PR.

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
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Unblocks: none (CPU time per frame in Sword of the Sea gameplay, PPSA number not recorded here)
  • AI-assisted: yes (Claude Code)

🤖 Generated with Claude Code

@xXPBSXx

xXPBSXx commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Windows test (MinGW GCC 15.2, Release, Windows 10, RTX 4080 SUPER). Built on main 534451426 together with six other open GPU PRs (their own commits applied, no merge conflicts), so only the tests that belong to this PR say something about it:

  • tests/BdaResources.cpp (target agc_driver_graphics_tests; agc_driver_graphics 10 of 10, agc_sparse_buffer_holes, agc_descriptor_pool_reset_failure, agc_compute_descriptor_layout_flags 3 of 3) and tests/Recorder.cpp (target agc_driver_recorder_tests; agc_pipeline_cache, agc_pipeline_library, agc_pipeline_library_off, agc_pipeline_library_fast_only, agc_unchanged_cpu_stamps, agc_storage_*, agc_recorder_read_list, agc_host_import_unmap and the other tests of that target): 3 of 3 passed each.

Not tested: no game run. The batch was built once, not per PR; nothing failed, so no bisect was needed.

AI-assisted (Claude Code).

@winstrex

Copy link
Copy Markdown
Contributor

Measured on a game: The Pathless (PPSA01825, 02.000.000) at the main menu, Windows 11, RTX 5070 Ti, WinLibs GCC 15.2.0 Release. Base is main c422b57 + #4491 (needed to reach the menu). Since this PR conflicts with main, I measured an equivalent commit applied on top of that base: the cached address space keeps one shared list of its in-place reads and the batch notes it by reference, once per batch, as here.

APS5_TRACE_SECONDS window of 12 s at the menu, warm caches, runs alternated base/change, 3 valid runs per side (runs lost to the Wwise crash of #361 were repeated):

frame time FPS Graphics.Draw on queue worker 0x0
base 100.3 ± 2.8 ms 9.98 ± 0.28 436 ± 17 us per draw, 69.4 ± 2.7 ms per frame
change 85.5 ± 1.5 ms 11.70 ± 0.21 347 ± 6 us per draw, 55.2 ± 1.0 ms per frame

About 159 draws per frame on both sides, so -14.8 ms per frame (+17 % FPS), with #4428 already on main.

@AlxcBTW

AlxcBTW commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @winstrex, that's a great independent number: −14.8 ms per frame (+17 % FPS) on The Pathless, a different title from the one I measured, with #4428 already on main. A rebase onto current main is on its way. The conflicts are test registration only (CMakeLists.txt and tests/BdaResources.cpp); I'll post the new head here once its CI is green.

@AlxcBTW
AlxcBTW force-pushed the perf/bda-read-lists-by-reference branch from 000aa39 to a6970f1 Compare October 13, 2026 18:24
@AlxcBTW

AlxcBTW commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. The only conflicts were in test registration (CMakeLists.txt, tests/BdaResources.cpp; both sides' tests kept). Full ctest on the fork CI is green on Windows and Linux. The only Linux failure is guest_math, which depends on the host libm (#5154).

@adriadam10

Copy link
Copy Markdown
Collaborator

Reviewed the diff: the shared directReads list is built once per published space, readOverlap keeps the note order across plain reads and lists, a repeated list is dropped, and the capture-trace / flip-read-check paths still note ranges one by one. space + own == InPlaceReads() and the by-reference vs one-by-one equivalence are pinned by the new tests, which don't compile without the change.

The only conflict with main was test registration in libSceAgcDriver/CMakeLists.txt; I rebased and kept both sides. Build and the libSceAgcDriver ctest scope pass on the rebased tree (agc_recorder_read_list and agc_driver_graphics included); agc_driver_tessellation segfaults inside RADV's pipeline-library link (libvulkan_radeon), unrelated to this change. Thanks @xXPBSXx and @winstrex for the Windows and The Pathless runs; merging once CI is green.

@adriadam10
adriadam10 force-pushed the perf/bda-read-lists-by-reference branch from a6970f1 to 57758d2 Compare October 14, 2026 11:25
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

main moved again (new tests registered next to this PR's in tests/BdaResources.cpp and tests/Recorder.cpp); I rebased and kept both sides, no logic changes. Full build and ctest pass on the rebased tree (agc_recorder_read_list and agc_driver_graphics included) except agc_driver_tessellation, which segfaults inside RADV's pipeline-library link (libvulkan_radeon), unrelated to this change. Merging once CI is green.

@adriadam10
adriadam10 force-pushed the perf/bda-read-lists-by-reference branch from 57758d2 to ccf399d Compare October 14, 2026 12:11
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
Every address-based build copied the cached address space's imported
ranges (about 9000 in Sword of the Sea gameplay) into its batch as pending
reads, only for the copy HLE's reader check to scan them. The space now
builds that list once, and a build that took the space whole notes it by
reference (Recorder::NotePendingReadList); its own ranges follow as before.

The space is immutable while its serial lives, the list equals the space's
part of InPlaceReads in its order, and the batch scans plain reads and lists
in note order, a list by bisection, so every PendingReadOverlaps and
DescribePendingRead answer and reader kind stay the same. A capture trace or
APS5_FLIP_READ_CHECK expands the list as before.

Tests: lists noted by reference answer like the same ranges noted one by one
(open and in-flight scans, kinds, a list noted twice, empty and null lists);
the space's list is shared while the registry stands and rebuilt when a range
is added or removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adriadam10

Copy link
Copy Markdown
Collaborator

main moved again (a new test name in the Windows set_tests_properties list in libSceAgcDriver/CMakeLists.txt); I rebased and kept both sides, no logic changes. Build and the libSceAgcDriver ctest scope pass on the rebased tree (agc_recorder_read_list and agc_driver_graphics included) except agc_driver_tessellation, which segfaults inside RADV's pipeline-library link (libvulkan_radeon), unrelated to this change. Merging once CI is green.

@adriadam10
adriadam10 force-pushed the perf/bda-read-lists-by-reference branch from ccf399d to 48cdce9 Compare October 14, 2026 12:34
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@adriadam10
adriadam10 merged commit 5507642 into boykopovar:main Oct 14, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants