Stream: git-wasmtime

Topic: wasmtime / PR #1901 Support for `funcref`s, `ref.func`, a...


view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 18:35):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 18:35):

fitzgen requested alexcrichton for a review on PR #1901.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 19:11):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 19:11):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 19:11):

alexcrichton created PR Review Comment:

FWIW this pattern with #[cfg] might be good to encapsulate somewhere with something like ir::types::rsize()

view this post on Zulip Wasmtime GitHub notifications bot (Jun 18 2020 at 23:41):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2020 at 21:48):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2020 at 22:51):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2020 at 22:52):

fitzgen requested alexcrichton for a review on PR #1901.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 22 2020 at 23:22):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 00:04):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

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?

view this post on Zulip 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?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"b"

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"c"

view this post on Zulip 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();
// ...

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

"a"

view this post on Zulip 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?

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 14:35):

alexcrichton created PR Review Comment:

Nice :+1:

view this post on Zulip 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!

view this post on Zulip 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?

view this post on Zulip 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

view this post on Zulip 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)

view this post on Zulip 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?

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 16:29):

fitzgen submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 17:33):

fitzgen submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 17:50):

fitzgen submitted PR Review.

view this post on Zulip 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>!

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 17:51):

fitzgen submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:03):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:41):

fitzgen submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:52):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:53):

fitzgen requested alexcrichton for a review on PR #1901.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 18:57):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 23 2020 at 23:37):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 00:28):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 13:54):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 13:54):

alexcrichton submitted PR Review.

view this post on Zulip 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),
};
// ...
~~~

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 15:59):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 17:01):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 17:08):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jun 24 2020 at 17:53):

fitzgen merged PR #1901.


Last updated: Dec 23 2024 at 12:05 UTC