Wasmtime: Can `HostRef<T>` be removed from the API?

[alexcrichton]

Reading over the wasmtime API documentation and implementation, I think we're in a great place in general for usage, but whenever I run across HostRef it always seems to trip me up. It seems particularly fraught due to usage of borrow and borrow_mut which seem like they're very easy to introduce panics by accident.

Would it be possible to remove HostRef from the public API? For example could types like Engine, Store, etc, all include an internal HostRef and Engine itself is just a wrapper around this HostRef?

I'd love to put it on us to take care of all panics and make it so you basically can't cause a panic if you use the API, even erroneously. This would also help simplify examples and "hello world" introductions since you wouldn't have to wrap types in HostRef or call borrow() on returned values.

@yurydelendik you probably know the most, do you think that this would work out?

[yurydelendik]

The API was designed around basic WebAssembly entities: Instance, Module, Func, Memory, etc. All this entities are use each other by references, so not sole ownership. Module can "produce" Instance, Instance contains Func or Memory. The user may hold all or any reference, e.g. Func and not the Instance.

The API was only designed for embedding. The scripting languages can map and control lifetime of WebAssembly entities as well as WebAssembly entities affect scripting languages objects. This can be addressed by attaching so called "host info", data and callback, to the above entities.

In C++ world, all such objects inherited from single class. In Rust world, things are more interesting. At this moment I chose the easy route similar to Rc<RefCell<_>>, which is HostRef<_>. The alternative is to just implement a trait and wrap all data objects -- it will be about the same design but easier for users to digest.

[yurydelendik]

I'd love to put it on us to take care of all panics and make it so you basically can't cause a panic if you use the API, even erroneously.

I'm a little bit confused about this statement. Does this mean we will take away from users ability to have control over reporting errors in the embedding? Embedders expect to run arbitrary code, so they may want to have a total control over this code error handling.

[alexcrichton]

Oh sorry so to be clear I'm not proposing actually removing any functionality, just reorganizing it a bit. For example an Instance itself would be Clone but it's just a handle, so it's like cloning an Rc. Today where you have HostRef<Module> it'd instead be Module { inner: HostRef<ModuleInner> }.

I think that shared ownership is the right design for this API, but I think that we don't want to expose primitives like borrow and borrow_mut. I don't think basically any of the apis should take &mut self because realistically you basically never have that with reentrancy and sharing.

I don't really understand the use case for the host info/data/callback attachments on every single object, but I'm not proposing we remove that just yet, just push it inside as internals instead of making it part of the public API that everything works with HostRef.

The major motivation for this was that it felt pretty weird manually wrapping everything in a HostRef when constructing items, and then it also felt pretty weird having to use borrow() to access anything, especially when I have no idea where the other borrow() calls are or if any might be borrow_mut() to look out for.

[joshtriplett]

@alexcrichton I would definitely favor that, for simplicity of embedding.

Hiding that as an implementation detail may also make it easier to adjust the implementation in the future, as well as add feature flags to reduce overhead. (For instance, we could have an enabled-by-default feature flag that when disabled would disable the "host info" fields and stub out the corresponding functions.)

[cedric-h]

Just for clarification, why would stubbing out such information be desirable? @joshtriplett

I understand the desire to reduce overhead, but I don't understand how "host info" fields provide any considerable amount of overhead.

[joshtriplett]

@cedric-h Every single Func, Global, AnyRef'd object, and various other common structures contain host info, adding an extra pointer-sized value to structures I expect to have many millions of. Those are very hot, very common structures, and making them a little bit smaller fits more of them into memory and cache.

[cedric-h]

Oh, wow. I stumbled across the host info trait and method in the documentation, but I wasn't aware that a copy of this information was stored with every single instance of those structures.

I agree; perhaps an issue tracking the implementation of this cargo feature and even documentation on this trait should be made.

https://docs.rs/wasmtime/0.8.0/wasmtime/enum.AnyRef.html#method.host_info
https://docs.rs/wasmtime/0.8.0/wasmtime/trait.HostInfo.html

[cedric-h]

I started writing a PR wherein HostRef is hidden from the user, starting with Module.

As @alexcrichton suggested, the current Module became ModuleInner, and Module became a struct which simply contained a HostRef to a ModuleInner.

Constructors on Module simply wrap their ModuleInner counterparts with Self { inner: HostRef::new(x) }, an internal macro may be useful for this as it's extended to the rest of the API.

Public static methods like validate() had to be moved from ModuleInner to Module, since I assume that we want to have ModuleInner be completely private.

The solution that I came up with for the getter methods on Module is sort of hacky, and I'm not extremely happy with it. At first, I considered implementing Deref, but I was a little scared that that would necessitate making ModuleInner public and wouldn't give us the control we need over some of the getters. I ended up making a custom override based on Ref::map for each of the getter methods, but I'm not really happy with this solution and I think we need to come up with something more versatile before going forward to remove HostRef from the rest of the API.

Furthermore, to get these to work, I couldn't simply return references as &T to the relevant data; I had to return Ref<T>s, because any references to the data inside of these Refs couldn't leave the scope of the function they were borrowed in. This makes some interfaces more awkward, for example, for export in &*module.exports() {}, where the dereference and reference are undesirable.

[alexcrichton]

I'm actually going to close this in favor of https://github.com/bytecodealliance/wasmtime/issues/708 which I think is a bit more detailed, there was actually quite a few things I think we should change with the public API!

Thanks for the initial work on this @cedric-h! I think once we're all on board with most of the changes in https://github.com/bytecodealliance/wasmtime/issues/708 we can go ahead and start landing PRs like your #696 to make some progress!