wasmtime / PR #2625 Implement defining functions at the E... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2625 Implement defining functions at the E...

Wasmtime GitHub notifications bot (Jan 30 2021 at 02:09):

peterhuene opened 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 01 2021 at 16:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

FWIW this sort of interning I was hoping would make Store faster since Store was short-lived, but since Engine is long-lived it's probably not necessary to do this and we can probably just get away with the simpler-to-read String everywhere perhaps?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Similar to above, could this document the Send and Sync bounds?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Do you think there's any need for this to be dynamic for the lifetime of the Engine itself? I would naively expect that any typical usage of this would be "let's create everything at the beginning and then that's it". If that's the case I think it's a bummer to put this behind an rwlock.

If you agree, perhaps we could move the building portion to Config which is basically an EngineBuilder at this point anyway. That way we wouldn't need to have the synchronization around this. Similarly we could literally have an EngineBuilder if needed to which is constructed with a Config and is then consumed to produce an Engine.

In any case this is mostly a stylistic thing, I just think it'd be nice to avoid the usage of locks if we can.

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Ideally this Store::upgrade would only happen once here, although it would make the resume_panic case slightly trickier where it has to be sure to drop store. Since we're already manually dropping things here, though, that may be ok?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

I think this can be a bit unsafe since EngineFunc doesn't embed a reference to its Engine, so this cloned object isn't guaranteed to not outlive an Engine. That being said I suspect the usage is all correct, but this comment may need to be updated.

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Due to the trickineses involved here, especially around not having local variables that need to be dropped, could this shim be shared with into_func above?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Could the docs here also be sure to point out the Send and Sync bounds and how more relaxed bounds are available on Func?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

One thing I'm worried about is reading this value later on since it could get signatures mismatched. Could this perhaps store u32::max_value() as a signature index (or w/e the max value is) so if it's accidentally used we weed out that bug sooner rather than later?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Could this have +Send+Sync as well? If it can't cast down could a transmute be used to erase the type bounds?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

To avoid the pub(crate) here could this perhaps be defined in the func.rs module?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

This is one of the things I'm most worried about with this PR. I don't think there's any issue with this currently, only that one could be lurking by accident in the future.

My concern here is that the not-threadsafe InstanceHandle is being shared across threads. This is ok because we happen to not touch any of the non-threadsafe state, but all of InstanceHandle and consumers are currently written without thread-safety in mind. Do you think there's anything we can do to future-proof this? For example I mentioned one elsewhere in this PR where we store an invalid index so if it's ever used we hopefully abort quickly. I think ideally we'd have checks in wasmtime_runtime::Instance as well, but I'm not sure if they would be too onerous for other usages.

Basically my main worry here is that this function is introducing threading in a non-compiler-checked fashion. All authors of runtime/src/instance.rs need to be aware of this but that's difficult to enforce because this happens in such an isolated location.

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

I think these may not need to be pub?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Like above could this have +Send+Sync?

Wasmtime GitHub notifications bot (Feb 01 2021 at 16:37):

alexcrichton created PR Review Comment:

Since define_func above returns a Result<()> should this as well?

Wasmtime GitHub notifications bot (Feb 01 2021 at 17:51):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 17:51):

fitzgen created PR Review Comment:

I don't know, deduplicating strings is kinda nice (module string will be repeated a bunch, for example)

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:12):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:12):

fitzgen created PR Review Comment:

My concern here is that the not-threadsafe InstanceHandle is being shared across threads. This is ok because we happen to not touch any of the non-threadsafe state, but all of InstanceHandle and consumers are currently written without thread-safety in mind. Do you think there's anything we can do to future-proof this?

Would it be feasible to split out a struct of Sync members from InstanceHandle that we can share, without sharing the whole thing across threads?

e.g.
struct InstanceHandleSync {
    // sync members in here...
}

struct InstanceHandle {
    sync: InstanceHandleSync,
    // non-sync members here...
}

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene created PR Review Comment:

That's an excellent point. I don't think one would redefine these functions after creating the Engine and doing this in a builder pattern would also remove the need to track "redefined" functions to keep them alive.

I think Config would work for this. Perhaps define_host_func and wrap_host_func?

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene created PR Review Comment:

I'll add that.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:51):

peterhuene created PR Review Comment:

I'll add that.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:52):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:52):

peterhuene created PR Review Comment:

I'll add those bounds.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:59):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Feb 01 2021 at 18:59):

