Skip to content

perf(libc): walk shared views in order and remember collectable ranges in Windows write collects - #5062

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
TideOPL:perf/collect-view-walk
Oct 14, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
TideOPL:perf/collect-view-walk

Conversation

@TideOPL

@TideOPL TideOPL commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

A clearing write collect on Windows (WindowsMappings::Collect with clear) first checks the whole range with collectable(), then walks it. Both looked up the shared-view map once per 16 KiB page (views.find(base)). A title that maps gigabytes of direct memory as shared views has hundreds of thousands of entries in that map. A collect over a 36 MB render target then costs thousands of tree searches, each a chain of cache misses, before any page is reported.

  • Walk the views in order. Collect and collectable() take one lower_bound at the start of the range and step the iterator forward (viewAt). A jump past a clean range or a private region falls back to lower_bound. The pages reported and the order are unchanged.
  • Remember ranges proven collectable. collectable() keeps the ranges it proved, merged, in collectableRanges. A clearing collect inside a proven range skips the check. Every call that can change the answer clears the set: Commit, Map, SetProtection and reset (behind Reset and Commit's placeholder replacement). Pin, Unpin, HandleWrite and the host-write calls change neither view membership nor a view's protection, so they leave it.

#4875 also edits Collect, but a different part: the GetWriteWatch buffer sizing on the private-memory path. The two touch different lines.

Tested

  • System: Windows 11 (26200), Ryzen 7 7800X3D, RTX 5070, WinLibs GCC 15.2, Release, BUILD_TESTING=ON, rebased on main 872f523.

  • Suites: after the rebase onto 872f523 (it walks the resident views from fix(libc): track writes to singly mapped direct memory without faults on Windows #5596 through the same in-order lookup, and keeps fix(libc): forget a released section's pages even while views remain on Windows #5730's shared section slots), with every test executable rebuilt: the full ctest passes 1140 of 1140 (skips as on main), including ctest -R "guest_memory|write_watch|libc|libkernel" (9 of 9) and ctest -R "^agc" (494 of 494, agc_driver_stencil_ref_export skipped as on main).

  • No new test. The change only affects speed: the pages reported are the same, so no test can fail on main and pass here. The existing CheckFailedCollectKeepsWrites in guest_memory proves a shared range collectable, mprotects half of it to no access, and requires the next clearing collect to fail and keep the write. With the clear in SetProtection removed, it fails at GuestMemory.cpp:1363, so it guards the cache.

  • tools/check_conventions.py --base origin/main passes.

  • Minecraft Dungeons II (PPSA16064), gameplay at the Squid Coast spawn, standing still. This is not a clean-main A/B: the title needs patches that are not in main yet to reach gameplay (perf(agc-driver): join the imports of adjacent large ranges when a buffer is bound across them #4515/perf(agc-driver): join the imports of adjacent ranges of any size that a buffer is bound exactly over #4517, feat(agc): depth planes sampled as 2D arrays, cubes and integer views #425 and local ones). Both sides are the same build with and without each step, captured with the feat(agc): record a present window as a Chrome trace and a summary line #4526 trace, APS5_TRACE_SECONDS=25:15 opened 25 s after the HUD appears. Each A/B had a warm-up boot per side and interleaved runs.

    • Stack samples of the graphics queue worker put about 8% of its time in write collects. About 70% of that was the per-page map search in libc.prx, not Windows. After both steps the collect samples fell from 461 to 254 in comparable runs; what remains is mostly VirtualProtect re-arming written views.
    p50 p99 presents / 15 s
    before 107.2 / 107.6 ms 117.1 / 117.2 ms 139 / 139
    walk in order 102.9 / 104.3 ms 114.9 / 115.3 ms 146 / 144
    p50 p99 presents / 15 s
    walk in order 102.7 / 100.6 ms 111.9 / 124.2 ms 146 / 147
    + proven-range cache 100.0 / 99.8 / 98.5 ms 116.5 / 113.0 / 112.4 ms 147 / 150 / 152

    About 6% at p50 together. I can attach the traces.

  • Not run: Linux and macOS (the change is in the Windows-only WindowsMappings), AMD, clean main in a title; the MCD2 numbers below were measured before this rebase and were not rerun.

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
  • GPUs tested: n/a (libc only; the AGC suites were run on an NVIDIA GeForce RTX 5070, driver 610.88, Windows 11)
  • AI-assisted: yes. Written with AI assistance (Claude Code); I reviewed and tested it.

@github-actions github-actions Bot added the libc label Oct 13, 2026
@xXPBSXx

xXPBSXx commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Review note (static): collectableRanges is cleared in Commit, Map, SetProtection and reset, but collectable() also accepts private regions that are MEM_COMMIT and MEM_PRIVATE. munmap in core/libs/prx/libkernel/DirectMemory/DirectMemory.cpp (the non-arena branch, VirtualFree(addr, len, MEM_DECOMMIT)) and munmap_release (MEM_RELEASE) change that state without going through WindowsMappings. If a proven range can contain such memory, a later clearing Collect would skip the check that used to return false for decommitted pages. Is that path unreachable for collected ranges (for example because only arena memory is collected), or should those two calls also invalidate the set? CheckFailedCollectKeepsWrites covers the SetProtection case only.

AI-assisted (Claude Code).

@TideOPL

TideOPL commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking. That path can't reach a collected range:

  • The write tracker only covers the guest arena: PageStates/the tracker take their range from GuestArenaRange (coverage.Initialize(base, size) in GuestMemory.cpp), and Watched/covers require it, so Collect is only ever called on arena memory.
  • In DirectMemory.cpp both calls are on the non-arena side: munmap runs VirtualFree(addr, len, MEM_DECOMMIT) only when KernelArena::Contains(addr, len) is false, and munmap_release is only called in the else of the same check. Arena memory is unmapped through GuestArenaReset, which reaches WindowsMappings::reset and clears collectableRanges.

So no proven range can contain memory those two calls change. CheckFailedCollectKeepsWrites covers the SetProtection case because that is the one way an arena view becomes uncollectable without a reset.

AI-assisted (Claude Code).

@CnRJay

CnRJay commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Heads-up: #3864 changes the same code. It keeps an iterator hint for the view lookup, skips ranges already in cleanRanges in collectable(), and remembers clean pages once per run in Collect. The view-lookup change duplicates this PR's in-order walk; the other two parts differ. Whichever merges second will rebase.

@TideOPL

TideOPL commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor Author

All good, I will rebase. I imagine your PR will be merged first.

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks for coordinating. I can’t predict maintainer merge order, and I don’t think either change is a functional prerequisite for the other. #5062 is clean on current main with its listed checks completed; #3864 still needs its requested rebase and a repeat of the original PPSA28416 measurement on the new base. If #5062 merges first, please rebase #3864 onto it; if #3864 merges first, I’ll rebase #5062 and retain both changes.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@TideOPL
TideOPL force-pushed the perf/collect-view-walk branch from d0cc992 to 3ea67da Compare October 14, 2026 03:21
@github-actions github-actions Bot added needs rebase Conflicts with main, rebase needed and removed needs rebase Conflicts with main, rebase needed labels Oct 14, 2026
@TideOPL
TideOPL force-pushed the perf/collect-view-walk branch from 3ea67da to b14d844 Compare October 14, 2026 08:48
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@adriadam10 adriadam10 self-assigned this Oct 14, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the diff: collectableRanges is cleared in Commit, Map, SetProtection and reset (which also covers view erasure on unmap), Collect does not mutate views while walking, so the iterator in viewAt stays valid, and the in-order walk reports the same pages. Pin/Unpin/HandleWrite/host writes change neither membership nor protection. The private-memory munmap question is answered above (the tracker only covers the guest arena). Merging once the Windows build job is green.

@adriadam10
adriadam10 merged commit 7d4f34c into boykopovar:main Oct 14, 2026
16 checks passed
@TideOPL

TideOPL commented Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

@adriadam10 thanks! The checklist was missing the newer "GPUs tested" line; it's filled in now (n/a, libc only), and check_conventions.py with --body passes on it, as does the re-run conventions check. AI-assisted is answered: yes.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants