Skip to content

runtime: expose AsyncCompiledModule::module + AsyncRuntime::wrap_module#29

Merged
colinrozzi merged 4 commits into
mainfrom
feat/wrap-module
Jun 13, 2026
Merged

runtime: expose AsyncCompiledModule::module + AsyncRuntime::wrap_module#29
colinrozzi merged 4 commits into
mainfrom
feat/wrap-module

Conversation

@colinrozzi

Copy link
Copy Markdown
Owner

Summary

  • New: AsyncCompiledModule::module() -> &Module — getter on the underlying compiled module.
  • New: 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

  • `cargo build -p packr` clean
  • `cargo test -p packr --lib` 93/93 pass

Consumer

`colinrozzi/theater` will use this for the per-connection child spawn cache in frontdoor v1. PR chain:

  1. theater#114 (engine sharing, lands first — no packr dep)
  2. this PR (packr wrap_module, then patch release)
  3. theater PR (compile cache, depends on this released)

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 colinrozzi left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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_modulemodule().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.

Comment thread src/runtime/mod.rs
/// 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<'_> {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

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

theater-dev added 2 commits June 12, 2026 05:24
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.
Per review on #29:

- `wrap_module` now asserts `Engine::same(module.engine(), &self.engine)`
  before constructing the `AsyncCompiledModule`. Cross-engine misuse
  panics here with a named message instead of deep inside
  `Linker::instantiate` on the cache hit. The check is an Arc pointer
  compare — free relative to instantiation.
- Doc section rewritten from "Safety / correctness" to a proper
  "# Panics" stanza matching the actual contract.
- Two new tests:
  - `wrap_module_roundtrip_through_cache_pattern` — exercises
    load_module → module().clone() → wrap_module → instantiate +
    typed call, the exact pattern the docs advertise and theater's
    cache will lean on.
  - `wrap_module_panics_on_cross_engine` — `#[should_panic(expected
    = "different Engine")]` confirms the misuse path hits the
    assert at the API boundary.
@colinrozzi

Copy link
Copy Markdown
Owner Author

Good catch on the panic semantics — fixed in 501e677.

  • wrap_module now asserts Engine::same(module.engine(), &self.engine) before constructing the AsyncCompiledModule. Cross-engine misuse panics at the API boundary with a named message instead of deep inside Linker::instantiate on the cache hit.
  • Doc rewritten as a proper # Panics stanza matching the actual contract.
  • Two new tests covering both the happy path and the panic path:
    • wrap_module_roundtrip_through_cache_pattern — exercises load_modulemodule().clone()wrap_module → instantiate + typed call (calls answer() -> i32 and asserts 42, so we know the engine genuinely owns the instance, not just that metadata-only instantiation passes).
    • wrap_module_panics_on_cross_engine#[should_panic(expected = "different Engine")] confirms the misuse path lands on the assert.

cargo test -p packr --lib 95/95 pass (was 93).

@colinrozzi colinrozzi merged commit e721111 into main Jun 13, 2026
2 checks passed
@colinrozzi colinrozzi deleted the feat/wrap-module branch June 13, 2026 14:40
@colinrozzi colinrozzi mentioned this pull request Jun 13, 2026
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 participant