wasmtime / PR #1901 Support for `funcref`s, `ref.func`, a... · git-wasmtime

The fallout from this change seems quite large (affecting the C API and many callers of the default API), but I can't figure out why this changed?

While this makes sense for Val I don't think this makes sense for Extern which is acquired from an instance, and I don't think that instances can export a null funcref, right?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

I think there's a safe conversion from &T to NonNull<T> to use here?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

I saw somewhere else that this was store-local, but it's also here per-instance? Could this perhaps be *mut and stored in the Store instad?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Could u64::from be used still to avoid as?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"b"

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"c"

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

This expression happens a lot in this block, so perhaps this could do this at the top?
let anyfunc = self.export.anyfunc.as_ref();
// ...

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"a"

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Since there's already a lifetime here, perhaps this could be &'a Store?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

I think this answers my question about where the None came from, but I think that we're guaranteed ExportFunction is never a "null function", right?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

This got a little gnarly, mind annotating the _ here?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Nice :+1:

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

An idle thought, but we could perhaps have something like:
impl Func {
    pub(crate) fn into_raw(self, dst: &Store) -> Result<NonNull<VMCallerCheckedAnyfunc>> {
        // ...
    }
}
to bake in how you can't access the raw value without using the right store. Anyway not necessary to do here, just a thought!

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

I think this should write the store "null funcref" singleton instead of a null pointer, right?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Another passing though, perhaps fn is_null(&self) -> bool could be added to VMCallerCheckedAnyfunc

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Is NonNull::new here necessary? This is related to my question above too I think.

I figured that ref.null would be implemented as returning the null singleton rather than a literal null pointer. (to make table.set a simple store instead of having a branch to store a non-null value)

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Ah it looks like there is indeed a null funcref per-store, but perhaps this could be reused for each Instance?

Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Could this perhaps go through the Store instead? One day if we ever support cross-compilation this'd be through that instead of wasmtime-runtime

Wasmtime GitHub notifications bot (Jun 23 2020 at 16:29):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 16:29):

fitzgen created PR Review Comment:

wasmtime-environ is the thing that should handle the cross compilation as it is about the environment of the compilation target, while wasmtime-runtime is always compiled into the target. So if we put it in wasmtime-runtime, then it is always going to reflect the compilation target.

If we wanted to support cross compilation at the wasmtime API level, then we would have to stop linking wasmtime-runtime directly, which would force us to (correctly) revisit calls to this function from the wasmtime crate. So I think we should be good here.

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:33):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:33):

fitzgen created PR Review Comment:

I... think you are correct. I'll look into it.

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:50):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:50):

fitzgen created PR Review Comment:

I didn't realize that NonNull<T> implements From<&T>!

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:51):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 17:51):

fitzgen created PR Review Comment:

FWIW, we don't use pointer equality for VMCallerCheckedAnyfuncs, so this would just be a memory optimization that saves two words per instance.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton created PR Review Comment:

Ah yeah that's all I'm imagining, fewer small allocations here-and-there. It also sort of helps the case of "I precompiled my wasm" since we'd presumably statically allocate this anyfunc instead of trying to dynamically allocate it.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton created PR Review Comment:

It's true yeah we're a long way away from wasmtime supporting AOT compilation, but I just figured this'd be a relatively easy future-proofing.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:41):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:41):

fitzgen created PR Review Comment:

I think we are as future proofed as it makes sense to be given that this function is safely inside wasmtime-runtime, as mentioned above.

I think our best path forward for AOT cross compilation is a wasmtime-aot crate.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:52):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:53):

fitzgen requested alexcrichton for a review on PR #1901.

Wasmtime GitHub notifications bot (Jun 23 2020 at 18:57):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 23 2020 at 23:37):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 24 2020 at 00:28):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 24 2020 at 13:54):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 24 2020 at 13:54):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jun 24 2020 at 13:54):

alexcrichton created PR Review Comment:

I think the return here can go away, or this may also benefit from a early-return match:
let anyfunc = match NonNull:new(anyfunc) {
    Some(f) => f,
    None => return Val::FuncRef(None),
};
// ...
~~~

Wasmtime GitHub notifications bot (Jun 24 2020 at 15:59):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 24 2020 at 17:01):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 24 2020 at 17:08):

fitzgen updated PR #1901 from ref-func to master.

Wasmtime GitHub notifications bot (Jun 24 2020 at 17:53):

fitzgen merged PR #1901.

Last updated: Nov 22 2024 at 16:03 UTC