Remove EngineGetter #247
Remove EngineGetter #247
Conversation
Signed-off-by: Jorge Prendes <[email protected]>
|
@cpuguy83 , I know you had concerns with removing |
| impl EngineGetter for Wasi { | ||
| type Engine = Vm; | ||
| fn new_engine() -> Result<Vm, Error> { | ||
| PluginManager::load(None).unwrap(); | ||
| let mut host_options = HostRegistrationConfigOptions::default(); | ||
| host_options = host_options.wasi(true); | ||
| #[cfg(all(target_os = "linux", feature = "wasi_nn", target_arch = "x86_64"))] | ||
| { | ||
| host_options = host_options.wasi_nn(true); | ||
| } | ||
| let config = ConfigBuilder::new(CommonConfigOptions::default()) | ||
| .with_host_registration_config(host_options) | ||
| .build() | ||
| .map_err(anyhow::Error::msg)?; | ||
| let vm = VmBuilder::new() | ||
| .with_config(config) | ||
| .build() | ||
| .map_err(anyhow::Error::msg)?; | ||
|
|
||
| Ok(vm) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a red herring.
It's actually never used, the WasmEdgeExecutor builds its own Vm from scratch.
There was a problem hiding this comment.
Oh I guess this answers my questions
There was a problem hiding this comment.
Should we build Vm in the instance and then passes it to Executor so that we enable sharing.
There was a problem hiding this comment.
Maybe.
I didn't share in this PR because the code here (removed) and the one in the executor were different, and wasn't sure the vm in the executor was shareable (it does things like installing plugins).
@CaptainVincent , do you have any input?
There was a problem hiding this comment.
From my memory, it was used before the refactor into the container/executor architecture, but the current structure indeed always creates a new one. The aspects of sharing and reusing also involve the implementation's support for wasm runtime context. I thought enabling instance sharing only at the shim level might be questionable. I think there shouldn't be an issue removing it at this point.
This PR removes the
EngineGettertrait and replaces it with the use ofDefault::default().In the case where an instance would have implemented a non-trivial
EngineGetter, there are two options:Enginetype is local to the crate, implement a suitableDefault.Enginetype and implement a suitableDefaultfor the wrapper (and optionally alsoDeref)