Repository navigation
refactor(libc): map a run of shared guest pages as one view on Windows - #9505
Conversation
windows_shared_map_cost maps 256, 16384 and 65536 pages of shared memory (4 MiB, 256 MiB and 1 GiB) through WindowsMappings::Map, then releases them, five times each, and prints the median map and release times, the time per page and the number of mapped views. It only measures and always passes, so a build of this commit and of a later one can be compared on the same machine.
Mapping shared guest memory page by page queried the address space three times for every 16 KiB page: twice to find the placeholder the page splits from, and once more to see whether the section page still had to be committed, which it then committed on its own. For a range of fresh direct memory that is seven system calls a page, and with shared imports attached every direct mapping takes this path on the guest thread that maps it, under the mapping lock. A mapping that goes page by page now commits its section range once through one window, and splits each page off the placeholder the previous page left, whose extent it already knows; only a page after a resident run or a demotion looks the placeholder up again. A page now costs a placeholder split, the view and its protection. Tested: built; windows_write_watch gains a case that maps 256 shared pages and counts the address-space queries the mapping makes, then stores to and reads back every page. With the per-page lookups it fails (775 queries for 256 pages) and with this change it passes. agc_driver_committed_imports, agc_write_watch_coverage, agc_private_collect, agc_write_guards, agc_driver_dcc_keys_first_write, agc_driver_residency_dirty_mirrors, agc_driver_residency_driver, guest_memory and the host import, window import and alias store tests pass.
…ections Committing the section range through one read-write window before the pages are mapped works for direct and shared memory, whose sections are committable and at least as long as the mapping. A file mapping passes the bytes its file still has: the window can run past the end of a short file, and a read-only file section refuses a read-write window, so every file mapping would fail. Only mappings that pass no section size commit the range up front; file mappings keep committing each view as before. guest_memory's regular file mapping case maps a read-only file and a file tail shorter than a page on Windows. Adapts fable-void's 027bff5 to main's file mappings.
Mapping shared direct memory while shared maps are imported placed one
view per 16 KiB page: a placeholder split, a map and a protect for every
page. A 1 GiB map made about 200,000 system calls, held the mapping
thread for about 0.4 s and took and dropped the mappings mutex on every
page, so threads taking write faults queued behind it.
Map now maps each run of shared pages that need a view as one view: one
placeholder split, one map at read-write-execute (the cap later guest
protections need) and one protect per run, and builds the per-page
bookkeeping under the mutex while yielding to waiting faults. The views
move the run's page references instead of copying them, so releasing the
run does not hold the mutex. Protections and write tracking change
subpages inside the view as before. A view is split only where a release
or a remap cuts it: reset unmaps the view, maps the two pieces again over
the same section offsets and restores every 4 KiB protection, so armed
write tracking and guest protections survive the split. An alias of a
page elsewhere and a per-page protection leave the view whole.
Tested: windows_write_watch gains a case that maps 1 GiB of shared
memory (65536 views before, one after), then protects one page, aliases
one page, arms write tracking on one page, releases one page and remaps
another: the run splits only at the released and remapped pages (2 and
4 views), every other page keeps its bytes and protection, and the armed
page still faults and is collected. It fails before ("a shared gibibyte
was mapped as 65536 views instead of one") and passes after.
agc_driver_committed_imports: the 1 GiB shared map and import takes
190-196 ms on the mapping thread instead of 418-427 ms, and the longest
write fault on another thread stays at 46-97 us. Built; the write watch,
committed import, guest memory, write guard, host and window import,
residency and mapping tests pass (58 tests).
A run view maps the section at read-write-execute over the whole run and commits the run through a read-write window. A file mapping passes the protection its file allows and the bytes its file still has, so a read-only file or a file that ends inside the run cannot be mapped that way. File mappings now take the per-page path they took before the run views: one view per page at the file's view protection, with the last page cut at the end of the file. Mappings that pass no section size, direct and shared memory, still map each run as one view. guest_memory's regular file mapping case maps a read-only file and a file tail shorter than a page on Windows. Adapts fable-void's 3d695fb to main's file mappings.
|
This pull request is labeled |
MarcZX10
left a comment
There was a problem hiding this comment.
The implementation has substantial Windows coverage: the PR reports successful shared-view, write-watch, guest-memory, JIT, shm, libc/libkernel and relinker runs, plus repeated same-runner map/release measurements. Eligibility, conventions, intake, labels, report and the reported matrix checks pass. I did not run Windows tests locally.
I’m requesting changes for these items:
-
The required performance report is still missing. The
perflabel bot correctly asks for theaps5-summary.jsonllines for main and this change, traces for both, andtools/trace_digest.pyoutput. CONTRIBUTING.md requires that evidence for a speed claim. The customwindows_shared_map_costtiming table is useful, but it does not provide those report artifacts. Please attach them for the same workload and machine, or revise the performance claim and checklist to match the evidence available. -
Please complete overlap coordination for #9098 and #9159. The live overlap scan reports shared edits in
WindowsMappings.hppandWindowsWriteWatch.cpp(and the CMake test registration for #9098). The PR description says the changes compose, but does not record agreement with those authors about merge order. Please confirm the order with them; addDepends onif this PR must follow either one, then rebase/recheck the resulting combined diff. -
The test query counter adds an unconditional atomic operation to production.
WindowsMappings::query()now performsqueries.fetch_add()for everyVirtualQuery, even in the runtime library where the counter is not read. Please compile the counter only into the test target (or otherwise keep it out of production), so a performance change does not add a global atomic RMW to this hot path.
The measured view-count and map-time improvements are promising; these requests concern the required report, merge-order process, and the production cost of the new instrumentation.
The VirtualQuery counter that windows_write_watch_tests reads is now compiled only into that test, through APS5_COUNT_MAPPING_QUERIES, so the runtime's WindowsMappings::query does no atomic update per query.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
MarcZX10
left a comment
There was a problem hiding this comment.
The performance evidence and hosted product checks are now present and green, so I withdraw the earlier performance-report concern. I verified the current heads directly with git merge-tree: #9505 and #9098 conflict in core/libs/CMakeLists.txt, core/libs/prx/libc/include/WindowsMappings.hpp, and core/libs/tests/WindowsWriteWatch.cpp. In particular the shared APS5_COUNT_MAPPING_QUERIES accessor/counter hunks in WindowsMappings.hpp conflict, so the PR description’s statement that these parts merge cleanly is stale. #9505 and #9159 also conflict in WindowsWriteWatch.cpp. The contribution rule requires author-agreed order for actual shared-line overlaps and Depends on for prerequisites. Please coordinate with #9098 and #9159 authors, update this PR’s overlap account/checklist, and rerun the merge-tree/overlap check after any rebase. The content-level work appears distinct; I’m requesting changes for the inaccurate overlap/order record only. Hosted eligibility, conventions, relinker matrix, ASan and write-watch checks pass; full Linux/Windows product jobs are skipped. I did not run local Windows tests.
|
Thanks, @MarcZX10, on both reviews.
The description also gains the |
|
Thanks for documenting #9098 as a dependent rebase and #9159 as independent. I checked the remaining overlap warnings: #5021 does not change |
MarcZX10
left a comment
There was a problem hiding this comment.
The Windows mapping change is supported by broad author-reported regression coverage and a dedicated mapping benchmark. I am requesting changes for repository policy/evidence: the perf(...) title/label requires valid same-title, same-scene, same-machine warm-cache summary lines, attached base/change traces and tools/trace_digest.py output. The current mapping benchmark does not meet that title-scene report requirement, and the described Resident Evil Village pair has not been posted yet. Please attach the compliant capture report or remove the performance claim/perf labeling. Also preserve the documented #9098/#9159 order and root test registrations during rebases. I did not run tests locally; hosted product build jobs are skipped.
|
Thanks, @MarcZX10. The |
MarcZX10
left a comment
There was a problem hiding this comment.
The #9098/#9159 ordering is now documented, the runtime query counter is test-only, and the CMake registration conflicts are accounted for. I am withdrawing the earlier overlap coordination request. One repository requirement remains: this PR still has a perf(...) title and perf label, but its body still says the PPSA01556 capture pair is queued and includes no summary lines, attached traces or trace_digest.py output. The mapping benchmark does not replace the prescribed same-title, same-scene, same-machine warm-cache report. Please attach that complete capture pair or remove the perf title/claim and revise the description accordingly. No local Windows/title tests were run by me.
|
Independent Windows run of this PR, as data only. The machine is a Ryzen 9 5950X on Windows 11 (10.0.22621), built with WinLibs GCC 15.2.0 UCRT r7, Release, Focused tests, 3 runs each:
Suites:
The libkernel timeouts aren't from this PR: Not tested: a title, or a performance pair. |
|
Thanks, @MarcZX10. The Resident Evil Village (PPSA01556) pair is attached now: the summary lines, a trace per arm and |
|
@MarcZX10 the Resident Evil Village (PPSA01556) pair is attached with summary lines, traces and |
A private guest commit on Windows (every anonymous kernel map commits with a 16 KiB granule) ran reset() on each page before replacing it: 10 VirtualQuery calls, a placeholder split and the VirtualAlloc2, so 12 calls per page. At the arena's first-fit frontier the placeholder is the arena's free tail, hundreds of GiB. Commit now queries a placeholder once, cuts the range out of it (one split where the range starts inside the placeholder, one where the placeholder extends past the range), and carves the pages from that bounded placeholder: one split and one VirtualAlloc2 per page, and no split for the last page. Each page is still its own allocation with the same protection, write watch and created ranges, so munmap, MAP_FIXED remaps, views and collects see the same layout as before. A piece that would cross the end of its placeholder (a whole-range granule over several placeholders) still takes the coalescing path through reset(), and committed memory in the range still only changes its protection. A zero granule now throws std::invalid_argument before touching the address space. windows_write_watch counts the queries with a compile-time counter that only that test defines (APS5_COUNT_MAPPING_QUERIES, copied from #9505) and checks: 256 fresh pages one page into a 1 GiB placeholder take one query (main's loop makes 2,561), each page is its own allocation and the placeholders around them are intact; the release and recommit of one page; a protection-only commit; the write watch of carved pages; a whole-range granule inside one placeholder and across two; committed pieces and adjacent placeholders inside the range; a partial release and a fixed remap that keep their neighbours; a granule that would cross the end of the placeholder being carved, which takes the coalescing path; a short last piece; a commit that runs to the end of its placeholder; an unwatched commit; and the zero-granule throw. PPSA14363 boot on an RX 6700 XT, idle PC, 4 runs on main and 3 with this change: the first 10 s after the first present go from 546-552 presents with a 578-704 ms frame to 588-594 presents with a 97-117 ms frame. Co-Authored-By: Claude <noreply@anthropic.com>
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Re-checked on the current main (7a25f87): main only added test registrations to |
|
@EagleBearerYS here is a post-merge review of the run views, done by reading
I can send fixes with tests for 1 and 2; 1 has a single-threaded red test. Or would you rather take them? AI-assisted: yes (Claude Code). |
|
@thuanquang thank you for the careful read. Yes please, send the fixes with tests for 1 and 2; I'll review them, and the single-threaded red for 1 is exactly what it needs. Points 3 to 5 are noted: if you'd like to include any of them as well, that's welcome, otherwise we'll follow up on them. |
|
@EagleBearerYS both fixes are up: #11344 for point 1, which MarcZX10 has approved, and #11552 for point 2. A correction to my suggestion for point 2: mapping the halves read-only and raising them afterwards isn't possible. |
|
@thuanquang thank you for both fixes, and for the correction on point 2; good to know |
What
On Windows,
WindowsMappings::Mapmaps shared guest memory one 16 KiB page at a time. That covers direct memory while shared imports are attached,shm_openmemory and JIT shared memory. Each page costs a placeholder split, a view, a protect and threeVirtualQuerycalls: two to find the placeholder the page splits from, one to see whether its section page still needs a commit. All of it runs under the mappings mutex. A 1 GiB map places 65,536 views.This ports two of fable-void's commits from his fork (
wip/new-gpu-path, 027bff5 and 3d695fb), with his authorship. On top of them are our two adjustments formain's file mappings, a bench, and a change after review:windows_shared_map_costmaps 4 MiB, 256 MiB and 1 GiB of shared memory throughWindowsMappings::Map, then releases it, five times each. It prints the median map and release times and the number of views. It only measures and always passes.VirtualQuerycounter that 312fb24 adds forwindows_write_watchis compiled only into that test, throughAPS5_COUNT_MAPPING_QUERIES, so the runtime'squery()does no atomic update per query.mainhas had the same counter since perf(libc): carve fresh private guest pages out of their placeholder #10070, which took it from this PR so that whichever landed second would drop its copy, so after the merge withmainthe PR's diff no longer carries it.fable-void's message on 312fb24 lists
agc_private_collectamong the tests he ran. That test comes with his private-collect commit (2760ad8 on his branch), which isn't part of this PR.Measured on GitHub's Windows runner, three runs each:
mainwith the bench (6c1f636, run) against this PR (run).main: map median (views)main→ this PROn his machine, fable-void measured the 1 GiB shared map and import in
agc_driver_committed_importsat 190–196 ms on the mapping thread, against 418–427 ms before.These measure the mapping itself. A Resident Evil Village (PPSA01556) pair (under Tested) shows no measurable change in a 40 s window of its load, so this PR makes no frame-rate claim.
Overlap: checked with
git merge-treeagainst every open PR touching these files, onmaina6dcea9. Two of ours overlap, on adjacent added lines only:git merge-tree) it conflicts on a test registration incore/libs/CMakeLists.txt, on the query counter inWindowsMappings.hpp(its accessor and the count inquery(), which this PR now guards withAPS5_COUNT_MAPPING_QUERIES), and on a test function beforemain()inWindowsWriteWatch.cpp.main0ea0f25 takes it in: itsaliasDemotedreset andunreportedmarks are in both of this PR's loops, and both sides' cases run inWindowsWriteWatch.cpp(see Tested).Both are mine. Merge order: #9098 goes after this PR, lists it under Depends on, and resolves these conflicts when it rebases.
The eight PRs that conflict with
mainthemselves were checked by their diffs: none changesWindowsMappings.hpp. #6044, #7201, #7275, #8285, #8650 and #9224 add root library test registrations incore/libs/CMakeLists.txtnext to this PR'swindows_shared_map_costand thewindows_write_watch_testsdefinition. Whichever PR rebases keeps every registration, and checks after resolving thatctest -Nstill lists all of them.Tested
windows_shared_map_cost,windows_write_watch,guest_memory,guest_jit_shared_memory,guest_shm_openandguest_raise_exceptionpass ×3;main's code doesn't buildwindows_write_watchthere, because fable-void's two new cases use the query counter 312fb24 adds. His messages give their reds: "775 queries for 256 pages" and "a shared gibibyte was mapped as 65536 views instead of one".windows_shared_map_costandwindows_write_watchpass 5 runs in a row, and the libc suite passes (142). In both attempts of that run, libkernel'sguest_raise_in_system_calltimed out after 30 s near the end of the suite. Run alone 20 times in a row, it andguest_raise_exceptionpass at a2e0bf1 (run) and at 1f19ce0 (run), whilemain's a6dcea9 times outguest_raise_exceptionwithin its 20 (run).main1e20650 (run). The one conflict was the list of cases inWindowsWriteWatch.cpp'smain(), wheremainadded three private-commit cases next to this PR's two; all five run.mainnow has the sameAPS5_COUNT_MAPPING_QUERIESquery counter (from perf(libc): carve fresh private guest pages out of their placeholder #10070), so after the merge this PR no longer changes those lines. Its other added and removed lines are unchanged.windows_write_watch,windows_shared_map_cost,guest_memory,guest_shm_open,guest_jit_shared_memoryandguest_raise_exceptionpass ×10: 6 run, 6 pass, 0 skip, 0 fail;guest_memory,guest_shm_open,guest_jit_shared_memoryandguest_raise_exceptionpass ×10: 4 run, 4 pass, 0 skip, 0 fail;APS5_SKIP_TIMING_TESTS=1.main0ea0f25 (run). Both conflicts come from fix(agc): keep a rewatched import's memory compared once it is shared #9159, whichmainhas taken since:WindowsMappings.hpp, fix(agc): keep a rewatched import's memory compared once it is shared #9159 added two lines to the per-page loop this PR replaces: thealiasDemoted = falsereset after a resident alias is demoted, and theunreported = allSubpagesmark on a new view of compared shared memory. The merge carries both into this PR's per-page loop and into its run loop, where each run view takes the mark from its shared page. The PR's other added and removed lines are unchanged;WindowsWriteWatch.cpp'smain(), the list of cases keeps this PR's two and fix(agc): keep a rewatched import's memory compared once it is shared #9159's four;windows_write_watch,windows_shared_map_cost, the write-tracking tests (fix(agc): keep a rewatched import's memory compared once it is shared #9159'sagc_rewatched_direct_shared,agc_import_alias_sharedandagc_compared_shared_generationsamong them),guest_memory,guest_shm_open,guest_jit_shared_memoryandguest_raise_exceptionpass ×10: 14 run, 13 pass, 1 skip (agc_driver_compared_shared_snapshot, no Vulkan device), 0 fail. Withmain's implementation,windows_write_watchfails: "a shared mapping queried the address space for every page: 775 queries for 256 pages";agc_write_tracking,agc_write_watch_coverage,guest_memory,guest_shm_open,guest_jit_shared_memoryandguest_raise_exceptionpass ×10: 6 run, 6 pass, 0 skip, 0 fail;^agcsweep, once (run): Linux 987 run, 923 pass, 64 skip, 0 fail. Windows 1,017 run, 287 pass, 726 skip, 4 fail:agc_submit_progress,agc_submit_progress_no_timeline,agc_driver_pipelined_metadata_passandagc_driver_pipelined_metadata_pass_draw_pipelinestop atvkCreateInstance(-9) on GitHub's runner, which has no Vulkan device, and fail the same way onmainc9b2660 alone (run);APS5_SKIP_TIMING_TESTS=1, which relaxesprecise_wait's median bounds on GitHub's hosted runners.guest_memory's regular file mapping case. Those runs were on an earlier branch of this port that also carried his private-collect commit, which doesn't touch file mappings.guest_jit_shared_memoryandguest_shm_openfail there too.guest_raise_exceptiontimed out once (30 s) in an earlier run of the port. It passed in a recheck (run) and in all three runs above. This PR doesn't touch libkernel.WindowsMappings.hppis Windows-only, so Linux builds unchanged code.guest_memory,guest_jit_shared_memory,guest_shm_openandguest_raise_exceptionpass ×3, and libkernel's suite passes (197). libc's suite has one failure,precise_wait, a timing test that flakes on that runner.maind9c99c9.windows_shared_map_cost(not onmain),windows_write_watch,guest_memory,guest_jit_shared_memoryandguest_shm_openpass 3 of 3, and libc's suite passes (142).guest_raise_exceptiontimes out there with and without this PR, asguest_raise_in_host_codedoes once onmain; JJenkx's breakdown of that time is libkernel: guest_raise_exception exceeds its 30 s budget on Windows; 14.8 s of it is raises on a thread looping in sceKernelSyncOnAddressWake #9610.check_conventions.pypasses, with no comment lines added.main1ff5c8a plus four local AGC driver commits, none in libc. D is A with this PR's six commits on top (ca13192259, the same diff as this PR).APS5_LAZY_SHADER_PREPARE=1,APS5_NO_GPL=1,APS5_HOST_IMPORT_MIB=6400,APS5_TRACE_SECONDS=900:40), the same route and a warm shader cache;aps5-summary.jsonlline andtools/trace_digest.pyoutput.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)core/libs/prx/libc/include/WindowsMappings.hpp): this PR first, then perf(libc): keep no per-page physical state for resident guest pages on Windows #11152 rebases onto it