Deferred from #157: the function-body hoisting-analysis cache (hoist_cache, added for #72) never evicts entries and pins each cached body via an owning Rc, so workloads that create many distinct dynamic function bodies grow memory without bound.
Original feedback by the Codex review bot on PR #157 (#157 (comment)):
Avoid pinning every dynamic function body forever
In programs that create and call many distinct dynamic functions, such as a loop around new Function(src)() or evaluated function expressions, this cached analysis stores a cloned Rc to every body in the interpreter-global hoist_cache and nothing ever removes entries (rg hoist_cache only finds initialization, lookup, and insertion). Those AST bodies could previously be freed once their function objects became unreachable, but now each first call retains the parsed body until the whole interpreter exits, so long-lived executions can grow memory without bound; consider a weak/evicting cache or tying cleanup to function/object lifetime.
Context: src/interpreter/eval.rs (lookup/insert, ~lines 1956–1973) and src/interpreter/mod.rs — HoistAnalysis._body: Rc<Vec<Statement>> pins the body so the Rc::as_ptr key can't ABA-collide; hoist_cache: FxHashMap<*const Vec<Statement>, Rc<HoistAnalysis>>. The cache only ever does get and insert — there is no remove/clear/retain/size cap anywhere.
Why deferred: diminishing-returns / unusual-workload — both AI reviewers confirmed the retention is real and a regression vs. pre-PR behavior, but it is not a correctness bug and the target workload (many distinct new Function/eval bodies in one long-lived interpreter) is uncommon for the project's test262-driven goal (each test runs in a fresh process). A correct bounded/evicting cache also interacts with the deliberate ABA-safety pinning invariant and warrants its own focused change plus a regression test, rather than expanding the scope of an otherwise self-contained perf PR.
Suggested fix: cap the cache size with simple eviction (LRU or capacity-based), keeping per-entry _body pinning so ABA safety still holds for entries that remain live; add a regression test that loops over many distinct new Function bodies and asserts the cache stays bounded.
Filed automatically by pm-autofix-pr after dual-evaluator triage by Claude Opus 4.8 (DEFER) and Codex GPT-5.x (FIX) — evaluator disagreement defaults to DEFER.
Deferred from #157: the function-body hoisting-analysis cache (
hoist_cache, added for #72) never evicts entries and pins each cached body via an owningRc, so workloads that create many distinct dynamic function bodies grow memory without bound.Original feedback by the Codex review bot on PR #157 (#157 (comment)):
Context:
src/interpreter/eval.rs(lookup/insert, ~lines 1956–1973) andsrc/interpreter/mod.rs—HoistAnalysis._body: Rc<Vec<Statement>>pins the body so theRc::as_ptrkey can't ABA-collide;hoist_cache: FxHashMap<*const Vec<Statement>, Rc<HoistAnalysis>>. The cache only ever doesgetandinsert— there is noremove/clear/retain/size cap anywhere.Why deferred: diminishing-returns / unusual-workload — both AI reviewers confirmed the retention is real and a regression vs. pre-PR behavior, but it is not a correctness bug and the target workload (many distinct
new Function/evalbodies in one long-lived interpreter) is uncommon for the project's test262-driven goal (each test runs in a fresh process). A correct bounded/evicting cache also interacts with the deliberate ABA-safety pinning invariant and warrants its own focused change plus a regression test, rather than expanding the scope of an otherwise self-contained perf PR.Suggested fix: cap the cache size with simple eviction (LRU or capacity-based), keeping per-entry
_bodypinning so ABA safety still holds for entries that remain live; add a regression test that loops over many distinctnew Functionbodies and asserts the cache stays bounded.Filed automatically by
pm-autofix-prafter dual-evaluator triage by Claude Opus 4.8 (DEFER) and Codex GPT-5.x (FIX) — evaluator disagreement defaults to DEFER.