Repository navigation
fix(agc): hash the pages of an import window instead of copying them - #7981
Conversation
|
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.
|
|
One overlap detail for the integration note: the overlap check also flags open #6308 in |
|
#6308 was already in the related notes, but I've made it clearer that its unwatch hook edits the same AI-assisted: yes |
|
A run on a current-main base, for honesty about where this matters. Base: The largest 10 s host memory jump during the level load was +1.86 GB with this PR, against +1.69 GB and +2.08 GB in two runs of the same base without it. So at main's normal load speed, Demon's Souls does not show the multi-GB burst, and this run shows the change is safe there but not a measurable benefit. The +3 to +19 GB jumps in the description come from the faster loads with the job-idle patch. My guess is that several of the 0.5 to 3 GiB imports overlap there, but I have not measured that. The copy itself is still on main (the footprint test fails there with "held 64 MiB"), so a title or machine that loads faster can still hit it. AI-assisted: yes |
f896e7a to
9a22b0f
Compare
|
Update: I discarded the first tracker-level approach because it conflicted with #6308 and #7505. The current local two-commit candidate instead filters coalesced runs in |
|
Thanks for your patience on this. #8172 (the fix for the #8103 regression) has merged, and it also rewrites The two changes fit together: #8172 splits a partly watched range into runs of covered 64 KiB blocks that stay watched, and your per-page hashing can apply within those spans. Could you rebase onto current main and redo your change on top of #8172's version of that code? We'll review the rebase and run CI on it as soon as it's up. |
|
Rebase handoff — rebased onto current
Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release, To pick it up: Head is |
9a22b0f to
4d893a9
Compare
|
Rebased onto current main (75787d2), on top of #8172. New head is 4d893a9. I used @blawesom's resolution as is. Thanks for doing the merge. It keeps #8172's span flow (covered 64 KiB runs, the Built on Linux and AI-assisted: yes |
9f1fffd to
68fba51
Compare
ImportWatched kept a copy of every non-zero page of the range being imported, so that a page the write watch reported during the import could be told from one that still held its bytes. The copy had the range's footprint: a first import of a 0.5-3 GiB guest heap held that much host memory while it ran. The window now keeps a 64-bit digest per 4 KiB page (four xxHash64 lanes) and compares digests: 8 bytes per page, 1/512 of the range. Same decision for every page, same reads of guest memory. CheckImportWindowFootprint imports a 64 MiB watched range and requires the heap held during the import to stay under 1 MiB. It failed with "held 64 MiB" before this change.
68fba51 to
ed921e2
Compare
|
Checked the diff against main (on top of #8172's span flow): |
What
ImportWatchedkeeps a copy of every non-zero page of the range it imports, so that afterwards it can tell a page the write watch reported from a page that still holds its old bytes (#3874, #5698). The copy is as big as the range. A first import of a 0.5 to 3 GiB guest heap holds that much host memory while the import runs (#7980).With this change the window keeps a 64-bit digest of each 4 KiB page (
HashPage, four xxHash64 lanes merged the way xxHash64 merges them) andWindowCopy::Holdscompares digests rather than bytes. That is 8 bytes per page, 1/512 of the range: a 3 GiB import holds 6 MiB rather than 3 GiB.Fixes #7980
ZeroPageandWindowCopy::Zeroare removed.Tested
CheckImportWindowFootprintinagc_write_tracking(glibc only, since it readsmallinfo2). It imports a 64 MiB watched range, all non-zero, and requires the heap growth seen inside the import to stay at or below 1 MiB. On main it fails withan import window over 64 MiB held 64 MiB of host memory during the import. With this change it passes. It then re-imports the range, writes the same byte to one page and a new byte to another, and checks that the first page reads as unchanged and the second as written.agc_write_tracking,agc_import_watchand the other 50 tests run byagc_write_tracking_testsandagc_driver_recorder_tests(52 in all) on this branch, based on main dbac9fa, Linux, Release, GCC 16. On lavapipe (Mesa llvmpipe): 47 pass and 5 are skipped for missing capabilities. On an NVIDIA GeForce RTX 5070 Ti (driver 595.91.07): all 52 pass.APS5_JOB_IDLE_SLEEP=1, which loads faster and makes the load burst sharper. I compared the same set with and without this commit and sampled host memory every 5 to 10 s:ImportWatchedallocation. Another had a 2.9 GiB anonymous block appear in the 10 s before the host OOM killer stopped it.Related
ImportWatched's unwatch and save paths near these lines (fix(agc): run the unwatch hook when a watched import leaves the write watch #6308's unwatch hook edits the same fallback block). None of them replaces the copy, so they don't overlap with this, but whichever lands second may have a textual conflict. I'll rebase if needed.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); this change adds no stubs.mdfiles or images added to the repository (attach them to this PR)