lunatic icon indicating copy to clipboard operation
lunatic copied to clipboard

Shared compiled wasm modules

Open tqwewe opened this issue 3 years ago • 7 comments

Allows compiled wasm modules to be shared between the same node.

Two new host functions were added:

  • "lunatic::message::push_module"
  • "lunatic::message::take_module"

Modules are no longer pre-linked. The upside of this is that WasmtimeCompiledModule no longer has a generic (which removes generics from many other places too), but the downside is that it might be a little less performant.

I'll try to get some benchmarks done to test this PR's performance compared to before.

tqwewe avatar Aug 19 '22 15:08 tqwewe

Main branch

spawn process           time:   [107.77 µs 108.37 µs 109.00 µs]                          
                        change: [-71.703% -71.432% -71.123%] (p = 0.00 < 0.05)
                        Performance has improved.

This PR

spawn process           time:   [384.67 µs 385.86 µs 387.08 µs]                          
                        change: [+251.58% +255.12% +258.40%] (p = 0.00 < 0.05)
                        Performance has regressed.

Seems like the performance does decrease substantially sadly :(

I'm not sure how to get past this since with pre linking requires WasmtimeCompiledModule to have a generic, which causes issues when sharing the module across processes.

tqwewe avatar Aug 19 '22 16:08 tqwewe

We only have one state (generic T) though. Could we bubble it up all the way to Resource<T> and then use the state type to create the specific Resource? I didn't look at the code for some time now and am just assuming bunch of stuff.

bkolobara avatar Aug 19 '22 16:08 bkolobara

I'm not entirelly sure what you mean, but its possible you might have a better solution. Optimally we can share wasm compiled modules across processes while keeping the same performance with pre-linking.

tqwewe avatar Aug 24 '22 04:08 tqwewe

I will take a look at it and see if there is a way to make it work.

bkolobara avatar Aug 24 '22 08:08 bkolobara

After re-reading your comment, I think I understand. The problem with Resource having a generic T is that we put Resource in a vec to store all resources.

pub struct DataMessage {
    // ...
    pub resources: Vec<Resource>,
}

If Resource had a generic here, then we couldnt store a vec of resources with different types.

Though, this might be fine to require every resource to have the same T state actually?

I've just made a branch on my fork which has the same performance and allows compiled modules to be shared via Arc by simply doing Vec<Resource<T>> (each resource has the same type). https://github.com/tqwewe/lunatic/tree/feat/shared-module-2

If you'd like, I can open it as a separate PR and close this one.

tqwewe avatar Aug 29 '22 05:08 tqwewe

The main requirement of the state is that it implements ProcessState. It seems like there's really only one type that implements ProcessState, and that is DefaultProcessState. Could we remove ProcessState trait and rename DefaultProcessState to ProcessState? So ProcessState would be a sturct instead of a trait.

tqwewe avatar Aug 29 '22 05:08 tqwewe

The main requirement of the state is that it implements ProcessState. It seems like there's really only one type that implements ProcessState, and that is DefaultProcessState. Could we remove ProcessState trait and rename DefaultProcessState to ProcessState? So ProcessState would be a sturct instead of a trait.

The reason why we introduced the ProcessState trait is that in a case where we embed the lunatic_runtime inside of another rust project, you can provide your own process state and don't need to use all the features.

For example, if you don't need networking in your embedded usage, you just provide a ProcessState without the networking parts and don't implement NetworkingCtx for it. So you can still use all the other built in host functions that we provide, except the networking ones.

At the moment there is only one implementation of the ProcessState (DefaultProcessState), but this could change in the future. Even for "per feature (networking, timeouts, spawning, ...) testing purposes", we could provide smaller process states that are crate local. We would not be able to use the DefaultProcessState because of circular dependencies in that case.

bkolobara avatar Aug 31 '22 09:08 bkolobara

Closing in favor of #148

tqwewe avatar Oct 21 '22 05:10 tqwewe