RFC: Add instance allocator support to Wasmtime.

[peterhuene]

Rendered RFC

This RFC proposes adding an "instance allocator" abstraction to Wasmtime to
allow for alternative instance resource allocation strategies.

One such strategy will be a "pooling instance allocator" that can quickly
allocate instances, memories, and tables from the host address space that has
been reserved in advance.

This RFC is inspired by a Lucet performance feature called "regions".

[alexcrichton]

Thanks for writing this up! The main piece I'm wondering about is whether, with this design, the InstanceAllocator traits are necessary. The Instance type (in both the wasmtime-runtime and wasmtime crates) doesn't have any sort of low-level constructor so there's really no way to implement this outside of the wasmtime crates themselves.

I think it'd be great if we could stabilize some form of low-level, possibly unsafe, constructor which would enable external crates to build on as well, but otherwise if all the types implementing these traits are local to the wasmtime crates themselves we may as well avoid the traits entirely and just have dedicated methods on Config?

[peterhuene]

I agree that the traits aren't adding much given the very-much-internal definitions of both Instance and InstanceHandle, other than perhaps a future allocator implementation doesn't need to change Config to implement. That's not worth a lot, though.

I think it would be beneficial to try to make Instance and InstanceHandle unsafe externally constructable so that the pool allocator can be put in its own crate and then the traits would have merit. I'll see what I can do.

[peterhuene]

@alexcrichton how awful would it be for Config to expose a method taking an Arc<dyn wasmtime_runtime::InstanceAllocator>? A complete non-starter?

This asymmetry between the InstanceAllocator traits in wasmtime and wasmtime-runtime necessitate a back pointer from wasmtime_runtime::Instance to the allocator that created it so that the instance can call into the allocator upon deallocation of the InstanceHandle. I would really like to see the back pointer removed.

Having Config directly take the runtime trait would both simplify the code (i.e. no duplicated types in the wasmtime crate) and eliminate the problem because Store could use the runtime trait directly to deallocate the instance.

This also means that if we open up the construction of Instance and InstanceHandle in the runtime so that instance allocators can be implemented in external crates, the Wasmtime API can use those implementations without introducing a wrapping type in the wasmtime crate.

[alexcrichton]

Heh I always start with the idea that anything is possible!

More to the point though a move like that would basically just be about the costs. It would cost us in terms of stability that we would have to document that usage of the method is not covered under our semver stability promises. That's probably feasible to do since we don't expect every user to have to implement this? Especially if we can bundle "common" allocation strategies in the wasmtime crate which are covered under the semver rules.

Other than that the only other alternative I can think of is to sync up on the code you've got and see if anyone else is struck with inspiration of how to not have the dependency

[fitzgen]

It wouldn't solve the duplication issue, but would solve the API stability issues: we could create a wasmtime-traits crate that defines just the InstanceAllocator trait and necessary types (e.g. InstanceRequest) and say it has the same stability that the wasmtime crate does (even re-export it from wasmtime).

[peterhuene]

What if we kept the sole InstanceAllocator trait in wasmtime_runtime where it would inherently be unstable (and thus implementors would know that their implementations are subject to breaks in wasmtime_runtime) but we would then consider Config::with_instance_allocator to itself be stable despite taking Arc<dyn wasmtime_runtime::InstanceAllocator> as an argument? The wasmtime crate and any implementors of InstanceAllocator must agree on the definition anyway, right?

The "out of the box" instance allocator implementations could be exported via the wasmtime crate and thus considered stable as their only public API surface are their constructors.

We generally don't expect users to implement the trait themselves as it requires a lot of internal runtime understanding to do so, but we do want an easy mechanism for users to swap out the allocator implementations using the public Wasmtime API.

[fitzgen]

If we don't expect anyone else to implement this trait, then we might as well make it an enum InstanceAllocationStategy { Default, Pool } at the wasmtime API level, and not expose its guts, types, and implementation details. This would let us write the instance pool in its own (internal) crate, preserve API stability, and avoid duplicating types.

[peterhuene]

I can get behind that. We could always extend any such enumeration for Custom if a trait is needed in the future.

[peterhuene]

I've updated the RFC with an enum approach that hides the implementation details from the wasmtime crate.

[alexcrichton]

I think that's actually a good point that having an enum here is probably the way to go. We can always extend the enum in a backwards-compatible way (or just add more methods to Config) and I think it's a good observation that the allocation strategy is so tightly coupled to the internals of wasmtime we wouldn't really expect anyone to implement the traits anyway.

The wasmtime crate changes look good here to me! I'm not reading the internal changes too too closely, but I'm assuming they're all fine as well (and are easy to change if a problem arises)

[peterhuene]

I agree that Default doesn't really describe what's going on. I'll go with OnDemand with a comment calling it the default allocation strategy.

[fitzgen]

👍

The public API looks good to me; the internal details make sense but, since they're internal, they aren't set in stone and we can always tweak and revisit things during implementation.

[peterhuene]

Motion to finalize with a disposition to merge

I'm proposing that we merge this RFC.

Early feedback has been received and all requested outstanding changes to the proposal has been made.

An implementation in Wasmtime has already been started.

Stakeholders sign-off

Tagging all employees of BA-affiliated companies who have committed to the Wasmtime repo in the last three months plus anyone who has given feedback on this PR as a stakeholder.

Fastly

• @alexcrichton
• @pchickey
• @fitzgen
• @sunfishcode
• @cfallin
• @iximeow
• @fst-crenshaw

IBM

• @uweigand

Intel

• @abrown

Mozilla

• @julian-seward1
• @yurydelendik

[fitzgen]

✔️

[peterhuene]

Entering Final Call Period

https://github.com/bytecodealliance/rfcs/blob/main/accepted/rfc-process.md#making-a-decision-merge-or-close

Once any stakeholder from a different group has signed off, the RFC will move into a 10 calendar day final comment period (FCP), long enough to ensure that other stakeholders have at least a full business week to respond.

The FCP will end on February 26th.

[peterhuene]

The FCP has elapsed without any objections being raised; as a result I'm going to merge this. Thanks everyone!