alexcrichton created PR Review Comment:

Eh I don't mind too much about the names, those seem fine to me!

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

VMSharedSignatureIndex::default() is u32::max_value() and guaranteed to never match a signature as the registry doesn't hand that value out.

Using the default should cause any indirect call to fail if somehow this value gets used, which would clearly be a bug.

This value should never be read anyway as all imports of the engine function should use the store-local VMCallerCheckedAnyfunc that has the correct shared signature.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

Unfortunately I don't think so since most JIT code goes from *mut VMContext straight to *mut Instance (the internals of the handle) which has all the non-Sync pieces. For example we just have a dynamic guarantee that the instance never tries to elem.drop, for example.

One of my bigger worries is leaking the incorrect VMSharedSignatureIndex which is configured in the VMContext. I think storing an out-of-bounds index will help weed things out, but otherwise I don't have any great ideas. That being said I don't believe that it's leaked today at all.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

Will add.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

I'll fix.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

Oh nice! I didn't realize that was the default but that's exactly the behavior I was hoping to have here, so sounds like there's nothing to do!

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

I share your concerns over safety here.

I still think we're getting some mileage out of the compiler for safety as Store is !Send+!Sync and the store to use for an engine function is always retrieved from TLS (and thus should be the same thread that created the store).

Perhaps a check in Caller::store() that ensures the call is happening on the same thread that created the store, just to be safe?

Would it be feasible to split out a struct of Sync members from InstanceHandle that we can share, without sharing the whole thing across threads?

I think splitting wasmtime_runtime::Instance like that shouldn't be necessary as the thread-affinity of an instance shouldn't be violated by these changes.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

Func::new returns Self because of the expect call on the result from generate_func_export.

Should we just change define_func to return () similarly to match?

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

Agreed, this comment is misleading as nothing in a runtime instance ties back to the engine itself.

This clone impl will likely not be needed if I remove the RwLock in favor of the builder pattern anyway.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

Yeah that seems fine to me! We'll probably want to document the shadowing behavior though since this differs slightly than that of Linker where by default Linker returns an error. Returning an error here though feels a bit heavyweight.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

These fields are also accessed from Store::get_engine_func, but I'm happy to make these private with accessor impls if you think it's warranted.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

My worry is moreso for the internals of Wasmtime itself rather than callers. You're right though in that we do have a number of consistency checks and internals (I suspect more than a few locations modified in this PR were found via assertion failures rather than knowing what to do off the top of your head). My biggest worry is hidden mutability in InstanceHandle, but I think it's not the end of the world for that to be something we remain vigilant for.

Perhaps a nice doc-block can go somewhere though explaining the threading involved here?

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

Oh true, perhaps though there could be something like engine_func.to_func(&store)? (mostly just looking to localize this logic if we can)

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

I don't have a strong opinion on this. I'm fine with removing the de-duplication for clarity's sake.

That said, I think de-duplication is more likely to be exploited at the engine-level where (ideally) WASI is being defined rather than in a Linker.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

I'll see what I can do to localize the implementation to EngineFunc.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

A non-issue if these get removed from Engine in favor of the builder-pattern, though.

Re: shadowing, this PR should include a check in Linker that ensures failure if an engine function exists and the linker is !allow_shadowing. I'll fix that.

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

peterhuene edited PR Review Comment.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

Agreed, this can be done once. I'll fix this.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

The problem is these two shims differ in how the store is accessed (one from capture and the other from TLS); related to that is the associated instance state also differs.

As such, I don't think much could be shared here since I wouldn't want the resume_panic call moved to any other frame than the shim function itself.

Wasmtime GitHub notifications bot (Feb 01 2021 at 21:48):

peterhuene edited PR Review Comment.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

So there's a problem with adding this bound.

We're passing self in the IntoFunc impls, which has type F with bounds F: Fn(Caller<'_>, $($args),*) -> R + 'static and unfortunately that bound is shared with into_func which wouldn't want those bounds on F otherwise it pollutes Func::wrap.

To properly implement this, we'd need a separate IntoEngineFunc impl. Thoughts?

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

peterhuene edited PR Review Comment.

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

peterhuene submitted PR Review.

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

peterhuene created PR Review Comment:

Or would we rather add these bounds here and transmute them in for the IntoFunc impl with a big comment as to why this is safe (i.e. because the bounds are present in the Engine::wrap_func)?