Skip to content

fix(interp): stop host allocation scanning the heap and leaking frame slots - #171

Merged
siyul-park merged 1 commit into
mainfrom
claude/issues-169-170-analysis-9evo2h
Aug 7, 2026
Merged

fix(interp): stop host allocation scanning the heap and leaking frame slots#171
siyul-park merged 1 commit into
mainfrom
claude/issues-169-170-analysis-9evo2h

Conversation

@siyul-park

@siyul-park siyul-park commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Two defects landed together with the heap-ownership rework and made
allocation-heavy guest programs regress badly (#169, #170).

owner() answered "is this pointer already in a heap slot?" by scanning
every live slot with reflect.ValueOf().Pointer(). It runs on Alloc,
Store, and Push, so an embedder that allocates once per guest operation
paid O(heap) per allocation and ran quadratically: a fannkuch-redux
program that took 6.1s stopped finishing inside 45s. Replace the scan
with an index keyed by the value itself. Only the host boundary records
hints, so the interpreter's own allocation path is untouched, and lookup
validates against the heap, so a hint left by a freed or reused slot
self-heals instead of needing removal bookkeeping on release and sweep.
A prune pass bounds the index and doubles the budget it survives, which
keeps upkeep amortized constant.

RETURN moved the returned values down and truncated the stack without
releasing the params, locals, and operands it discarded, so every call
that received a reference leaked one count. land already releases those
slots when an exception unwinds a frame; RETURN now does the same.
Reference counting had leaked this way before, but the old root-based
mark and sweep reclaimed it; trial deletion reads an inflated count as
ownership outside the heap and cannot, so the leak became unbounded.
Frames whose every slot is a plain scalar skip the sweep when their
operand stack is balanced, which keeps scalar recursion close to its
former cost.

Measured with the reporter's program corpus on linux/amd64 (min of
three): fannkuch 6.9s where it previously did not finish, binarytrees
1.60s against 1.80s before the regression window, and the unrelated
kernels unchanged. RecursiveFib(20)/threaded costs 604 -> 625 ns/op for
the exact counts.

Adds vm_gc_cycles_total and vm_gc_slots_total so collector pressure is
observable, a heap-exhaustion regression test that fails without the
RETURN fix, and two builder-based benchmarks covering the reported
shapes with no external dependency.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_017ZsuY2FrrwQ7S3PGw98RQh

Summary by CodeRabbit

  • Performance

    • Improved memory-management performance for recursive workloads and host-integrated operations.
    • Reduced unnecessary cleanup work during routine execution and returns.
  • Bug Fixes

    • Improved frame cleanup to prevent retained references from exhausting available memory.
    • Strengthened pointer ownership handling and garbage-collection reliability.
  • Documentation

    • Added guidance for heap ownership, return-value memory behavior, and garbage-collection metrics.
    • Expanded benchmark coverage and documented new memory-related results.
  • Monitoring

    • Added metrics for total garbage-collection cycles and processed heap slots.

… slots

Two defects landed together with the heap-ownership rework and made
allocation-heavy guest programs regress badly (#169, #170).

owner() answered "is this pointer already in a heap slot?" by scanning
every live slot with reflect.ValueOf().Pointer(). It runs on Alloc,
Store, and Push, so an embedder that allocates once per guest operation
paid O(heap) per allocation and ran quadratically: a fannkuch-redux
program that took 6.1s stopped finishing inside 45s. Replace the scan
with an index keyed by the value itself. Only the host boundary records
hints, so the interpreter's own allocation path is untouched, and lookup
validates against the heap, so a hint left by a freed or reused slot
self-heals instead of needing removal bookkeeping on release and sweep.
A prune pass bounds the index and doubles the budget it survives, which
keeps upkeep amortized constant.

RETURN moved the returned values down and truncated the stack without
releasing the params, locals, and operands it discarded, so every call
that received a reference leaked one count. land already releases those
slots when an exception unwinds a frame; RETURN now does the same.
Reference counting had leaked this way before, but the old root-based
mark and sweep reclaimed it; trial deletion reads an inflated count as
ownership outside the heap and cannot, so the leak became unbounded.
Frames whose every slot is a plain scalar skip the sweep when their
operand stack is balanced, which keeps scalar recursion close to its
former cost.

Measured with the reporter's program corpus on linux/amd64 (min of
three): fannkuch 6.9s where it previously did not finish, binarytrees
1.60s against 1.80s before the regression window, and the unrelated
kernels unchanged. RecursiveFib(20)/threaded costs 604 -> 625 ns/op for
the exact counts.

Adds vm_gc_cycles_total and vm_gc_slots_total so collector pressure is
observable, a heap-exhaustion regression test that fails without the
RETURN fix, and two builder-based benchmarks covering the reported
shapes with no external dependency.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZsuY2FrrwQ7S3PGw98RQh
@siyul-park
siyul-park merged commit 0820060 into main Aug 7, 2026
3 of 4 checks passed
@siyul-park
siyul-park deleted the claude/issues-169-170-analysis-9evo2h branch August 7, 2026 10:06
@coderabbitai

coderabbitai Bot commented Aug 7, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 518792f0-eb22-411f-a836-3d26c1e0713e

📥 Commits

Reviewing files that changed from the base of the PR and between 8b4e647 and c7a3a86.

📒 Files selected for processing (10)
  • benchmarks/memory_test.go
  • docs/benchmarks.md
  • docs/host-integration.md
  • docs/memory-model.md
  • docs/profile.md
  • internal/cmd/geninterp/lower.go
  • interp/interp.go
  • interp/interp_test.go
  • interp/threaded.go
  • interp/trace.go

📝 Walkthrough

Walkthrough

The interpreter adds indexed heap ownership tracking, return-time frame-slot cleanup, garbage-collection metrics, regression tests, documentation, and recursive memory benchmarks.

Changes

Memory runtime behavior

Layer / File(s) Summary
Indexed heap ownership tracking
interp/interp.go, interp/trace.go, interp/interp_test.go, docs/host-integration.md, docs/profile.md
The interpreter records and validates pointer ownership through an index, isolates tracer ownership maps, records GC metrics, and tests pointer allocation behavior.
Return-time frame cleanup
internal/cmd/geninterp/lower.go, interp/threaded.go, interp/interp_test.go, docs/memory-model.md
Return paths release discarded frame values and operands, preserve returned values, correct coroutine ranges, and skip sweeps for balanced scalar-only frames.
Recursive memory benchmark coverage
benchmarks/memory_test.go, docs/benchmarks.md
The benchmark suite adds permutation-flip and binary struct-tree workloads with VM, native, and language comparison implementations and reference results.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant HostBoundary
  participant Interpreter
  participant owners
  participant Frame
  participant GC
  HostBoundary->>Interpreter: Alloc, Store, Push, Load, Retain, Pop
  Interpreter->>owners: Record and validate heap-slot ownership
  Frame->>Interpreter: returnOp
  Interpreter->>Frame: retire discarded frame slots
  Interpreter->>GC: Record collection cycles and heap slots
Loading

Possibly related PRs

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/issues-169-170-analysis-9evo2h

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.45.0)
interp/threaded.go

ast-grep timed out on this file


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

2 participants