runtime: expose AsyncCompiledModule::module + AsyncRuntime::wrap_module#29
runtime: expose AsyncCompiledModule::module + AsyncRuntime::wrap_module#29colinrozzi wants to merge 2 commits into
Conversation
Lets callers maintain an external compile cache: let m = runtime.load_module(bytes)?; // pay compile cost once let cached: Module = m.module().clone(); // ...later, on cache hit: let m2 = runtime.wrap_module(cached); // no recompile m2.instantiate_with_host_and_interceptor_async(...).await?; wasmtime::Module is Send + Sync + cheap-clone (Arc internally) but its construction (Module::new) runs cranelift, which dominates per-spawn cost in any runtime that compiles the same wasm repeatedly. Theater's actor runtime spawns a fresh actor per inbound connection in the per-conn-child pattern; the wasm bytes are identical across N spawns. Without these two methods packr's AsyncCompiledModule fields are private and theater can't cache Modules without piercing the abstraction or reaching directly into wasmtime. Both methods are additive — no semantic change for existing callers. Docs flag the engine-scoping constraint on wrap_module: a Module built against engine A cannot be instantiated by engine B. Misuse surfaces as wasmtime's instantiation error, not memory unsafety, so the method is safe but logically narrow.
colinrozzi
left a comment
colinrozzi
left a comment
There was a problem hiding this comment.
Ship it after one small fix. The API shape is right — additive, mirrors load_module, borrowed-engine lifetime matches the existing struct — and it's exactly what theater's compile cache needs. One finding:
The "Safety / correctness" doc is wrong about the failure mode. wasmtime treats cross-engine usage as a programmer error and panics; it does not return an instantiation error. wasmtime 27's Linker::instantiate Panics section: "Panics if any item used to instantiate module is not owned by store. Additionally this will panic if the Engine that the store belongs to is different than this Linker." So a Module from engine A wrapped into a runtime on engine B takes down the calling task at instantiate time with a panic from deep inside wasmtime — worse diagnostics than the doc promises, and in theater's case it would fire on the first cache hit after any engine-identity bug, not at cache-insert time.
Suggested fix — check eagerly at the API boundary instead of documenting around it:
pub fn wrap_module(&self, module: Module) -> AsyncCompiledModule<'_> {
assert!(
Engine::same(module.engine(), &self.engine),
"wrap_module: Module was compiled by a different Engine than this runtime"
);
AsyncCompiledModule { module, engine: &self.engine }
}Module::engine() and Engine::same both exist in wasmtime 27 and the comparison is an Arc pointer compare — free relative to instantiation. Then the doc becomes simply "Panics if module was compiled by a different Engine," which is both true and a better contract. (A debug_assert! + corrected doc is acceptable too, but given the cost is nil I'd make it unconditional.)
Nice-to-have, not blocking: a roundtrip test (load_module → module().clone() → wrap_module → instantiate + call) so the exact cache pattern the docs advertise has coverage — that's the path theater is about to lean on per-connection.
| /// runtime's engine (or a clone of it). Misuse will surface as an | ||
| /// instantiation error from wasmtime, not memory unsafety, so this | ||
| /// method is `safe` but logically narrow. | ||
| pub fn wrap_module(&self, module: Module) -> AsyncCompiledModule<'_> { |
There was a problem hiding this comment.
Doc nit with teeth: misuse does not surface as an instantiation error — wasmtime panics on cross-engine usage (Linker::instantiate docs, wasmtime 27). Suggest an eager assert!(Engine::same(module.engine(), &self.engine), "...") here so the panic happens at the API boundary with a message that names the bug, then reword this section to "Panics if module was compiled by a different Engine."
Both types already appear in the module's public API (AsyncRuntime::engine(), and now wrap_module / AsyncCompiledModule::module) but were not nameable by callers without a direct wasmtime dependency. Needed by theater to hold cached Modules in its compile cache.
1 participant
Summary
AsyncCompiledModule::module() -> &Module— getter on the underlying compiled module.AsyncRuntime::wrap_module(Module) -> AsyncCompiledModule<'_>— constructor from a pre-compiled module bound to this runtime's engine.Both additive, no semantic change for existing callers.
Why
Lets a caller maintain an external compile cache:
```rust
let m = runtime.load_module(bytes)?; // pay compile cost once
let cached: Module = m.module().clone(); // Module is cheap-clone (Arc-internal)
// ... later, on cache hit ...
let m2 = runtime.wrap_module(cached); // no recompile
m2.instantiate_with_host_and_interceptor_async(...).await?;
```
`wasmtime::Module` is `Send + Sync` and cheap-clone (`Arc` internally), but `Module::new` runs cranelift which dominates per-spawn cost in any runtime that compiles the same wasm repeatedly. Theater's actor runtime spawns a fresh actor per inbound connection in the per-conn-child pattern that frontdoor v1 needs; the wasm bytes are identical across N spawns. Without these methods packr's `AsyncCompiledModule` fields are private and theater can't cache `Module`s without piercing the abstraction or reaching directly into wasmtime.
Safety / correctness
Docs on `wrap_module` flag the engine-scoping constraint: a `Module` built against engine A cannot be instantiated by engine B. Misuse surfaces as wasmtime's instantiation error, not memory unsafety, so the method is `safe` but logically narrow. The typical usage pattern (one shared runtime per caller, cache the `Module`, wrap on hit) makes this trivially correct.
Test plan
Consumer
`colinrozzi/theater` will use this for the per-connection child spawn cache in frontdoor v1. PR chain: