Skip to content

Store engine together with Module to mitigate memory increase issue #1982

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

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

webmaster128
Copy link
Member

@webmaster128 webmaster128 commented Jan 6, 2024

Closes #1978

@webmaster128 webmaster128 marked this pull request as ready for review January 9, 2024 16:17
Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Great patch!
I tested this with dhat and bytes at t-gmax is reduced to 13MB by this. It is also reached at the end of the compilation step, so this definitely fixes the problem.
Only question I have is about the size_estimate (see below)

/// The runtime engine to run this module. Ideally we could use a single engine
/// for all modules but the memory issue described in <https://github.com/wasmerio/wasmer/issues/4377>
/// requires using one engine per module as a workaround.
pub engine: Engine,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should update the docs for size_estimate too. It still mentions "Store/Engine are not cached anymore".
Also: Don't we need to update the code for size_estimate now that we store the engine again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good points. Was thinking about them before but then forgot. Should be addressed in the last commit. Once approved, I can squash the patch commits for easy porting and add a CHANGELOG entry.

@webmaster128
Copy link
Member Author

I tested this with dhat and bytes at t-gmax is reduced to 13MB by this. It is also reached at the end of the compilation step, so this definitely fixes the problem.

Thanks for the confirmation! I did not test it yet

Copy link
Contributor

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

LGTM

@webmaster128 webmaster128 force-pushed the one-engine-per-module branch from b7ef836 to 1b110c6 Compare January 11, 2024 17:08
@chipshort
Copy link
Contributor

@Mergifyio backport release/1.5 release/1.4

Copy link
Contributor

mergify bot commented Jan 12, 2024

backport release/1.5 release/1.4

✅ Backports have been created

chipshort added a commit that referenced this pull request Jan 15, 2024
Store engine together with Module to mitigate memory increase issue (backport #1982)
chipshort added a commit that referenced this pull request Jan 15, 2024
Store engine together with Module to mitigate memory increase issue (backport #1982)
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.3 -> 1.4 regression bug: Memory usage increases over time
2 participants