Stream: git-wasmtime

Topic: wasmtime / PR #4086 runtime: refactor `Memory` to always ...

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:11):

abrown opened PR #4086 from refactor-static-memory to main:

While working with the runtime Memory object, it became clear that
some refactoring was needed. In order to implement shared memory from
the threads proposal, we must be able to atomically change the memory
size. Previously, the split into variants, Memory::Static and
Memory::Dynamic, made any attempt to lock forced us to duplicate logic
in various places.

This change moves enum Memory { Static..., Dynamic... } to simply
struct Memory(Box<dyn RuntimeLinearMemory>). A new type,
ExternalMemory, takes the place of Memory::Static and also
implements the RuntimeLinearMemory trait, allowing Memory to contain
the same two options as before: MmapMemory for Memory::Dynamic and
ExternalMemory for Memory::Static. To interface with the
PoolingAllocator, this change also required the ability to downcast to
the internal representation.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:11):

abrown requested alexcrichton for a review on PR #4086.

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:13):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:16):

abrown edited PR #4086 from refactor-static-memory to main:

While working with the runtime Memory object, it became clear that
some refactoring was needed. In order to implement shared memory from
the threads proposal, we must be able to atomically change the memory
size. Previously, the split into variants, Memory::Static and
Memory::Dynamic, made any attempt to lock duplicate logic
in various places.

This change moves enum Memory { Static..., Dynamic... } to simply
struct Memory(Box<dyn RuntimeLinearMemory>). A new type,
ExternalMemory, takes the place of Memory::Static and also
implements the RuntimeLinearMemory trait, allowing Memory to contain
the same two options as before: MmapMemory for Memory::Dynamic and
ExternalMemory for Memory::Static. To interface with the
PoolingAllocator, this change also required the ability to downcast to
the internal representation.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

I think this can be an assert since this should have already been checked in Memory::grow

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

I think this may actually be historical at this point, while you're here could you try removing this (and dependants) and see if it's actually still needed?

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

I think with my above suggestion this could ideally be a private-to-this-module structure

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

(I think this was also present in the original code where this check wasn't necessary)

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

Putting the downcast here and dealing with ExternalMemory I think would be better inside of memory.rs because otherwise it's created via Memory::new_static but then open-coded here that we're reaching into the internals.

Perhaps something like Memory::unwrap_static_image which returns Option<MemoryImage> which is used here to call clear_and_remain_ready and such?

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

If possible, like above, I think it'd be best for this to not be pub(crate) but that may not be possible (if it's needed it's needed, can fix later)

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

Could this get folded into ExternalMemory::new?

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

This happens below as well so I think this one can be removed

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:31):

alexcrichton created PR review comment:

These last two statements are pretty good now to be a match perhaps? (since handling both cases is trivial)

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:58):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 18:58):

abrown created PR review comment:

I tried... mem::take in pooling.rs requires a Memory to be Default. Is there another way to do something equivalent there?

Wasmtime GitHub notifications bot (Apr 28 2022 at 19:23):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 19:23):

alexcrichton created PR review comment:

Ah that can be replaced with mem::take of the memories list itself rather than each individual memory I thin

Wasmtime GitHub notifications bot (Apr 28 2022 at 19:23):

alexcrichton edited PR review comment.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:25):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:25):

abrown created PR review comment:

I see other checks in Memory::grow but none that are equivalent to this one. Do you mean that we should move this check back to the Memory::grow function and duplicate it here and in MmapMemory as a debug_assert!?

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:29):

alexcrichton created PR review comment:

I think it's covered by this check since this variant always has a maximum_byte_size of Some, but yeah here I think this check can be an assert! or a debug_assert!

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:38):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:38):

abrown created PR review comment:

Ok, I did it. I was hoping I could get rid of RuntimeLinearMemory::into_any but I don't see how: I still need to be able to convert Box<dyn RuntimeLinearMemory> to Box<dyn Any> in the new Memory::unwrap_image_slot. Let me know if there's something preferable... I do see a lot more #[cfg(feature = "pooling-allocator")] here.

I also felt like I had to add back in a way to assert that only external memory is used in the pooling allocator, as was done previously; see the new Memory::is_external.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:39):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:40):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:40):

abrown created PR review comment:

Can we? memory is coming from self.memories.get(instance_index) so I don't think I can just take from the iterator? Maybe I'm just not seeing it...

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:48):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 20:55):

abrown created PR review comment:

I might be doing this wrong... I'm trying to get to a &dyn Any so I can check the downcast is of the right kind but I'm concerned that I have the wrong type. I see a failure building the benchmarks that I don't think should happen. Rust help needed!

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:11):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:11):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:11):

alexcrichton created PR review comment:

Yeah I don't think this is correct, you'll need to change the trait's into_any method into something like as_any and use that to get the &Any to test

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:11):

alexcrichton created PR review comment:

Yes with memory: &mut PrimaryMap<K, V> you can mem::take. Only the base comes from self.memories.get(instance_index)

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:12):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 21:12):

alexcrichton created PR review comment:

I'd personally prefer to make "external" memory an internal implementation detail of the memory.rs module if we can. Right now it has a Memory::new_static constructor so in theory we would have something like Memory::unwrap_static_image where we keep the "static" name and the unwrap bit implies the assertion and the method takes out the input argument to Memory::new_static

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:05):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:07):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:07):

abrown created PR review comment:

Ok, had to add RuntimeLinearMemory::as_any (even more pooling-allocator-specific trait functions!) but that seems to be correct. I can't use into_any because that takes a Box<Self>.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:08):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:08):

abrown created PR review comment:

Oh, you're right! Sorry, wasn't looking closely enough.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:12):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:13):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:22):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:22):

abrown created PR review comment:

Ok, I changed all of the pub functions to use the same naming: is_static and unwrap_static_image now match new_static. I left the struct as ExternalMemory (instead of StaticMemory, though I can change it to that if so desired) because I started to feel that this static/dynamic nomenclature here isn't actually the most accurate. Sure, the old Memory::Static had a static starting address, but it turns out Memory::Dynamic could as well (e.g., MemoryStyle::Static)! Static/dynamic are rather overloaded terms anyways, so I started thinking of this structure in terms of external/internal. External memory is managed "outside the engine" by, e.g., the pooling allocator, and internal memory is managed directly by the engine; this mental switch helped me understand more clearly why we need more than one option here and perhaps it would be the same for others. What do you think? If you agree, I can take up this renaming in a separate PR.

Wasmtime GitHub notifications bot (Apr 28 2022 at 22:22):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Apr 29 2022 at 13:56):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 29 2022 at 14:19):

abrown updated PR #4086 from refactor-static-memory to main.

Wasmtime GitHub notifications bot (Apr 29 2022 at 15:12):

abrown merged PR #4086.

Last updated: Oct 23 2024 at 20:03 UTC