Stream: git-wasmtime

Topic: wasmtime / Issue #1400 Option for host managed memory

Wasmtime GitHub notifications bot (Mar 25 2020 at 03:04):

github-actions[bot] commented on Issue #1400:

Subscribe to Label Action

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

<details> <summary>Users Subscribed to "wasmtime:api"</summary>

• @peterhuene

</details>

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

Learn more.

Wasmtime GitHub notifications bot (Mar 26 2020 at 03:05):

eust-dfinity commented on Issue #1400:

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?

Wasmtime GitHub notifications bot (Mar 26 2020 at 03:52):

eust-dfinity edited a comment on Issue #1400:

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 :) )

Wasmtime GitHub notifications bot (Mar 27 2020 at 15:07):

alexcrichton commented on Issue #1400:

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)

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

eust-dfinity commented on Issue #1400:

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!

Wasmtime GitHub notifications bot (Apr 03 2020 at 06:27):

eust-dfinity commented on Issue #1400:

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)

Wasmtime GitHub notifications bot (Apr 03 2020 at 06:29):

eust-dfinity edited a comment on Issue #1400:

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?

Wasmtime GitHub notifications bot (Apr 03 2020 at 16:15):

alexcrichton commented on Issue #1400:

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

Wasmtime GitHub notifications bot (Apr 06 2020 at 00:49):

eust-dfinity commented on Issue #1400:

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.

Wasmtime GitHub notifications bot (Apr 06 2020 at 14:52):

alexcrichton commented on Issue #1400:

Thanks!

Last updated: Nov 22 2024 at 17:03 UTC