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

Remove Engine from InstanceConfig #774

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jprendes
Copy link
Collaborator

@jprendes jprendes commented Dec 18, 2024

This PR removes the Engine generic from InstanceConfig.
As we move away from reusing one engine for all containers, we don't need to pass the engine around anymore.

This is a breaking change.

@jprendes jprendes force-pushed the remove-engine branch 4 times, most recently from 24c5373 to 5ff77cb Compare December 19, 2024 09:59
@jprendes jprendes force-pushed the remove-engine branch 2 times, most recently from 8c7ce9c to f2ca823 Compare December 19, 2024 13:50
@jprendes jprendes marked this pull request as draft December 19, 2024 14:53
@jprendes
Copy link
Collaborator Author

Converting to draft until I figure out how to deal with the regression in wasmtime.

@jsturtevant
Copy link
Contributor

As we move away from reusing one engine for all containers, we don't need to pass the engine around anymore.

This seems like the right thing to do since resolves a bunch of outstanding issues. This sharing of the engine was one of the "benefits" as I understood. It is such a small amount of shared memory/additional cpu, I don't think it will really be something that will be noticed but did we measure the chain against the bench mark at all?

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
@jprendes
Copy link
Collaborator Author

There seem to be a regression where wasmtime e2e tests are timing out, so that's a first indication.
But I think it's related to the pool allocator, which is allocating loads of virtual memory.
Before we were doing that operation once in the process, while now we do it twice per container.

@jsturtevant if you know how to get the benchmark reports for PRs, please let me know :-)

Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
Signed-off-by: Jorge Prendes <jorge.prendes@gmail.com>
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.

3 participants