Proposal to rework MemoryCreator and remove internal types. #14989

Building on #14885, I'd like to tackle the other half of the linear memory traits. This is already a FIXME in the memory code:

Since it involves a breaking public API change I thought I'd first go through an issue before I go into slashing the codebase.

Feature

The change would remove RuntimeMemoryCreator and MemoryCreatorProxy. The DefaultMemoryCreator struct would be implemented wholly in terms of the public MemoryCreator trait.

This requires a breaking change to the MemoryCreator. First, the DefaultMemoryCreator receives reservation_for_growth, which is currently not present on the public trait's new_memory. Second, more minor, the current trait requires to return a Result<_, String> instead of the standard wasmtime error type, so fidelity would be lost as the DefaultMemoryCreator would convert all strongly-typed errors to string.

Benefit

Simplifies the code base, fixes an outstanding FIXME, avoids running into issues similar to #10740 because of a mismatch of how the user can implement the memory backend from the outside and internal-only shortcuts.

Implementation

My proposal would be to change the public API so that new_memory task a single parameter, MemoryRequest. That way things can be added as functions on the type without breaking current implementations. Public accessors on the request would return all things the creator needs to serve the request:

impl<'a> MemoryRequest<'a> {
  pub fn ty(&self) -> MemoryType;
  pub fn minimum(&self) -> usize;
  pub fn maximum(&self) -> Option<usize>;
  pub fn reserved_size_in_bytes(&self) -> Option<usize>;
  pub fn guard_size_in_bytes(&self) -> usize;
}

The return type turns into the standard wasmtime Error. This is not really an issue, a user of wasmtime already has to cope with the wasmtime Error anywhere, I don't think there's any advantage to allowing Result<_, String> as it currently is.

Internally, RuntimeMemoryCreator is gone, as is MemoryCreatorProxy. Config holds an Arc<dyn MemoryCreator> instead. The implementation that creates memory in vm::Memory will construct a MemoryRequest and route it to the creator.

Not all things are exposed publicly, I don't necessarily want to increase the public API surface here. The DefaultMemoryCreator needs MemoryTunables for some of the config as well as ty: &wasmtime_environ::Memory. These are not public types, so I'd keep them on MemoryRequest exposed as pub(crate), but not to the outside.

Alternatives

There's a few things one could consider changing:

1. The documentation now doesn't mention that, but the custom creator is also used for GC heaps. An implementation might want to serve these differently, because their purpose and access patterns are different, but currently there's no way to distinguish that from the public parameters. A simple pub fn kind(&self) returning a non-exhausting MemoryKind enum with LinearMemory and GcHeap fixes this.
2. The other parameters are passed via tunables and not exposed. For example, MmapMemory uses reservation_for_growth and guard_before_linear_memory, plus signals_based_traps and memory_init_cow to decide whether to use Mmap or Malloc-backed memory.

I also have one open question: reserved_size_in_bytes is an Option<usize>, but the engine always passes Some, with Some(0) meaning "no reservation". Is there any reason for this? What would be the semantic difference between Some(0) and None? Can we just make it a usize?