fix(host): retain linked synchronization objects for later reclamation #2858
Open
github-sync
wants to merge 2 commits from
github/pr/2858 into main
pull from: github/pr/2858
merge into: MagicBeanDev:main
MagicBeanDev:main
MagicBeanDev:github/pr/2893
MagicBeanDev:github/pr/2890
MagicBeanDev:github/pr/2887
MagicBeanDev:github/pr/2878
MagicBeanDev:github/pr/2875
MagicBeanDev:github/pr/2874
MagicBeanDev:github/pr/2873
MagicBeanDev:github/pr/2871
MagicBeanDev:github/pr/2756
MagicBeanDev:github/pr/2897
MagicBeanDev:github/pr/2896
MagicBeanDev:github/pr/2718
MagicBeanDev:github/pr/2842
MagicBeanDev:github/pr/2691
MagicBeanDev:github/pr/2895
MagicBeanDev:github/pr/2743
MagicBeanDev:github/pr/2765
MagicBeanDev:github/pr/2708
MagicBeanDev:github/pr/2732
MagicBeanDev:github/pr/2755
MagicBeanDev:github/pr/2817
MagicBeanDev:github/pr/2876
MagicBeanDev:release-please--branches--main
MagicBeanDev:release-please--branches--main--release-notes
MagicBeanDev:github/pr/2778
MagicBeanDev:github/pr/2888
MagicBeanDev:github/pr/2866
MagicBeanDev:github/pr/2877
MagicBeanDev:github/pr/2800
MagicBeanDev:github/pr/2829
MagicBeanDev:github/pr/2850
MagicBeanDev:github/pr/2889
MagicBeanDev:github/pr/2882
MagicBeanDev:github/pr/2860
MagicBeanDev:github/pr/2849
MagicBeanDev:github/pr/2886
MagicBeanDev:github/pr/2794
MagicBeanDev:github/pr/2840
MagicBeanDev:github/pr/2830
MagicBeanDev:github/pr/2834
MagicBeanDev:github/pr/2719
MagicBeanDev:github/pr/2742
MagicBeanDev:github/pr/2741
MagicBeanDev:github/pr/2820
MagicBeanDev:github/pr/2771
MagicBeanDev:github/pr/2859
MagicBeanDev:github/pr/2823
MagicBeanDev:github/pr/2757
MagicBeanDev:github/pr/2769
MagicBeanDev:github/pr/2844
MagicBeanDev:github/pr/2843
MagicBeanDev:github/pr/2673
MagicBeanDev:github/pr/2855
MagicBeanDev:github/pr/2739
MagicBeanDev:github/pr/2807
MagicBeanDev:github/pr/2851
MagicBeanDev:github/pr/2789
MagicBeanDev:github/pr/2814
MagicBeanDev:github/pr/2818
MagicBeanDev:github/pr/2774
MagicBeanDev:github/pr/2772
MagicBeanDev:github/pr/2750
MagicBeanDev:github/pr/2786
MagicBeanDev:github/pr/2785
MagicBeanDev:github/pr/2782
MagicBeanDev:github/pr/2779
MagicBeanDev:github/pr/2775
MagicBeanDev:github/pr/2706
MagicBeanDev:github/pr/2766
MagicBeanDev:github/pr/2747
MagicBeanDev:github/pr/2759
MagicBeanDev:github/pr/2745
MagicBeanDev:github/pr/2740
MagicBeanDev:github/pr/2733
MagicBeanDev:github/pr/2725
MagicBeanDev:github/pr/2727
MagicBeanDev:github/pr/2728
MagicBeanDev:github/pr/2729
MagicBeanDev:github/pr/2721
MagicBeanDev:github/pr/2722
MagicBeanDev:github/pr/2715
MagicBeanDev:github/pr/2714
MagicBeanDev:github/pr/2707
MagicBeanDev:github/pr/2703
MagicBeanDev:github/pr/2705
MagicBeanDev:github/pr/2697
MagicBeanDev:github/pr/2679
MagicBeanDev:github/pr/2676
MagicBeanDev:github/pr/2669
MagicBeanDev:docs/what-hookshot-connects-to
MagicBeanDev:fix/hdr-encode-test
MagicBeanDev:fix/main-tests
MagicBeanDev:fix/play-cache-files
MagicBeanDev:perf/game-barrier-order
MagicBeanDev:feat/heap-lazy
MagicBeanDev:feat/texture-detail
MagicBeanDev:perf/astc-opaque-bc1
MagicBeanDev:fix/fs-dir-lock
MagicBeanDev:fix/framegen-native-kept
MagicBeanDev:fix/update-no-downgrade
MagicBeanDev:port/k10-gcn-f16-mirrors
MagicBeanDev:fix/framegen-mult-cap
MagicBeanDev:chore/release-as-2.1.1
MagicBeanDev:perf/pcache-load-switch
MagicBeanDev:fix/prepare-progress-log
MagicBeanDev:fix/update-closes-stale-game
MagicBeanDev:fix/hdr-scaled-sampled
MagicBeanDev:perf/first-start-preset
MagicBeanDev:perf/zero-upload
MagicBeanDev:fix/vcore-escape-2s
MagicBeanDev:test/fake-gh-atomic
MagicBeanDev:perf/batch-idle-flush
MagicBeanDev:test/finish-stub-a
MagicBeanDev:chore/record-2.1.0
MagicBeanDev:fix/main-red-tests
MagicBeanDev:fix/test-display-startup-driver-cache
MagicBeanDev:feat/mesh-shader-mode
MagicBeanDev:fix/menu-probes-at-startup
MagicBeanDev:fix/fps-counter-classic
MagicBeanDev:fix/test-log-batch-flake
MagicBeanDev:fix/narrow-screen-hud
MagicBeanDev:ci/prereleases
MagicBeanDev:release/v2.0.15
MagicBeanDev:hotfix/v2.0.15
MagicBeanDev:fix/present-ahead-replay
MagicBeanDev:release/v2.0.13
MagicBeanDev:fix/bracket-paths-2013
MagicBeanDev:fix/hdr-overlays-2013
MagicBeanDev:fix/fps-counter-scale-2013
MagicBeanDev:fix/stick-seen-at-rest
MagicBeanDev:hotfix/menu-fits-screen
MagicBeanDev:fix/f1-always-opens-menu
MagicBeanDev:fix/ini-keep-lines
MagicBeanDev:claude/cranky-pike-7cf247
MagicBeanDev:perf/texture-answer-flat
MagicBeanDev:fix/stamp-test-modules
MagicBeanDev:fix/appimage-fonts
MagicBeanDev:fix/display-awake
MagicBeanDev:release/v2.0.12
MagicBeanDev:fix/discord-windows
MagicBeanDev:rc/v2.0.12-tests
MagicBeanDev:fix/dlss-keep-feature
MagicBeanDev:rc/v2.0.12
MagicBeanDev:button-icons
No reviewers
Labels
Clear labels
accessibility
Barrier affecting people with disabilities
autorelease: pending
autorelease: tagged
bug
Something isn't working
dependencies
Pull requests that update a dependency file
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
github_actions
Pull requests that update GitHub Actions code
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
testing wanted
wontfix
This will not be worked on
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!2858
Loading…
Reference in a new issue
No description provided.
Delete branch "github/pr/2858"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
A finalized synchronization object can lose its retirement record while it is still linked to a multi-wait. After the link clears, its payload buffers then remain allocated and its node cannot be reused. Keeping the record fixes that lifecycle; the review followup also prevents the collector from reclaiming it before an unlink finishes its last access.
Fixes #2804. Draft: the source-level defects are reproduced, but no gameplay incidence, physical-RAM saving or FPS improvement has been measured. This is not an explanation of the reported multi-GiB growth.
Technical changes
mw_lock.mw_lockundertable_lock: the existing reverse order remains valid. Preserve reusable-node policy and guest wait/signal semantics.docs/worklist.mdis restored byte-for-byte to the PR base, including the unrelated text encoding.Reviewed file overlaps change other paths, not the collector or this unlink ordering. Private investigation preceded the delivered claim.
Validation
Current source head:
9bba8f35ea087b8efa4cbba7f26e1b9c2b475578, a fast-forward followup tod09b08e7326fc7fa0b31d920147c90f6a4753622(basede8192345086c7c79c115fff260163508bb19c43). The old head's passing sequential tests did not cover the race found in review; they are not proof of its safety.d09b08e7; the after-publication control fails one. Existingtest_multiwait_wake.pypasses, including watcher counts through Link, Unlink, UnlinkAll and MoveAll.nn_os_sync.cpass strict Windows syntax checking with the actual host interface and retained local generic runtime headers. This is not a linked game.python -I -B tests/run_all.py --jobs 2on exact9bba8f35finished 425 passed, 4 failed, 71 skipped in 809 seconds. The exact tree and commit object are verified, with shallow history at the previous head, and source hashes remained unchanged. The failures aretest_bake_portability.py,test_hookshot_ctl.py,test_mem_gate_children.py, and the concurrent-nightlytest_release_scripts.pycase. Their baseline controls and limitations are described below; this is not an all-green result. All local suite/stress children were absent on final inspection.Historical full-suite results, explicitly not current-head certification:
d09b08e7Linux 425 passed / 4 failed / 71 skipped; earlier identical sequential-fix source on Windows 438 passed / 25 failed / 33 skipped. Three Linux failures also reproduced on unchanged51da0464: missing zstandard (test_bake_portability), a missing runtime-directory prerequisite (test_hookshot_ctl), and child RSS below the expected accounting threshold (test_mem_gate_children). The fourth was the concurrent-nightly release-script mock HTTP404/checksum failure, also seen on #2823; it passed in another run of the same release files. Its cause is not established, and none of these results is labelled all-green. Not all Windows failures have clean-base controls.Gameplay remains required: boot, scene changes and save/load, with temporary reclaimed-record/byte counts, then the uninstrumented fix. A later collection is still needed after links clear; permanently unreachable links keep their records pending. Independent review was COMMENTED, not approval. No game build or memory-saving measurement is claimed.
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: source jobs check synthetic merge
bd40671590b4e0bda4c897ad188fae3779f9b1b3(9bba8f35plus main65a4d874), whereas the local full-suite result above is standalone9bba8f35. Linux 1/2 failstest_program_cache_one_variant.pywith undeclareddisk_zstd,kZstdHeadandprogram_cache_payload; Windows 1/2 reports that failure andtest_readme_menu_rows.pymissingSkip openingfrom its README table. These files are outside this patch, but their cause on clean current main has not been reproduced here; the integration failures remain open. AppImage and Windows Vulkan builds passed; other checks were still pending.The eligibility check above (
!d->kind && !d->mw && !d->nlinks) now runs while an unlink of the same object can be running on another thread undermw_lock(on main the record was already gone by then). Plain reads ofmw/nlinkshere can seenlinks == 0beforemw->links[i] = ...has stored, and thenfree(d->links)/ the push ontosparesraces it. Suggest__atomic_load_n(..., __ATOMIC_ACQUIRE)in that check, with Unlink/UnlinkAll storingnlinks/mwlast with__ATOMIC_RELEASE. Takingmw_lockhere would invert the existing mw_lock -> table_lock order.This line is otherwise unchanged, but
instalaçaobecameinstalaçao(an encoding round-trip). Please restore the original bytes.Thanks for this, and for being upfront in the description about what's measured and what isn't. The bug is real: on main, a tombstone that's still linked at its first pass loses its only record, so it can never get back to the spare list. Keeping it in the ring is the right fix, and it doesn't touch wait/signal or lock order. I checked against #2765, #2709, #2708 and #2487: none of them change the collector, so they don't care which order they merge in.
Some things to look at before it comes out of draft:
A new race (inline). The collector reads
d->mw/d->nlinksundertable_lock, but Link/Unlink write them undermw_lock. On main that didn't matter. A multi-wait whose links were already clear at its first pass had no unlink left to run, and one that was still linked was dropped for good. This PR adds the case of an unlink on a buried object running while another thread runsbury_locked. If the collector seesnlinks == 0partway throughmw->links[i] = mw->links[--mw->nlinks], it can freelinksbefore the write into it lands, or put the object onspareswhile Unlink still holds it. The window is tiny, but this is exactly the path the PR opens. Takingmw_lockinside the collector would invert the existing mw_lock -> table_lock order (wait_any's lookups under mw_lock can take table_lock on a cache miss), so don't do that. Use atomic acquire loads in the check and have Unlink/UnlinkAll publishnlinks/mwlast with a release store. Then the collector can't see zero before the unlink is done with the object.Which unlinks actually reach a buried object. Once a holder's slot is taken,
UnlinkMultiWaitHolder(holder)looks the address up, misses the tombstone and makes a fresh holder, so the old one stays linked. A buried holder can only come back throughUnlinkAllMultiWaitHolderon its multi-wait, and a buried multi-wait only through each holder's own Unlink. That's what your two linked cases test, which is good. Records that neither path reaches stay in the ring for the rest of the session. That costs 16 bytes and one revisit every 30 s each, and the object was leaked on main anyway, so it's bounded. Still worth one line in the comment abovegraves(which still says such tombstones are "freed 30 s later").Evidence (rule from #2712). That same comment notes that freeing still-linked objects "hung most boots" once. This PR only recycles them after unlink, but it reuses memory that main never reused, so it needs a real run in the game: a boot plus a long session through scene changes (and a save/load or two), with no hang or crash. It also needs the count of records this path actually reclaims (a temporary counter or log line is fine) and the bytes involved. Without that we don't know if this ever happens in OoT, and the worklist row's S2 is a guess.
docs/worklist.md (inline). The diff re-encodes an unrelated line:
instalaçaobecameinstalaçao(UTF-8 read as Latin-1). Please restore it. I'd also hold the new row until there's an in-game number: it has no player report behind it, and "1.0 upper bound for host synchronization users" means every player.Nits:
remainingis redundant. A retained record getsnowas its time, so the loop stops on its own when it comes back around. It's fine as a guard, though, if you'd rather keep it. The test is a good model of the single-threaded lifetime (real table, real Link/Unlink, real collision burial). It doesn't cover Unlink racing the collector, which is point 1.CI was still running when I reviewed (Windows builder and AppImage pass), and the branch has no conflicts.
To come out of draft: point 1, the worklist line restored, and the in-game run with the reclaimed count from point 3.
Addressed in
9bba8f35. The collector now uses acquire loads of kind/mw/nlinks. Unlink reads mw inside mw_lock, finishes array/watch accesses, then release-publishes the cleared holder link and final count with no later access to those objects. UnlinkAll/MoveAll also publish their final cleared references/counts last. No mw_lock acquisition was added under table_lock.Two native-thread controls pause before and after publication; published
d09b08e7fails 3 and 1 assertions respectively. The new seven scenarios (40 assertions) pass in 3 Windows Clang runs and 3 GCC + 3 Clang + 3 ASan/UBSan Linux runs. An early-publication mutant fails the after-publication control, and 100 additional controlled interleaving runs pass. Negative payload reclamation is quarantined, not deliberately executed as an invalid write. Existing watcher-count tests pass on Windows/Linux. The exact-head9bba8f35full Linux run finished: 425 passed, 4 failed, 71 skipped in 809 seconds. The four failures and baseline controls are recorded in the description; the suite is not all green. Gameplay and general race-freedom are not claimed.Restored docs/worklist.md byte-for-byte to the base in
9bba8f35, fixing the unrelated encoding change and removing the unmeasured ranking row. The graves comment now says recycling requires a later collection after the grace period, and explicitly notes that a blocked record remains pending when no unlink can reach it. #2804 is unranked until in-game record/payload counts establish incidence; the PR stays draft.