Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compiler: fix compiledModule leak #1608

Merged
merged 8 commits into from
Aug 2, 2023
Merged

compiler: fix compiledModule leak #1608

merged 8 commits into from
Aug 2, 2023

Conversation

ncruces
Copy link
Collaborator

@ncruces ncruces commented Aug 1, 2023

Fixes #1600.

I'm not 100% sure why this is needed, but it's being done as well in:

func (e *engine) Close() (err error) {
e.mux.Lock()
defer e.mux.Unlock()
// Releasing the references to compiled codes including the memory-mapped machine codes.
for i := range e.codes {
for j := range e.codes[i].functions {
e.codes[i].functions[j].parent = nil
}
}
e.codes = nil
return
}

Somehow, not doing this prevents the releaseCompiledModule finalizer from running.

Fixes #1600.

Signed-off-by: Nuno Cruces <[email protected]>
@ncruces
Copy link
Collaborator Author

ncruces commented Aug 1, 2023

This is obviously not the fix, but we clearly need to do more than #1535.

@achille-roussel
Copy link
Collaborator

I pushed a commit as a suggestion for a slightly different approach, but please feel free to revert or change it if we think this isn't the right approach.

The Go runtime cannot guarantee that finalizers will be run in the presence of cyclic references between objects, so I added an indirection layer to break the cycle between compiledModule and compiledFunction. A new compiledCode object is introduced that compiledModule and compiledFunction both reference, effectively removing the reference cycle between the two. The finalizer is still set on compiledModule, and is now called more reliably, which addresses the memory leak.

I've used a modified version of examples/allocation/zig/greet.go as suggested by @ncruces to reproduce the issue, adding debug statements to track memory usage (see https://gist.github.com/achille-roussel/ac529f1c10982b5c6dcd6239e72d828b), which shows stable memory usage at ~15MiB (tested on darwin/arm64).

@ncruces
Copy link
Collaborator Author

ncruces commented Aug 1, 2023

Any ideas on what to do about the failing test?
Otherwise your fix seems fine @achille-roussel! 👍

@mathetake
Copy link
Member

excellent collaboration 🥇

@mathetake mathetake marked this pull request as ready for review August 2, 2023 01:05
@mathetake mathetake merged commit 90f58bc into main Aug 2, 2023
59 checks passed
@mathetake mathetake deleted the fix-1600 branch August 2, 2023 01:14
@mathetake
Copy link
Member

kudos to @ncruces and @achille-roussel

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.

binary.decodeCode cause Memory leak ?
3 participants