Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Mar 25 2020 at 02:38):

eust-dfinity opened PR #1400 from host_memory to master:

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

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton created PR Review Comment:

Could you elaborate a bit on your use case for this? I would sort of expect that to override everything about allocation you'd have imported memories, but it sounds like your use case is also covering situations where the host wants to instantiate modules that define their own memory, but the host still wants to configure how those memories are allocated.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton created PR Review Comment:

FWIW this is making the wasmtime_runtime crate a public dependency of the wasmtime crate. We've previously avoided doing this because the wasmtime_* family of crate are intended to be far more lower level than the wasmtime crate.

That being said we haven't quite had a vector of customization like this before where completely dynamic behavior is needed. I suspect we're going to have these grow over time, but I figured it's worth pointing out that this has a few consequences for the wasmtime crate:

• This primarily means that if wasmtime_runtime has a major version bump, we have to bump the major version of wasmtime as well. It's expected that wasmtime_runtime will have lots of major version bumps because it's just a bunch of internals. For wasmtime, however, I think the idea is that eventually we reach a steady state where we just release new patch/minor versions.

• This is also relatively difficult to use since you'll have to match up, manually, with the version of wasmtime_runtime that wasmtime is using.

All that is to say that if we can avoid exposing this degree of a public dependency I think we should. One way, for example, would be to define traits in wasmtime as well as in wasmtime_runtime. The wasmtime crate would then provide its own implementation of the traits in wasmtime_runtime for provided types. This is a bit wonky, however, from our perspective and may not be the most trivial to manage.

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton created PR Review Comment:

The term "Allocator" feels a bit general here, would it be possible to tweak the naming here to indicate that this isn't like a malloc/free allocator but rather an allocator of linear memories?

Wasmtime GitHub notifications bot (Mar 25 2020 at 14:37):

alexcrichton created PR Review Comment:

In terms of safety something here needs to be unsafe due to the trickiness to get this all right (and segfaults which happen if there's a bug). I think all traits introduced here (both Allocator and LinearMemory) will need to be unsafe trait declarations since they have a number of safety guarantees to uphold in their implementations.

Wasmtime GitHub notifications bot (Mar 26 2020 at 02:46):

eust-dfinity submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 02:46):

eust-dfinity created PR Review Comment:

Our use case is in fact to control all memories used in modules, not just imported ones. Basically we monitor memory usage and track memory changes. Possible reallocations which may happen in wasmtime right now make it more difficult.

Wasmtime GitHub notifications bot (Mar 26 2020 at 02:51):

eust-dfinity submitted PR Review.

Wasmtime GitHub notifications bot (Mar 26 2020 at 02:51):

eust-dfinity created PR Review Comment:

Thank you for the info. I was meaning to ask about it as well.
I'll try to move things around, but first I'd rather come to a consensus on the general picture of how the whole thing should work internally.

There is also a question of MemoryPlan which resides in environ now.

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

eust-dfinity updated PR #1400 from host_memory to master:

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

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

alexcrichton submitted PR Review.

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

alexcrichton submitted PR Review.

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

alexcrichton created PR Review Comment:

How come this is wrapped in a RefCell?

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

alexcrichton created PR Review Comment:

I think it's safe to remove this and have as_ptr return *mut u8 since all linear memories need to support interior mutability anyway.

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

alexcrichton created PR Review Comment:

It might be good to leave a statement here too saying that this is a pretty new and experimental feature and it's recommended to be familiar with the code before using it. I'm not confident personally at least that this is an exhaustive set of what you need to guarantee for usage of this to be safe.

Wasmtime GitHub notifications bot (Apr 02 2020 at 23:39):

eust-dfinity updated PR #1400 from host_memory to master:

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

Wasmtime GitHub notifications bot (Apr 02 2020 at 23:59):

eust-dfinity submitted PR Review.

Wasmtime GitHub notifications bot (Apr 02 2020 at 23:59):

eust-dfinity created PR Review Comment:

This is complementary to:
https://github.com/bytecodealliance/wasmtime/blob/a325b62ade47540b4fe528d0c98e3c7dc1bfc042/crates/runtime/src/memory.rs#L147
Basically it's here because RuntimeLinearMemory::vmmemory() takes &self and not &mut self, but needs to return mut ptr.

Wasmtime GitHub notifications bot (Apr 03 2020 at 05:30):

eust-dfinity updated PR #1400 from host_memory to master:

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

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

eust-dfinity updated PR #1400 from host_memory to master:

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

Wasmtime GitHub notifications bot (Apr 03 2020 at 08:56):

eust-dfinity updated PR #1400 from host_memory to master:

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

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

eust-dfinity updated PR #1400 from host_memory to master:

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

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

alexcrichton merged PR #1400.

Last updated: Nov 22 2024 at 16:03 UTC