Skip to content

perf: release memory when guest leaves - #22

Merged
MeteorsLiu merged 1 commit into
xgo-dev:mainfrom
MeteorsLiu:codex/guest-exit-gc
Sep 20, 2026
Merged

MeteorsLiu merged 1 commit into
xgo-dev:mainfrom
MeteorsLiu:codex/guest-exit-gc

Conversation

@MeteorsLiu

Copy link
Copy Markdown
Collaborator

No description provided.

@fennoai fennoai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread guest_linux.go
}
// runGuest has released its state graph and input mapping. Collect after
// those roots leave the stack, before the guest process exits.
runtime.GC()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

codecov Bot commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
guest_linux.go 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@MeteorsLiu
MeteorsLiu merged commit abdf205 into xgo-dev:main Sep 20, 2026
3 of 7 checks passed
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