Stream: git-wasmtime

Topic: wasmtime / PR #2625 Implement shared host functions.

Wasmtime GitHub notifications bot (Feb 02 2021 at 02:54):

peterhuene edited PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Engine rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Engine and then use a Linker (or directly with
Store::get_engine_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 02:58):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Engine rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Engine and then use a Linker (or directly with
Store::get_engine_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 02:59):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Engine rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Engine and then use a Linker (or directly with
Store::get_engine_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 02:59):

peterhuene edited PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:01):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:01):

peterhuene created PR Review Comment:

I've added a comment where we have the unsafe impls for HostFunc.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:02):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:02):

peterhuene created PR Review Comment:

I'm now in favor of keeping the interning as the map can be cloned into multiple engines from a single config.

Thoughts with the recent changes?

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:02):

peterhuene requested alexcrichton for a review on PR #2625.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:02):

peterhuene requested alexcrichton and fitzgen for a review on PR #2625.

Wasmtime GitHub notifications bot (Feb 02 2021 at 03:15):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:58):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:58):

alexcrichton created PR Review Comment:

Hm we may wish to brainstorm a different API for this. I'm worried about this causing unexpected panics in applications because you couldn't, for example, call this method recursively twice. I fear that would hamper usage of this API quite a lot. For example if you're implementing a WASI API which calls back into the module to malloc you'd need to be extremely careful to not with_context while malloc is being run.

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:58):

alexcrichton created PR Review Comment:

Would it be possible to move this logic into self.get directly? That internally handles the module-linking feature where if you import an instance it can be satisfied with a function definition of a two-level import. I think that if self.store were consulted in self.get that it'd naturally support that though.

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:58):

alexcrichton created PR Review Comment:

Ah ok makes sense! I don't feel that this should be removed really, only that I wasn't sure if the complexity was worth it (not even sure it's entirely worth it in Linker, not like I've profiled anything...). Seems fine to leave though!

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:58):

alexcrichton created PR Review Comment:

We could unify these two actually by fetching the Store from the same location, the location in TLS post-dates the original storage into the function's environment here. I think then the two could be shared?

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:59):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 15:59):

alexcrichton created PR Review Comment:

I think the ideal API here would be fn get<T>(&self) -> Option<&T> perhaps.

Could we not support remove and if consumers want it then they could have T = RefCell<Option<U>> perhaps?

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:42):

fitzgen created PR Review Comment:

Does this mean that we need to check that this store is using an engine that is using this HostFunc's config? is it possible for these things to get out of sync (which seems like it would lead to unsafety)

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:42):

fitzgen created PR Review Comment:

+1 for exposing &T instead of &mut T

if users are the ones adding ref cells, then they are aware of the re-entrancy concerns and can design around that, rather than being surprised when they finally hit a corner case and have already designed and implemented a bunch of stuff that now needs to be redone

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:42):

fitzgen created PR Review Comment:

Seems like, based on this call site, they should stay in-sync, but that perhaps to_func isn't always guaranteed to be safe. If I'm understanding all this correctly, then I think to_func should be unsafe and this call site should be the place with the unsafe block that has a comment like "we know this store is using the same config as the host func".

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:44):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:44):

fitzgen created PR Review Comment:

Could we not support remove and if consumers want it then they could have T = RefCell<Option<U>> perhaps?

Yeah, good point. I guess switching to &T alone isn't enough, we also have to not support removal to make reentrancy panic-free.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:45):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:45):

fitzgen created PR Review Comment:

Worth documenting for users how they would get mutability and the ability to remove context via RefCell<Option<T>> in the doc comments.

Wasmtime GitHub notifications bot (Feb 02 2021 at 18:45):

fitzgen edited PR Review Comment.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:44):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:44):

peterhuene created PR Review Comment:

If we want to do that rather than sourcing it from the instance data, I'll make the shim shared.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:47):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:47):

peterhuene created PR Review Comment:

I think you're right, get makes more sense for this implementation as it should behave as if the function were already defined in the linker even though it comes from the store.

I'll make that change.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:49):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:49):

alexcrichton created PR Review Comment:

Yeah I think we may as well change to that. It also has the added benefit of where the host state can remain 0-sized if Self is 0-sized (as is the case with all wasi functions). Additionally I think you could actually go ahead and remove into_func and simply have into_host_func, and then Func::wrap would have a small amount more glue to handle the add-the-instance-to-the-store case?

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:53):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:53):

peterhuene created PR Review Comment:

That's a good point about reentrancy and moving the RefCell into the context value would give the callers more control over mutation scope.

This will necessitate a method on Wasi or WasiCtx to set the context in the store with the exact type the generated Wasi host functions expect, though (it's how I had it in the RFC, but this was a poor attempt at a "cleaner" API).

I'll get this fixed.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:54):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:54):

peterhuene created PR Review Comment:

I will add a debug assertion just to double check that the HostFunc is indeed related to the associated config.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:55):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 19:55):

