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

One engine to rule them all #1753

Merged
merged 13 commits into from
Jul 24, 2023
Merged

One engine to rule them all #1753

merged 13 commits into from
Jul 24, 2023

Conversation

webmaster128
Copy link
Member

This PR follows the recommendation of the Wasmer team to avoid using more engines than necessary.

We now have two main types of engines:

  • Compiling engines contain a compiler such as as Singlepass. They are also responsible for the middlewares.
  • Runtime engines are headless, i.e. do not contain a compiler.

A few cool things happen here:

  • This seems to work with Wasmer 4.0.0 without the patch amd as such this unblocks us
  • We avoid carying around a compiler instance when we don't need it
  • We still need one compiling engine per module since we cannot reuse the metering a middleware for multiple modules
  • We don't need to hold the Engine in the in-memory caches

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good to me.

This looks much simpler than the current version. Looks like the right approach: Funny how we migrated from a cached Store to a cached Engine to nothing cached but Module.

pub fn make_engine(middlewares: &[Arc<dyn ModuleMiddleware>]) -> Engine {
/// Creates an engine without a compiler.
/// This is used to run modules compiled before.
pub fn make_runtime_engine(memory_limit: Option<Size>) -> Engine {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Nit: These can better live in their own engine.rs for consistency.

packages/vm/src/wasm_backend/store.rs Show resolved Hide resolved
packages/vm/src/wasm_backend/store.rs Show resolved Hide resolved
packages/vm/src/instance.rs Outdated Show resolved Hide resolved
packages/vm/src/cache.rs Outdated Show resolved Hide resolved
@@ -35,14 +34,15 @@ pub fn main() {
println!("wasm size: {wasm_size} bytes");

// Compile module
let module = module_compile(&wasm);
let engine = make_compiling_engine(None, &[]);
let module = compile(&engine, &wasm).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This compile helper is now a little redundant / useless. I would say, let's either remove it, or use it everywhere instead of the explicit Module creation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I kept it to hide some internals in one of the examples. Not sure anymore. I use it now in all places.

packages/vm/src/modules/in_memory_cache.rs Show resolved Hide resolved
Copy link
Collaborator

@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.

Looks good 👍

packages/vm/src/cache.rs Outdated Show resolved Hide resolved
packages/vm/src/wasm_backend/store.rs Outdated Show resolved Hide resolved
packages/vm/src/wasm_backend/store.rs Show resolved Hide resolved
packages/vm/src/cache.rs Outdated Show resolved Hide resolved
@webmaster128 webmaster128 merged commit ba5efd5 into main Jul 24, 2023
3 checks passed
@webmaster128 webmaster128 deleted the one-engine branch July 24, 2023 07:52
da1suk8 added a commit to da1suk8/cosmwasm that referenced this pull request Nov 30, 2023
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