wasmtime / PR #1524 Refactor · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1524 Refactor

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:39):

sunfishcode opened PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

I think this would probably be best done by keeping &self as an argument, because that way if you want to test Func a few times you don't have to worry about cloning and getting the Func back in case of an error.

I think we can still return a 'static closure here, it just needs to close over the InstanceHandle as well.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

Out of curiosity, how come this changed? Is the intention to avoid storing the GlobalType next to the ExportGlobal since they're basically inferred from one another?

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

It looks like a lot of the diff is due to the change here, I'm curious how come this changed though?

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

I think if we return an ExactSizeIterator we can avoid adding an extra method for this, since it should still be possible to do module.imports().len() in that case.

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

This may need double-checking, but I think self.module may not be necessary any more after this change. I think the Store is still needed but we may be able to remove the Module field from an Instance?

Wasmtime GitHub notifications bot (Apr 16 2020 at 18:59):

alexcrichton created PR Review Comment:

To enhance this a bit, perhaps this could return an ExactSizeIterator which should allow for enumeration and such?

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:04):

sunfishcode created PR Review Comment:

With Instance::exports() computing Extern values on demand, it now returns Extern instead of &Extern. In code like let func = instance.get_export("foo").unwrap().func().unwrap(), if func() took a &self, it'd be taking a reference to a temporary.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:04):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:13):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:13):

alexcrichton created PR Review Comment:

Makes sense yeah, could we perhaps add alternative methods for these idioms though? Idiomatically this feels like the correct signature for this function (a by-value perhaps being into_func(self)). Perhaps we could add Instance::functions() and such which return iterators over just those values?

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:14):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:14):

sunfishcode created PR Review Comment:

I did a quick experiment with this, and Linker::instance currently needs Instance::module. We don't currently store the export names in the instance otherwise.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:16):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 19:16):

alexcrichton created PR Review Comment:

Ah ok no worries. We could make the internal handle pub(crate) so the linker could access it, but it's fine to tackle this later if really necessary.

Wasmtime GitHub notifications bot (Apr 16 2020 at 20:12):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 20:12):

sunfishcode created PR Review Comment:

This is because Module::exports is now giving you results by value instead of by reference, and without this a lot of common scenarios have to do awkward things to avoid taking a reference of a temporary.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:49):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:49):

sunfishcode created PR Review Comment:

And, it turns out, we can actually make Instance::exports provide the names in a reasonable way, which helps the Linker, and makes the API much nicer as you no longer need to zip with Module::exports just to get the names.

Now, nothing in Instance itself or its users need it to have a Module. I've left it in for now because it looks like it might unregister some things in its Drop, but if we want to sort that out we could drop the Module after instantiation.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:50):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:50):

sunfishcode created PR Review Comment:

Cool, done.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:50):

sunfishcode created PR Review Comment:

Works great.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:50):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:51):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:51):

sunfishcode created PR Review Comment:

Makes sense. I've now implemented this.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:51):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 16 2020 at 23:54):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 17 2020 at 00:14):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 17 2020 at 00:14):

sunfishcode created PR Review Comment:

Ok, renaming them to into_func().

We could add Instance::functions() if that's a common use case, but users could also write Instance.exports().filter_map(|export| export.into_func()) if that's what they want.

Wasmtime GitHub notifications bot (Apr 17 2020 at 00:17):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 17 2020 at 00:34):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

Stray lifetime?

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

Instead of having a struct for this could we perhaps return a tuple from the exports iterator?

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

I've worried historicaly that this is pretty unwieldy. The previous incantation was sort of just on the line of not motivating me to try to improve things, but this feels like it's pretty unergonomic to document as a pattern to call things. Would you be ok adding more accessors in this PR for Instance? For example I think we could make this:
instance.get_func("foo")?.get2::<i32, i32, i32>()?;

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

You can skip the struct type definition here, inside of the closure you can do drop(&instance_clone) and just use export directly, and that'll capture everything necessary.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

I also don't mind pushing up some new iterators/accessors for Instance as part of this PR too

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

Similar to above, I think we should keep this as an internal field with a public accessor so we can change the make up of the structure over time.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

I personally don't think we should have public-structs-with-public-fields in the API. They're quite inflexible in terms of future additions as well as changing the makeup of the internals. For example committing to two String allocations here I think may be something we want to tweak in the future. We could, for example, have something like Arc<str> which is cheaply cloneable and contains module/string, along with an index of where to split the string.

Wasmtime GitHub notifications bot (Apr 17 2020 at 17:06):

alexcrichton created PR Review Comment:

Could the old methods be preserved since they can be useful in their own right too?

Wasmtime GitHub notifications bot (Apr 18 2020 at 00:56):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 18 2020 at 00:57):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:06):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:17):

sunfishcode created PR Review Comment:

Oops, yes. This was from an earlier experiment. Fixed now.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:17):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:22):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:22):

sunfishcode created PR Review Comment:

Good idea, I've now added get_func etc. accessors and update the examples and docs to use them, and that makes things much cleaner.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:23):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:23):

sunfishcode created PR Review Comment:

I started cleaning this, and ended up finding it made sense to go ahead and eliminate the allocations. So we now have accessor functions, and they return & 'module str.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:24):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:24):

sunfishcode created PR Review Comment:

Also fixed.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:26):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:32):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:32):

sunfishcode created PR Review Comment:

I'm not opposed, I just haven't found cases where they're useful. For the "I don't know what this is" or "This might be one of a few different things" use cases, Extern is just an enum and you can plain old match or if let on it, which read better anyway.

With the new Instance::get_func accessors, even Extern::into_func is a little dubious, because you can use match and if let to do that too, but they are handy when you want to report different errors in cases like this:
    instance
        .get_export("_start")
        .context("expected a _start export")?
        .into_func()
        .context("expected export to be a func")?
        .call(&[])?;

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:33):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 16:33):

sunfishcode created PR Review Comment:

Ah, cool. I've now done this.

Wasmtime GitHub notifications bot (Apr 20 2020 at 18:36):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 18:41):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:09):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:47):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:47):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:47):

alexcrichton created PR Review Comment:

I think that this is auto-inferred, but could this be changed to Export<'me>?

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:47):

alexcrichton created PR Review Comment:

I think the 'module self can just be self here (keeping the &'module str ret)

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:47):

alexcrichton created PR Review Comment:

ditto as above

Wasmtime GitHub notifications bot (Apr 20 2020 at 19:48):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:03):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:09):

sunfishcode updated PR #1524 from refactor to master:

This contains refactorings and API changes split off from #1516.

The main public-facing change here is that Instance and Module now compute their imports and exports on demand rather than holding precomputed arrays, and Func::get* return closures which capture an InstanceHandle so that they hold the instance live dynamically.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode created PR Review Comment:

Done

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode created PR Review Comment:

Done

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:11):

sunfishcode created PR Review Comment:

Done

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:55):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 20 2020 at 20:55):

alexcrichton merged PR #1524.

Last updated: Nov 22 2024 at 16:03 UTC