Skip to content

fix(host): keep address regions reserved until discard completes - #2823

Draft
Arkex-ai wants to merge 2 commits into
NotRexed:mainfrom
Arkex-ai:fix/v1zl-1-region-retirement
Draft

Arkex-ai wants to merge 2 commits into
NotRexed:mainfrom
Arkex-ai:fix/v1zl-1-region-retirement

Conversation

@Arkex-ai

@Arkex-ai Arkex-ai commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Freeing an address region made its address available before the old pages finished being discarded. Another thread could allocate that address and then have its new payload cleared by the old discard. Freeing the lowest region also left a stale boundary that continued to restrict heap growth.

Fixes #2799. Draft: the source-level failures are reproduced; this head has not been run in the game. This does not claim a reduction in physical RAM or explain the reported multi-GiB memory pressure.

Technical changes

  • Keep a retiring range reserved until its discard callback returns, without holding the region lock through that callback; refind the entry after possible table relocation.
  • Exclude retiring entries from classification and enumeration, preserve enumeration's total-count/truncation contract, and recompute the remaining high-region boundary under the same lock as its readers.
  • Preserve empty and repeated finalization, low-hole allocation, existing page-discard/GPU-import policy, and the renderer interface.
  • Add a portable game-free regression using the actual allocator and reader functions with controlled commit/discard and native thread synchronization.

File overlaps reviewed: #2558 changes lazy heap commitment, not this lifecycle. #2478 adds another mapped-range reader; when combined, that reader must also exclude retiring entries. Neither implementation is duplicated here.

Validation

Current head: 45dfb5612c67c73613082838b5976137fea58e74. The review followup removes the unmeasured worklist row, leaving #2799 as the incidence record. It changes no host code or tests. The source/test measurements below were collected at c12c0242862beb2c4d10268265112d783438625b, based on 51da0464b4cf0c7f56b81ebd6bb309325835e175; those files remain byte-identical.

Windows, Clang 23.1.5 / llvm-mingw: the integrated regression passed 45 assertions in each of five runs. The unchanged base failed 14 behavioral assertions with the same fixture, not a compiler/setup failure. Cases include delayed discard/reallocation, callback reentry, table growth, live-range queries, holes below the heap, repeated and empty finalization, and 10,000 allocation/free cycles. The test also caught and rejected an earlier private candidate that stranded empty reservations. Strict compilation uses -Wall -Wextra -Werror.

The complete Windows game-free suite on these exact four file contents, one worker, finished 437 passed, 26 failed, 33 skipped. It is not green. Clean-base controls for those 26 failures are not yet complete, so they are not all attributed to the environment or described as pre-existing. The focused causal regression passes; hosted results are listed below, and the remaining local baseline controls are pending. Skips include platform, shader-tool and generated-game prerequisites.

The exact committed source and test files now pass three Linux GCC 14 runs, three Clang 17 runs and three Clang AddressSanitizer + UndefinedBehaviorSanitizer runs, with leak detection enabled. Each candidate run passes all 45 assertions; the same base control still fails 14 behavioral assertions. These are source-extracted CPU/native-thread fixtures, not Linux gameplay. The complete base and fixed hos_mem.c also pass strict syntax checking against the real host interface and retained local generic runtime headers; no linked game was produced. Full linked-game compilation and controlled AMD gameplay remain pending. No FPS or RAM-saving claim is made.

Historical hosted checks on c12c0242 (not the new documentation-only head): both Windows source shards and Linux source shard 1/2 passed, as did the Windows Vulkan build, Linux AppImage, SPIR-V fixtures, workflow lint, title and Builder package. Linux source shard 2/2 failed in test_release_scripts.py (test_nightly_two_runs_at_once_end_at_the_newer_commit, first=A): the fake release workflow reported HTTP 404 and mismatched checksums after retries. The address-region regression was not the failing check. An unchanged-base Linux control of that release-script test passed all 33 unit cases in one local run; that does not establish why the hosted failure occurred or turn the CI run green. Exact-head source CI, failed job.

Independent code review was received as COMMENTED, not approval. The requested in-game counters (frees, allocation skips while retirement is pending, and heap resizes enabled by a raised floor), followed by an uninstrumented run, remain unperformed; zero observed incidence must be reported as zero, not inferred from this fixture. Existing snapshot callers still need their own lifetime discipline, and the pre-existing heap-size check and subsequent heap mutation are not made a single transaction by this change.

Current-head hosted checks: Game-free source regressions, Linux AppImage, PR title, Windows builder. Their live conclusions are separate from local tests.

