Stream: git-wasmtime

Topic: wasmtime / PR #3737 Pooling allocator: make backend (mmap...

Wasmtime GitHub notifications bot (Jan 28 2022 at 04:42):

cfallin opened PR #3737 from pooling-backend-dynamic-choice to main:

(Stacked on top of #3697)

Currently, the pooling allocator makes heavy use of compile-time
features to determine its runtime behavior and allocation strategy. This
makes testing harder, and makes runtime configuration from higher levels
of the system impossible.

This PR passes a PoolingBackend value down from the configuration
plumbing and into the pooling allocator, and uses its value to
dynamically choose a backend instead. This is mostly a matter of lots of
plumbing and conditionals.

The most complex part of this PR is actually the testing and feature
setup. We still want the memfd and uffd backends to be optional -- i.e.,
one should be able to build a wasmtime binary without them included.
This implies not including the enum options in the config enum, and
everything down from there.

In order to test all possible situations, we actually need to test four
builds -- with neither uffd nor memfd, with just one, or with both. We
also want to test that everything works when different backends are
specified, and the simplest way to do this is just to run the whole test
suite with the default backend set to each possible option. Hence we run
tests with both backends included and the default set three different
ways; with one backend included and the default set to that or the
allback mmap backend; etc.

I'm not sure that this is actually the simplest option -- it may, in the
end, be simpler to entirely remove uffd, if we eventually determine that
memfd supersedes it, and then unconditionally include memfd support on
Linux. But it was suggested here [1] to opt for a dynamic approach, so
this PR explores what that would look like!

[1] https://github.com/bytecodealliance/wasmtime/pull/3697#discussion_r792246996

Wasmtime GitHub notifications bot (Jan 28 2022 at 04:42):

cfallin requested alexcrichton for a review on PR #3737.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

I definitely don't think we should have to do this in CI. There's no way we can do the whole matrix here so if we feel we need to then I think the design should be tweaked elsewhere.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

I think that this API for configuring the pooling allocator is a bit overly-wordy and we would probably want to move to a builder-style configuration system if we added another top-level configuration option like this.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

I agree that the need to do something like this is definitely overkill, althought the suggestion below of impl-as-methods-on-something might cut down on the wordiness here.

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

If we go the runtime-value-PoolingBackend route I think this would make more sense as:

impl PoolingBackend {
    unsafe fn make_accessible(&self, ...) { ... }
}

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

And another possible way to organize this is something like:

enum PoolingBackend {
    Mmap(NativeMmap),
    Uffd(Uffd),
    Memfd(Memfd),
}

struct NativeMmap;

impl NativeMmap {
    // methods here, probably one impl of `NativeMmap` per-OS
}

enum Memfd {
    #[cfg(feature = "memfd")]
    Supported,
}

impl Memfd {
    fn new() -> Result<Memfd> {
        #[cfg(feature = "memfd")]
        return Ok(Memfd::Supported);
        #[cfg(not(feature = "memfd"))]
        bail!("support not compiled in (or os not supported");
    }

    fn some_method(&self) -> Result<()> {
        match self {
            #[cfg(feature = "memfd")]
            Memfd::Supported => {
                // the implementation
            }
        }
    }
}

I think this would be a more manageable way to handle platform-specific and feature-specific functionality perhaps?

Wasmtime GitHub notifications bot (Jan 28 2022 at 19:08):

alexcrichton created PR review comment:

Personally I don't really feel the need to have separate features for default backends. If this is purely for test coverage then we could have normal cargo test just loop through the backends or otherwise just pick a random one for each test so we get more coverage over time or something like that.

Wasmtime GitHub notifications bot (Feb 09 2022 at 21:59):

cfallin closed without merge PR #3737.

Last updated: Dec 23 2024 at 12:05 UTC