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: add munmap finalizer on cache hits to avoid memory leak #1815

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

valpackett
Copy link
Contributor

Not sure how this was never noticed, trying to run the same thing over and over in response to request with cache enabled would OOM very quickly…

Screenshot from 2023-10-23 19-08-13

@ncruces
Copy link
Collaborator

ncruces commented Oct 24, 2023

Thanks for the report. I don't think this is the correct/best place to fix?

I think this should be added somewhere to this function:

func deserializeCompiledModule(wazeroVersion string, reader io.ReadCloser, module *wasm.Module) (cm *compiledModule, staleCache bool, err error) {

cc: @mathetake

@valpackett
Copy link
Contributor Author

Seemed like the place with least duplication and seemed good that it's the same function as the non-cached path. Inside getCompiledModule I thought in some way does make sense too, but there would need to be duplicated for the memory hit and disk hit paths.

@ncruces
Copy link
Collaborator

ncruces commented Oct 24, 2023

The memory cache hit doesn't create a new instance, so it doesn't need one?

You're actually replacing the finalizer that's already there, which probably causes no harm, but is unnecessary.

Signed-off-by: Val Packett <val@packett.cool>
@valpackett
Copy link
Contributor Author

Oh, makes sense. I see wazevo actually does set the finalizer and it's in getCompiledModule for the disk branch, so this should match that now.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

Thanks! I actually noticed this before but yeah completely it slipped my mind

@mathetake mathetake merged commit 99c057b into tetratelabs:main Oct 25, 2023
53 checks passed
@valpackett valpackett deleted the leaks branch October 25, 2023 08:25
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.

None yet

3 participants