Option for host managed memory

[ghost]

As per discussion on host managed memory on zulip started by @lostman here is a simple implementation.

To make things least invasive I ended up with two traits - one for allocator and one for actual memory. This way LinearMemory is not forced to know anything about the allocator and consequently does not need to inherit restrictions placed on the allocator (e.g. Sync).

Additionally DefaultAllocator is made public, so it's possible to create a wrapper around the existing allocator/memory (insert some hooks) and supply it as a custom one.

There are no specific test cases for custom allocators. Can be added if required.
Implementation itself is tested within existing tests with DefaultAllocator.

@alexcrichton @tschneidereit @peterhuene please have a look

[github-actions]

Subscribe to Label Action

This issue or pull request has been labeled: "wasmtime:api"

Users Subscribed to "wasmtime:api"

• @peterhuene

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

Thanks for this! From an API perspective I think that this is an unfortunately tricky addition from the wasmtime crate. In addition to some of the comments I've made below I think it'd also be good to have something like Memory::new_custom or basically something where you can construct a Memory with a custom allocator and then pass that around to mirror the functionality here as well.

[ghost]

Thanks for the comments!

As you pointed out Allocator is a too general term. I'll be calling it MemoryCreator here.

As for the safety concerns and possible things that can go wrong.
I distinguish two different entities here. The first one is LinearMemory. This is meant to resemble a simple buffer, not thread safe (but you can hardly expect anything more). In a basic case the buffer can have a bigger preallocated size, and extend it's accessible size on grow(). In general case, it could reallocate itself, but current wasmtime's memories already do that, so it's handled.
The true unsafety part comes from vmmemory call, which basically gives away a pointer, so it's up to wasmtime to give a guarantee, that this pointer will not be used again after grow() is called.
On the host side, it needs to guarantee that it won't do anything with this memory while it's owned by wasmtime, excluding actions inside grow() function. Another thing is alignment and size being a multiple of host page size (but I'm actually not sure if these are hard requirements). I think these are all the restrictions that apply here.

The second entity is the MemoryCreator. This one should be thread safe (Sync). It could just call an mmap, or it could provide the memory from some preallocated pool. The memory creator should conform with MemoryPlan. If it can't it should fail. The biggest problem here is that if MemoryPlan struct changes (gets a new field), the maintainers of a custom MemoryCreator may fail to notice that.

Currently MemoryPlan is quite a rich struct. Maybe it's a good idea to reorganize it? In particular memory limits and offset_guard_size are the fields which seem like good candidates to be passed to the custom allocator. As for MemoryStyle it seems to me like this is more like wasmtime's internal parameter used by wasmtime's MemoryCreator. The Memory::shared flag worries me most, but it doesn't seem to be used at all... Or we could just pass the required parameters explicitly and not through this struct.

I think it'd also be good to have something like Memory::new_custom or basically something where you can construct a Memory with a custom allocator and then pass that around to mirror the functionality here as well.

Can you elaborate a bit more on that? I'm not sure I understand. Isn't MemoryCreator::new_memory() basically doing that?
EDIT: ahh, you mean extermals::Memory, ok, now I understand (I think :) )

[alexcrichton]

I'll be calling it MemoryCreator here.

Sounds reasonable to me!

As for the safety concerns and possible things that can go wrong.

What you wrote down sounds good to me, thanks for writing that up! You're right yeah that the vmmemory method is the one that must be accurate generally, and that's why I'm thinking it'll probably be an unsafe trait.

Maybe it's a good idea to reorganize it?

This is where I think if we could define an API in the wasmtime crate which we then translate to underlying layers it might be a good idea. The MemoryPlan structure is currently pretty raw and it might be best to have a layer on top of that which we specifically design for API consumption rather than JIT internals. (the example here is the shared flag which is ignored since it's not implemented in the internals, but we still parse it because "one day we'll take a look at it")

Can you elaborate a bit more on that?

A right yeah, I'm thinking of wasmtime::Memory::new where you should ideally be able to create a wasmtime::Memory with your own custom backing scheme, so you don't have to rely on these custom traits.

(Ideally if we could get away with only updating wasmtime::Memory that'd be great. Then we wouldn't have to have a global config setting of "how to allocate a wasm-defined memory, you'd only have to concern yourself with creating the right wasmtime::Memory structures that conform to your needs)

[ghost]

I created appropriate traits in the API.
The mechanism in principle does not change when compared to the previous version.
I tried a few different things here, but this one seems most clean to me.
Modifying only Memory is difficult. Memory and LinearMemory are two different concepts.
As it is now, Memory is more of a struct which represents a (memory) inside a module.
LinearMemory is just a buffer and is way more low-level. I think that mixing them up would just create confusion.

I ended up not adding Memory::new_custom thing. I don't really have a clear picture in my mind of how this should work. Maybe I'm misunderstanding something after all. The problem here is, that we want to be able to capture all memories created by "create_memories" from runtime, and current Memory interface allows to create only one. I guess, if that is what you would like to have there, I can add it like that. Let me know.
For now please take a look and confirm if this satisfies all the crate dependency concerns you had.
Thanks!

[alexcrichton]

Looks great to me, thanks for this!

Could this also add a few tests to crates/api/tests/* which exercise usage of this?

[alexcrichton]

How come this is wrapped in a RefCell?

[ghost]

This is complementary to:

wasmtime/crates/runtime/src/memory.rs

Line 147 in a325b62

let mut mmap = self.mmap.borrow_mut();

Basically it's here because RuntimeLinearMemory::vmmemory() takes &self and not &mut self, but needs to return mut ptr.

[ghost]

I added an example test and updated the rest according to the suggestions.

Btw, maybe it's a good idea to make LinearMemory::grow() mutable? (Same for actual grow in wasmtime_runtime)

Also I wrapped the whole test I added in not_for_windows module. Is there a better way to exclude this test from windows e.g. on the Cargo.toml level?

[alexcrichton]

Looking good! I think we'll want to remove &mut self entirely from the trait (which would remove the need for the RefCell in the translation wrapper. There's so much aliasing going on here I'm not sure we can actually guarantee the &mut property needed for growth, so implementations will need to internally hold a RefCell or a Mutex as appropriate

[ghost]

Looking good! I think we'll want to remove &mut self entirely from the trait (which would remove the need for the RefCell in the translation wrapper. There's so much aliasing going on here I'm not sure we can actually guarantee the &mut property needed for growth, so implementations will need to internally hold a RefCell or a Mutex as appropriate

Done. Also added a test for grow.

[alexcrichton]

Thanks!