Hosted integration status observed at 18:09 UTC: the source jobs check synthetic merge 0b70dcc373b32c6f69ce659fcbf0a3cf221733c6 (45dfb56 plus main 65a4d874), not standalone 45dfb56. Linux 1/2 fails test_readme_menu_rows.py because the menu has Skip opening but the README table lacks it; Linux 2/2 fails test_program_cache_one_variant.py with undeclared disk_zstd, kZstdHead and program_cache_payload. The address-region regression passes in the latter log. These are different failures from the older concurrent-nightly run. They remain unresolved integration failures; no green CI or clean-main reproduction is claimed. AppImage and Windows Vulkan builds passed; other checks were still pending.

Keep retiring regions unavailable until their old pages have been discarded,
exclude them from live-range readers, and restore the remaining heap bound.
Preserve empty reservation finalization and existing discard/import policy.

Fixes NotRexed#2799

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <noreply@openai.com>
@Arkex-ai

Copy link
Copy Markdown
Contributor Author

The exact-head allocator regression now also passes GCC, Clang and ASan/UBSan on Linux. The remaining hosted failure is test_release_scripts.py's concurrent nightly-publish case (first=A), with a mock release HTTP 404/checksum mismatch; the same unchanged-base test passed all 33 cases in one local control. I have not changed or weakened that test. Could a maintainer rerun the failed Linux source shard to check whether this is reproducible? The PR stays draft pending gameplay and CI.

Failed job: https://github.com/NotRexed/Hookshot/actions/runs/43249026786/job/131289446554

@ghost ghost left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a small, careful fix and the logic checks out. Here's what I went through:

  • Reuse while discarding: fixed. Allocation's first-fit still treats a releasing entry as taken, so nobody else can get that address until the discard returns. After relocking, the entry is looked up again by address, which handles the table being reallocated or shifted in the meantime. The discard still runs outside the lock. It has to, because the Linux path asks the renderer whether the pages are imported.
  • Released twice: can't happen. A second or concurrent free of the same address skips the releasing entry and returns, so only one discard runs.
  • Windows and Linux: the change is in the bookkeeping above the per-platform discard, so it covers both. On Windows, DiscardVirtualMemory/MEM_RESET leave the contents undefined, and on Linux, MADV_REMOVE or the zeroing of imported pages wipes them. Either way the new owner's data was at risk, and both are covered. The large-pages and HOS_KEEP_FREED_PAGES early returns are unchanged.
  • Address space: an entry is held only for the length of one discard call, so nothing builds up over a session. The 10,000-cycle churn case covers the table size too.
  • GPU imports: this path is unchanged; rx_guest_imported is still asked at discard time. #2795 changed when the Vulkan side gives back a retired pool's import and doesn't touch this code.
  • Test: it uses the real allocator and readers, with a discard stub that blocks, so it reproduces the real ordering at the bookkeeping level. The platform discard is stubbed out, which is fine for a check that runs without the game.

To take it out of draft (rule 3c, #2712): the bug needs to be shown in the game, not only in the test. A temporary log line is enough. Over a normal session, count the address-region frees, the allocations that had to skip a releasing entry (the race actually being hit), and any heap resize that succeeds only because the floor went back up. Then run this head with the log removed and report no regressions, with the GPU, driver and OS named and a row in tested-configurations. If the race counter stays at zero over a long session, please say so. The change is still worth having as hardening, but then the worklist score should drop (see the inline note).

CI: the only failure is the concurrent nightly case in test_release_scripts.py on ubuntu part 2/2. This diff doesn't touch the release scripts, and test_address_regions.py passes on both platforms, so I'd expect a rerun to clear it. There are no merge conflicts.

Overlaps: #2478 adds another reader of the region table, and it will need to skip releasing entries, as you noted. #2558 (lazy heap commit) now has to handle heap growth into space that used to belong to an address region. Since that space has already been discarded, it should just work. This PR is the smaller one, so I'd land it first and rebase both drafts onto it.

Comment thread host/hos_mem.c
break;
}
}
region_floor = ~0ULL;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part the game can actually see. The floor used to only go down, so a heap resize after the lowest region was freed got refused. Now it can succeed. That's the intent, and it's correct (holes below the heap are left out, as in the allocator). Please log it in the in-game run, so we know whether the game ever does this and that it behaves when it does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation boundary remains explicit: no game run has established how often these paths are reached. The PR stays draft. The required temporary observations are frees, allocation skips while a range is retiring, and heap resizes admitted only by the raised floor, followed by an uninstrumented run. None of those counters has been measured yet, so the allocator fixture is not presented as a player fix or a RAM-saving result.

