perf: release memory when guest leaves - #22
Conversation
There was a problem hiding this comment.
Review summary
Small, isolated change adding runtime.GC() at the end of guestEntry() after a successful runGuest(). The change is well-commented and the placement/error handling are correct (GC is skipped on the os.Exit(1) error path). Security review found no concerns: the memfd (fd 3) is already closed and the input image unmapped via defer inside runGuest before this point, and GC touches only the guest's private heap.
However, three of the review passes converged on one substantive concern about whether the call accomplishes anything — see the inline comment. No blocking issues.
| } | ||
| // runGuest has released its state graph and input mapping. Collect after | ||
| // those roots leave the stack, before the guest process exits. | ||
| runtime.GC() |
There was a problem hiding this comment.
guestEntry replaces main.main, so once it returns the guest process exits and the OS reclaims the entire address space regardless of GC state. A synchronous runtime.GC() here does a full stop-the-world mark/sweep whose only product is freed heap that is about to be discarded anyway — it adds teardown latency on every Run without an observable benefit.
The deterministic cleanup this seems aimed at is already handled inside runGuest: defer unix.Close(3) and the deferred unmap() (munmap) run during runGuest's return, before control reaches this line. I confirmed there are no runtime.SetFinalizer registrations in the production state/reflectx packages, so GC won't trigger any observable finalizer cleanup either.
The comment is also slightly inaccurate: runGuest doesn't explicitly "release its state graph" — the graph is a stack local that simply becomes unreachable on return; only the input mapping is explicitly released (via unmap()).
Suggestion: remove the runtime.GC() call, or — if it's retained for a specific measured reason (e.g., heap-profile hygiene for tooling) — document that concrete reason and consider gating it so it doesn't run on the hot path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
No description provided.