peterhuene created PR Review Comment:

I'll make it unsafe and comment as to the safety of the call.

Wasmtime GitHub notifications bot (Feb 02 2021 at 20:17):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 20:17):

peterhuene created PR Review Comment:

That works. Thoughts on renaming the trait then?

Wasmtime GitHub notifications bot (Feb 02 2021 at 20:18):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Feb 02 2021 at 20:25):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 02 2021 at 20:25):

peterhuene created PR Review Comment:

Actually, the lookup can't be done quickly as the HostFunc doesn't store the requisite information to query the passed store.

I think making this unsafe as suggested in the other comment is the best approach.

Wasmtime GitHub notifications bot (Feb 02 2021 at 22:55):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 23:05):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 02 2021 at 23:07):

peterhuene requested alexcrichton for a review on PR #2625.

Wasmtime GitHub notifications bot (Feb 02 2021 at 23:55):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 03 2021 at 19:13):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 03 2021 at 19:40):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with an Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 04 2021 at 02:52):

peterhuene edited PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with a Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 04 2021 at 17:57):

peterhuene updated PR #2625 from engine-funcs to main:

This PR introduces defining host functions at the Config rather than with
Func tied to a Store.

The intention here is to enable a host to define all of the functions once at
with a Config and then use a Linker (or directly with
Store::get_host_func) to use the functions when instantiating a module.

This should help improve the performance of use cases where a Store is
short-lived and redefining the functions at every module instantiation a
noticeable performance hit.

See bytecodealliance/rfcs#9 regarding these changes.

Wasmtime GitHub notifications bot (Feb 19 2021 at 01:00):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 09 2021 at 03:01):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 09 2021 at 03:47):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 09 2021 at 20:53):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 09 2021 at 20:58):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 09 2021 at 21:34):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 09 2021 at 23:52):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 10 2021 at 16:29):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 10 2021 at 16:29):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 10 2021 at 16:29):

alexcrichton created PR Review Comment:

Hm although then you'd have to also validate that no functions were defined yet, which is a bit wonky...

Do you have thoughts on this? I think it's fine either way myself.

Wasmtime GitHub notifications bot (Mar 10 2021 at 16:29):

alexcrichton created PR Review Comment:

The purpose of this being a constructor on Store was primarily to avoid switching at runtime, but because Config is intended as a builder and it's "frozen" as an Engine I think this is fine to simply have as an extra method on Config (probably named async_)

Wasmtime GitHub notifications bot (Mar 10 2021 at 18:12):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 10 2021 at 18:12):

peterhuene created PR Review Comment:

I'm leaning towards making it a setting like any other, rather than having Config::new_async, and having to set "async" prior to defining any async shared host functions.

With the instance allocator and now host functions in Config, I'm concerned that Config is too stateful now. There's a similar problem with async_stack_size not being able to be set after an instance allocation strategy was set because the pooling allocator relies on that setting.

I'm wondering if we should move the instantiated instance allocator and shared host functions into Engine (like we have for Compiler), leaving Config a bunch of settings and only concerned with conflicts rather than ordering of the settings. This wouldn't be a breaking change to the API.

This would change the semantics a little as each engine would now have its own instance allocator, but to me that makes more sense than having each engine share the instance allocator from a "configuration".

We'd still have to panic when you attempt to construct an Engine from an incorrect Config, though (e.g. you defined an async host function without setting the "async" setting, but now the order of those operations doesn't matter).

What's your take?

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:04):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:04):

peterhuene created PR Review Comment:

However, we'd also have to panic when constructing an engine if we can't allocate enough memory for the pooling instance allocator (when used) instead of what we do currently which is return error from Config::with_allocation_strategy.

Given this would ever likely be done once early on and isn't recoverable in a way that would make progress (out of memory :shrug:), I'm not terribly concerned with a panicking if it improves Config.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:05):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:06):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:08):

peterhuene edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:53):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:53):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:53):

peterhuene created PR Review Comment:

I've pushed up a change for the instance allocator to get a sense of what I'm talking about above. It also removes that confusing "default" instance allocator in the process.

I'll see about the same for moving "async" to a setting and making instantiating the host functions into the engine rather than config.

If we don't like it this way, I'll remove the commits.

Wasmtime GitHub notifications bot (Mar 10 2021 at 19:57):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 10 2021 at 23:55):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 11 2021 at 00:09):

peterhuene updated PR #2625 from engine-funcs to main.

Wasmtime GitHub notifications bot (Mar 11 2021 at 16:14):

alexcrichton merged PR #2625.

Last updated: Nov 22 2024 at 16:03 UTC