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

Question: Thread-Safety #90

Open
christophwille opened this issue Feb 1, 2022 · 11 comments
Open

Question: Thread-Safety #90

christophwille opened this issue Feb 1, 2022 · 11 comments

Comments

@christophwille
Copy link

christophwille commented Feb 1, 2022

I have asked that in the past most likely via an at-mention in https://github.com/christophwille/dotnet-opa-wasm/ - but today it came back up for multi-threaded usage of the Wasmtime API. I have cooked up a sample and I am pretty sure I remember our past discussion wrongly... the overall goal is to not incur the compilation hit.

https://github.com/christophwille/dotnet-opa-wasm/blob/master/spikes/HigherLevelApisSpike/IPolicyFactory.cs#L129

I load a wasm file into a byte array, create an engine and use the engine for module load - and for the sake of discussion, throw away that engine. The question now is: is Module safe to use in parallel later for

// _store is created on a new Engine, _linker same
var instance = _linker.Instantiate(_store, module);

Ie can one cached Module be used to create unlimited instances in parallel.

Thanks.

@peterhuene
Copy link
Member

peterhuene commented Feb 1, 2022

Yes, you should be able to share a module between threads for the purpose of instantiation. The module will keep a reference on the engine used to create it, but you can dispose of your own reference to that engine if it is no longer needed.

It's only Store (and all store-related objects like instances) that cannot be shared between threads.

Also, the .NET API supports loading of modules from "precompiled" modules, but not creating them as we don't currently expose the Engine::precompile_module method from the Wasmtime API for the .NET API to use. If you have Wasmtime installed, you can use wasmtime compile <module> to generate a precompiled file (cwasm), however.

The benefit of using a precompiled file is that there is no JIT compilation at all; the module is simply mapped into memory upon load.

@christophwille
Copy link
Author

christophwille commented Feb 1, 2022

Ah, didn't know about the precompilation feature, interesting though (very much so).

Then my current object model was already correctly split between Module and instance with linker and store. And for my new approach I only need to cache one Module for creating multiple instances.

Edited for completeness:

using var opaRuntimeForLoad = new OpaRuntime();
using var module = opaRuntimeForLoad.Load("example.wasm");

using var opaRuntime = new OpaRuntime();
using var opaPolicy = new OpaPolicy(opaRuntime, module);
using var opaPolicy2 = new OpaPolicy(opaRuntime, module);

@peterhuene
Copy link
Member

From a cursory inspection, that should work as you intend.

christophwille added a commit to christophwille/dotnet-opa-wasm that referenced this issue Feb 1, 2022
@willhausman
Copy link

Yes, you should be able to share a module between threads for the purpose of instantiation. The module will keep a reference on the engine used to create it, but you can dispose of your own reference to that engine if it is no longer needed.

@peterhuene does this mean that it doesn't matter if the same Engine is used to create an Instance from a Module?

e.g.

var engine = new Engine();
var module = Module.FromFile(engine, file);

// now stuff happens with the application keeping module in memory but disposing engine

var engine = new Engine();
var linker = new Linker(engine);
var store = new Store(engine);
DoSetupLinkerStuff();
var instance = linker.Instantiate(store, module);  // <- different engines

@peterhuene
Copy link
Member

Cross-engine instantiation isn't supported and I'd expect that to throw an exception (at least it should; please file a bug if not).

You'll need to use the same engine for the module, linker, and store. I forgot that we don't have an Engine property on Module or Linker in the .NET API like we do in the underlying Wasmtime API in Rust as it's not exposed via the C bindings.

You'll need to keep a reference to the same engine around for now.

@christophwille
Copy link
Author

christophwille commented Feb 2, 2022

You'll need to use the same engine for the module, linker, and store. I forgot that we don't have an Engine property on Module or Linker in the .NET API like we do in the underlying Wasmtime API in Rust as it's not exposed via the C bindings.

Could you expose such a property? Or is this a no-no for the .NET object model? Because with that would a allow

using var opaPolicy = new OpaPolicy(module.Runtime, module);

without keeping the Engine around "some other way". The Engine is thread-safe though?

@christophwille
Copy link
Author

Cross-engine instantiation isn't supported and I'd expect that to throw an exception (at least it should; please file a bug if not).

https://github.com/christophwille/dotnet-opa-wasm/blob/master/src/Opa.Wasm.UnitTests/ThreadSafetyTests.cs#L10 it does fail but not very "obvious" as to the cause unless you look at the line it is failing on https://github.com/christophwille/dotnet-opa-wasm/blob/master/src/Opa.Wasm/OpaPolicy.cs#L140 and know the innards of Wasmtime.

@christophwille
Copy link
Author

Kind of related question: if Engine and Module are tied tightly together - is there somewhere guidance on tradeoffs/use cases for (a) one engine married to one module (possibly hundreds around at the same time) vs (b) one engine for multiple modules? Which approach is "better", to use when?

@peterhuene
Copy link
Member

Right now there isn't a way for the .NET API to expose an Engine property on either Module or Linker without storing a reference on the .NET side which is generally something I've been trying to avoid doing (I prefer the native side keep the references to keep things simpler and relatively deterministic).

It wouldn't be hard to add C API functions to get a reference to the engine for a given module or linker and then make use of those from the .NET API, though. I can look at doing that when I get a chance.

With regards to the exception you're getting, that's definitely an unexpected and confusing one, so I can dig into why it isn't the expected "inter-engine instantiation isn't supported" message.

@peterhuene
Copy link
Member

Oh and Engine is thread safe too.

@peterhuene
Copy link
Member

It's generally fine to create a single Engine per process and create all modules from that engine.

An engine is simply a JIT compiler, an instance allocator, and a signature registry.

The JIT compiler is thread-safe and will synchronize the compilation of any modules being compiled for that engine.

The instance allocator is what is used to allocate new instances from the instantiation of modules. By default (and what's supported by the .NET API), it's an on-demand allocator that simply allocates the needed resources of an instance when requested; it doesn't store any state that grows over time. The Rust API has another allocator type that uses resource pools to allocate from, but that's not yet supported by .NET.

Lastly, the signature registry is a global place we assign unique ids to function signatures. This is used as an optimization so that we can very quickly verify function signatures when doing indirect calls from WebAssembly. This will grow over time, but is bounded by how many unique function signatures are encountered in compiled modules and host functions (thus likely a fairly small set).

None of these things would be impacted from having a single Engine shared across all modules.

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

No branches or pull requests

3 participants