Repository navigation
fix(agc): cache Linux describePages results - #1810
Swastik-51 wants to merge 3 commits into
Conversation
6f09745 to
36ffd37
Compare
|
I traced the current-main dependency before reviewing the cache path. On Linux, This change needs #861 first to make its intended Linux arena behavior active. CONTRIBUTING asks dependent PRs to say so explicitly; please add |
|
Thanks for adding |
|
I checked the branch base against current upstream |
8dc179c to
966d503
Compare
|
Linux results, since the PR could only be tested on Windows: this fixes a large slowdown on kernels before 6.11. Setup. Ubuntu 24.04.5, kernel 6.8 (no Before.
With the change, On kernel 7.0, where Tests on Linux (main + #861 + #1810): Tested and written by Claude (AI assistant) on a contributor's machine. |
…re-reading /proc/self/maps describePages looks pages up in a per-span bitmap before it queries the kernel, and on Linux two things stopped that bitmap from ever being filled. First, PageStates::initialize asks GuestArenaRange for the guest window, but the Arena constructor in libc is entirely inside #ifdef _WIN32, so the range comes back empty, PageSpan::allocate returns false, and spanOf always answers null. The fix falls back to the window libkernel actually reserves on Linux, 0x200000000 with a size of 0x400000000, so the bitmap exists. Second, the Linux branch queried mappings and stored nothing, while the Windows branch stores its VirtualQuery results under an allocation-generation check. The fix stores the same way, and invalidates the two munmap trims in DirectMemory that run outside any GuestAllocations::Mutation, so no readable bit can outlive the mapping it came from. Measured on this title: the map-parse path went from 1,172,045 parses per run to zero, 42,772 verifications now cost 0.1 s, forgets 715 covering 2377 MiB confirms invalidation is live, and the CPU-bound frame rate went from 7.4 to 60.3 FPS with no fatal lines over a 13 minute soak. Enabling the span also activates the invalidator, which was never registered before because it is only installed when a span allocates. The store-back half of this is already proposed upstream as boykopovar/AnyPS5#1810 for boykopovar/AnyPS5#1791. The span fallback here is only needed while this tree predates boykopovar/AnyPS5#861, which makes the span non-empty upstream; drop it once that lands.
|
Independent confirmation on a second title, with a measured frame-rate outcome, and one thing this change The measurement. A retail title ( After the store-back fix, on the same tree, same seed, same input, three runs per arm:
Zero fatal lines over a 13 minute soak with continuous input, and screenshots showed correct scenes One thing this change seems not to cover on its own. The store-back only helps if a span exists to store A second, smaller gap. Two One consequence worth knowing: enabling the span also activates the invalidator, because Written with AI assistance; the numbers above were measured on the hardware listed. |
Adapt upstream PRs boykopovar#861 and boykopovar#1810 to the existing guest mapping, alias, protection and write-watch implementation. Preserve explicit high-address mappings, retain arena reservations after unmap, and bound cached Linux page queries to the requested range. Guard permanent host intervals and validate reservation placement.
|
Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head
No title was run. 🤖 Addressed by Claude Code |
|
Conventions fails because the description isn't in the PR template: it needs the What, Tested and Checklist sections (including AI-assisted yes/no). The change itself waits on #861, since the Linux arena span is empty without it. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for this, the Linux measurements in the thread (@johnmaia, @jeffrey12cali) show it is worth landing. Ran build + the AgcDriver ctest scope here: green. What's left:
- Conventions is red (as @oneandonlydean noted): the PR body has no
### What,### Testedand### Checklistsections (template in.github/pull_request_template.md). Please move the text under those headings. - The new test never runs on Linux (
core/libs/prx/libSceAgcDriver/tests/GuestAllocations.cpp:190): it returns whenGuestArenaAvailable_nid_postfix()is false, soagc_driver_graphicsalso passes withGuestMemory.cppreverted. Either make it exercise the cache without the arena (e.g. a span that doesn't depend on #861), or land it after #861 and show it failing without the change. - Stale cached bits on the alignment trim (
core/libs/prx/libkernel/DirectMemory/DirectMemory.cpp:523-531): the prefix/suffixmunmapcalls run outside anyGuestAllocations::Mutation, soForgetPagesnever sees them. Once readable bits are cached they can outlive the unmap. Invalidate both ranges (as @jeffrey12cali offered) or explain why it can't matter. - Keep
Depends on #861: on currentmainthe Linux span is empty and this change is inactive. It can merge once #861 does.
describePages stored the whole VMA that PROCMAP_QUERY or /proc/self/maps returned. Linux merges adjacent anonymous mappings with equal flags, so an arena VMA can span GiBs, and the first table miss inside it after an invalidation rewrote every page of it. Only the pages of the request, rounded out to whole pages, are stored now, as boykopovar#1791 describes. When the guest arena is available, RunUnmappedGapTests checks that a check stores no pages outside its range, and that protecting and unmapping through the registry clears the stored pages. The registry checks follow the test on the perf/linux-page-cache branch linked from boykopovar#1791. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revoke a checked arena page with mprotect behind the registry and check that the stored answer comes back, so the test fails without the cache. Pass the gpu argument that GuestAllocations::Mutation::Add and Protect now take.
966d503 to
6f82d9f
Compare
|
@Swastik-51 Rebased onto current main and pushed to the branch; your commits are unchanged. One commit on top:
Alignment trim ( Linux, GCC, Release: ctest 694/695 on main + #861 + this branch, and on main + this branch. I also moved the description into the PR template so |
…re-reading /proc/self/maps describePages looks pages up in a per-span bitmap before it queries the kernel, and on Linux two things stopped that bitmap from ever being filled. First, PageStates::initialize asks GuestArenaRange for the guest window, but the Arena constructor in libc is entirely inside #ifdef _WIN32, so the range comes back empty, PageSpan::allocate returns false, and spanOf always answers null. The fix falls back to the window libkernel actually reserves on Linux, 0x200000000 with a size of 0x400000000, so the bitmap exists. Second, the Linux branch queried mappings and stored nothing, while the Windows branch stores its VirtualQuery results under an allocation-generation check. The fix stores the same way, and invalidates the two munmap trims in DirectMemory that run outside any GuestAllocations::Mutation, so no readable bit can outlive the mapping it came from. Measured on this title: the map-parse path went from 1,172,045 parses per run to zero, 42,772 verifications now cost 0.1 s, forgets 715 covering 2377 MiB confirms invalidation is live, and the CPU-bound frame rate went from 7.4 to 60.3 FPS with no fatal lines over a 13 minute soak. Enabling the span also activates the invalidator, which was never registered before because it is only installed when a span allocates. The store-back half of this is already proposed upstream as boykopovar/AnyPS5#1810 for boykopovar/AnyPS5#1791. The span fallback here is only needed while this tree predates boykopovar/AnyPS5#861, which makes the span non-empty upstream; drop it once that lands.
adriadam10
left a comment
There was a problem hiding this comment.
The code is fine: build and the AgcDriver ctest scope are green, and agc_driver_graphics fails with the GuestMemory.cpp change reverted, so the cache test pins the fix.
One thing left: Conventions fails with Fill in the pull request template: answer AI-assisted with yes or no. @Swastik-51 please replace <!-- yes / no --> in the AI-assisted: line of the description with yes or no. Once that check is green this can be merged.
|
@Swastik-51 I tried this in Hades II (PPSA36082) on my machine. Setup: Linux, RTX 4090, Ryzen 7 7800X3D, main aa79dbd plus a few local changes the game needs to get in game, with this PR's head (6f82d9f) merged on top. Warm caches, standing still in the hub for 20 s, one run each, same session:
That's about 27% faster. It merged cleanly onto current main. LLM assistance used. |
|
One status update from the current branch: #861 merged into main on October 10, so the stated dependency has landed. The latest conventions run still fails because the |
|
Another Hades II (PPSA36082) measurement, on different hardware from @Raghav314's. Setup: Linux (CachyOS, KDE Wayland), Ryzen 9 5950X, RTX 3080 Ti (NVIDIA 610.57.04), Release build. Base:
Before the change |
What
Fixes #1791.
On Linux,
describePagesqueriedPROCMAP_QUERY(or/proc/self/maps) but did not store the readable pages it found in the page table, so every uncached guest-range check repeated the query.PageStates::record(), shared by both platforms, to populate the cached page state.VirtualQuerypath already does.PROCMAP_QUERYresults in the page table, and does the same for the/proc/self/mapspath.Depends on #861: on current
mainthe Linux arena span is empty, so the cache stays inactive on Linux until #861 lands.Tested
python -m unittest discover tools/tests: 14/14 passed.python tools/check_conventions.py --base upstream/main: passed.git diff --check: passed.tools/check_test_registration.py(no Ninja/CMake build tree) and the Linux C++ build/tests (Windows environment).agc_driver_graphics).Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)