Comment thread docs/worklist.md Outdated
|---:|---|---|---:|---:|---|---|
| 5.6 | RAM and VRAM fill up over play time until the game freezes the PC, swaps, or stops with "Out of memory (RAM plus the page file)"; fps fades and a restart fixes it. The most reported problem after v2.1.0: 13 forum threads from v2.1.0's release (Nov 10, 9:29 PM ET) to Nov 11 22:33 ET, against 4 before it; 6 GB to 12 GB cards (RTX 3080 Ti, GTX 1080, 4 GB cards) with 12-16 GB RAM, one at 20+ GB RAM. Merged after v2.1.0, so not released yet: #2206 (`RX_VK_PCACHE_LOAD=0`, opt-in), #2358, #2198, #1009, #2187, #2226, #2288; open: #2309, #2484, #2554, #2557, #2558, #2585, #2618, #2624. Next: retest on the next release with a v2.1.0-or-later log from each of the reporters | S0 | 1.0 upper bound; worst on 16 GB RAM and 4-8 GB VRAM | 0.7 | | #993 (logs from 6 commenters); #bug-reporting [[v2.1.0] Severe Memory Leak / OOM Crash in and outside Kokiri Forest](https://discord.com/channels/1568810484818378804/1568813928002224148/threads/1570044700549972008), "[v2.1.0] Out of memory ... on 16GB system", [RAM swap](https://discord.com/channels/1568810484818378804/1568813928002224148/threads/1570170903160029307), "Progressive fps lost while playing", "I have a VRAM leak...", "Frequent Freezing.", "Memory leaks.", "Excessive consumption of memoria ram", "Crash after intro and RAM issue", "FPS drop when loading into new area.", "Sudden Performance drop, restarting often solves it", "More VRAM, more shared memory...", "ram optimizations please" |
| 3.5 | Above a 60 fps Frame rate cap, frame-counted game logic misbehaves (#1612 steps each frame by its own time since v2.0.13): the Scarecrow's Song stops after its first note (a softlock where it is needed), Link loops a jump animation in dialogue after Gohma, Gohma runs into or sticks on walls, a picked-up item hangs above Link's head at 240 fps, the pick-up counter stays (fixed, see below); work-around: cap at 60. Still reported on v2.1.0 (Nov 11: Scarecrow's Song, item above head at 240 fps, Gohma on an RTX 5080, cap not stated). Related fixes merged after v2.1.0: #2006, #2093, #2409, #2470; none covers the Scarecrow's Song, Gohma or the jump loop | S1 | 1.0 upper bound; only players with a cap above 60 | 0.7 | | #1742 (#1726 closed into it); #bug-reporting "My link is jumping forever after figting Ghoma", "First boss (Gohma) doesnt work with higher FPS than 60", "Gohma boss bug with an FPS above 60", "Gohma sometime gets stuck on the wall when climbing to the ceiling.", "Bug scarecrow song", "Scarecrow Song Bug.", [The game freezes when the scarecrow song is played.](https://discord.com/channels/1568810484818378804/1568813928002224148/threads/1570176409991778334), "item pickup hanging above head at 240 fps", "water sharp and strange animated in 240fps" |
| 3.2 | Address-region storage can be reused before its previous discard finishes; a controlled two-thread regression clears the new owner's payload. Freed lowest regions also leave a stale heap boundary. Source reproduction on Windows/Linux; gameplay incidence and physical RAM savings unmeasured | S0 | 1.0 upper bound for allocator users | 0.4 in-game impact; source ordering reproduced | Arkex-ai | #2799 |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: once this PR merges, this row describes something already fixed. Either leave the row out and keep #2799 open until the in-game check is done, or point the row at this PR as the candidate fix, the way the other rows list theirs. Also, if the in-game counter shows the race never happens, S0 x 0.4 is generous for a race nobody has seen in the game.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the speculative worklist row in 45dfb56. The file now matches the PR base byte-for-byte; #2799 remains the incidence record and is unranked until a game run establishes reach/impact. This followup changes no host code or tests.

Address review on NotRexed#2823. Keep gameplay and reach unmeasured until
local hardware validation; restore the unrelated worklist bytes.

Assisted-by: GPT-6 Astra Pro
Co-Authored-By: GPT-6 Astra Pro <noreply@openai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Address regions can be reused before their old pages finish discarding

1 participant