Skip to content

fix(codegen): release the inner-container temporary of a chained subscript read#524

Open
mirchaemanuel wants to merge 2 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/516-chained-subscript-leak
Open

fix(codegen): release the inner-container temporary of a chained subscript read#524
mirchaemanuel wants to merge 2 commits into
illegalstudio:mainfrom
mirchaemanuel:fix/516-chained-subscript-leak

Conversation

@mirchaemanuel

Copy link
Copy Markdown
Contributor

Fixes the dominant leak behind the latency ramp in #516: a chained subscript read $a[$i][$j] on a nested container leaks the materialized inner container on every evaluation.

Root cause

Index-read results are owned (+1) for refcounted and Mixed elements (the array_get/hash_get emitters incref pointer payloads and box Mixed cells) and borrowed for strings. The hoisted form $inner = $a[$i]; $inner[$j] releases that +1 through the local-slot retaining-store machinery — but a chained read's intermediate never touches a local slot, so its reference is never dropped. Under --web this compounds request after request until the fixed heap is exhausted (Fatal error: heap memory exhausted); the latency ramp is the allocator's first-fit free-list scan growing with the leaked live set.

The fix (IR-level, target-independent)

  • src/ir_lower/expr/mod.rs: in lower_array_access_from_value, after the consuming read has extracted its result, emit a release of the receiver when it is an owned index-read temporary. Lowered by the existing target-aware lower_release, so all three supported targets are covered by the same change.
  • src/ir_lower/context.rs: new predicate value_is_owned_index_read_temp — defining op is one of ArrayGet/ArrayGetSilent/HashGet/ArrayGetMixedKey/ArrayGetMixedKeySilent AND the result type is refcounted-non-Str or Mixed/Union. String results are excluded (borrowed pointers into the container payload, nothing to release). The emission is additionally gated by release_if_owned, which skips Borrowed/NonHeap/Moved values.
  • Safety of the release point: the parent container still holds its own reference to the intermediate, so releasing the +1 cannot free storage the just-extracted result may borrow (e.g. a string element pointing into the inner array's payload).

Also included (verification tooling, second commit): --gc-stats now emits its counters once per request under --web — previously it printed nothing there because the report only lives in the exit-based main epilogue, which a --web worker never reaches. One doc paragraph added. This is the instrumentation that made the diagnosis and the numbers below possible.

Evidence

Minimal repro (2,000-element array of [string, string] pairs, chained read per element):

before after
CLI --heap-debug leak report 6,000 live blocks / 400,056 bytes clean (allocs == frees)
--web --gc-stats, live blocks across 20 requests +6,000/request constant
output correct identical

Real-world app (ivory, the #516 measurement subject, GET /p/{id}, no recycling):

before (main e63d5d337) after
net leak ~6,252 blocks/request ~2,655/request (−58%)
worker crash heap memory exhausted at ~request 92 none within 120 requests
response byte-correct byte-identical (26,373 bytes)

The remaining ~2.6k/request in that app comes from a different, pre-existing source (it does not reproduce through chained subscripts; see notes) — #516 stays open for it.

No-regression sweep (all byte-correct, no crashes): hoisted form still clean; $a[$t][0] . $a[$t][1] 300→0 blocks; string-keyed $m['x']['y'] 8→0; three-level chain 32→2 (the residual 2 is a pre-existing leak in building the triple-nested structure, present on clean main); isset() chains 150→0; foreach and nested-write behavior unchanged (their pre-existing quirks are identical before/after).

Tests: 3 regression tests added in tests/codegen/runtime_gc/regressions.rs (fail pre-fix, pass post-fix). Focused suites all green: runtime_gc 125, arrays 337, array_basics 81, foreach 103, isset 20, nested 118, nullsafe 21, ir_backend_smoke_test 255; web_tests 40/41 — the one failure reproduces byte-identically with the pristine-main compiler (pre-existing local flake, not from this change). cargo build/--release clean under the zero-warnings gate; EIR validator (dev profile) clean.

Verified on macos-aarch64; the change is IR-level and lowered through existing dual-arch paths, but Linux targets were not run locally — relying on CI for the matrix.

Pre-existing issues surfaced while testing (unchanged by this PR, listed for triage)

  • nullable-receiver chained reads (?array) leak through a separate hidden-temp path;
  • an inner-index miss on refcounted elements segfaults (sentinel deref) — identical before/after;
  • foreach row copies leak a constant amount per iteration;
  • pushing a nested array literal leaks 2 blocks per triple-nested build;
  • Mixed nested writes ($a[$i][$j] = ...) silently don't stick (the known array/array nested-write miscompile).

…cript read

A chained index read ($a[$i][$j]) consumes the inner read's result directly
as the receiver of the outer read. Container reads of refcounted or boxed-Mixed
elements return a +1 caller reference, and unlike the hoisted form
($inner = $a[$i]; $inner[$j]) no local-slot release machinery ever drops
it, so every materialized inner container leaked (issue illegalstudio#516: 3 heap blocks per
element, 6000 blocks per request on the reported workload).

Lowering now releases the intermediate once the consuming read has extracted
its result. The release is restricted to receivers produced by the index-read
ops themselves (array_get, array_get_silent, hash_get, array_get_mixed_key,
array_get_mixed_key_silent) with refcounted non-string or Mixed results: their
parent container still holds its own reference, so the release cannot free
storage a borrowed string result may point into. The release lowers through the
existing Op::Release path, which is target-aware for all supported targets.
Under --web the handler returns to the bridge server loop and the exit-based
main epilogue is never reached, so a --gc-stats binary printed nothing. The web
handler epilogue now emits the counters once per request (before the
callee-saved restores), making per-request leaks observable as a growing
allocs-frees gap on stderr. Documents the --web behavior on the --gc-stats page
and aligns pre-existing assembly comment columns in the touched file.
@mirchaemanuel

Copy link
Copy Markdown
Contributor Author

The pre-existing issues listed in the PR notes are now filed individually with verbatim repros (all re-verified against pristine main e63d5d337): #525 (nullable-receiver chained read leak), #526 (segfault on first-index miss in a chained read), #527 (foreach by-value copy leak), #528 (loop-grown array rebind leak), #529 (nested Array(Mixed) write silently lost — correctness).

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.

1 participant