Repository navigation
Fix two unbounded-growth paths in copyTableSafe and SwitchAttributeNode - #2565
Open
p11primera wants to merge 2 commits into
Open
p11primera wants to merge 2 commits into
p11primera wants to merge 2 commits into
Conversation
copyTableSafe kept its sub-table memo in a module-level table that was only wipedTable'd on the SUCCESS path of the outermost call. Any copy that never returned -- a coroutine abandoned while suspended mid-copy, or a copy aborted by an error -- left its memo populated forever, pinning the entire source sub-tree plus every mirror already built. Across a session these leaked memos grow the Lua heap unbounded until a memo write throws 'table overflow' (LuaJIT LJ_MAX_HBITS) and the post-error full GC stalls for minutes. Thread the memo through the recursion as a parameter so it is created by the outermost call and becomes garbage the moment that call returns, including the abandoned/error paths. Behaviour on the success path is unchanged: shared sub-tables still collapse to a single copy via memo identity within one copy operation.
…ions
Every call deep-copied the SHARED passive-tree node and attached a
brand-new mirror generation (ReplaceNode + ProcessStats); BuildAllDepends
AndPaths then projects that override back onto the shared node. So each
re-switch DOUBLES the node's reachable graph. Repeated allocate/
deallocate cycles over an attribute node -- weapon-set switching, build
import, tree reload while overrides are retained -- accumulate
generations: copy cost and heap grow exponentially until one copyTableSafe
crosses LuaJIT's hard 2^26 hash-node limit and throws 'table overflow'.
Two guards:
1. Idempotence -- record _attrOptionIndex on the override and no-op
when re-switching to the same option.
2. Generation cache -- build at most one mirror per (nodeId, option)
and re-attach the SAME object when the override map is reset (import
and class-change wipe hashOverrides; DeallocNode nils entries). A
shared node stays at one generation per option. ReplaceNode /
ProcessStats only re-project shared nodes FROM these overrides (an
idempotent re-projection), so reusing the cached object is safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two independent memory-growth fixes found while profiling repeated tree allocate/deallocate cycles. Both manifest the same way — Lua heap growing unbounded until a
copyTableSafememo write throwstable overflow(LuaJITLJ_MAX_HBITS, hard 2^26 hash-node limit) and the post-error full GC stalls for minutes.1.
copyTableSafe— abandoned copies leaked their memo (Common.lua)The sub-table memo lived in a module-level table that was only
wipeTable'd on the success path of the outermost call. Any copy that never returned — a coroutine suspended mid-copy and later dropped, or a copy aborted by an error — left its memo populated permanently, pinning the whole source sub-tree plus every mirror table already built. Thread the memo through the recursion as a parameter so it is created by the outermost call and garbage-collected the moment that call returns, including the abandoned/error paths. Success-path behaviour is unchanged (shared sub-tables still collapse to one copy via memo identity).2.
SwitchAttributeNode— re-switches doubled the shared node every call (PassiveSpec.lua)Each call deep-copied the shared passive-tree node and attached a new mirror generation (
ReplaceNode+ProcessStats);BuildAllDependsAndPathsthen projects that override back onto the shared node. So every re-switch doubles the node's reachable graph. Repeated allocate/deallocate over an attribute node — weapon-set switching, build import, tree reload while overrides are retained — accumulate generations until one copy crosses the 2^26 limit.Two guards:
_attrOptionIndexon the override; no-op when re-switching to the same option.(nodeId, option); when the override map is reset (import/class-change wipehashOverrides,DeallocNodenils entries) re-attach the same cached object, so a shared node holds at most one generation per option.ReplaceNode/ProcessStatsonly re-project shared nodes from these overrides (idempotent re-projection), so reusing the object across specs is safe.Reproduction / evidence
Diagnosed on the tree-optimizer branch (which drives thousands of alloc/dealloc cycles in droppable coroutines) with
sample/vmmap/in-process probes on macOS arm64 + Linux:lj_tab_*/ GC frames, 0 trace/mcode frames.memo = 67108864(= 2^26 exactly) while the widest table copied had only 30 keys; caller wasSwitchAttributeNodein every capture. Post-memo-fix failure rate was still ~24%; the generation cache is what closes it.The fixes are self-contained to these two files and do not depend on the optimizer. Happy to split into two PRs if you'd prefer one fix each.