wasmtime / PR #8628 Use bytes for maximum size of linear ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #8628 Use bytes for maximum size of linear ...

Wasmtime GitHub notifications bot (May 15 2024 at 17:57):

alexcrichton opened PR #8628 from alexcrichton:memory-pages-to-bytes to bytecodealliance:main:

This commit changes configuration of the pooling allocator to use a byte-based unit rather than a page based unit. The previous PoolingAllocatorConfig::memory_pages configuration option configures the maximum size that a linear memory may grow to at runtime. This is an important factor in calculation of stripes for MPK and is also a coarse-grained knob apart from StoreLimiter to limit memory consumption. This configuration option has been renamed to max_memory_size and documented that it's in terms of bytes rather than pages as before.

Additionally the documented constraint of max_memory_size must be smaller than static_memory_bound is now additionally enforced as a minor clean-up as part of this PR as well.

Wasmtime GitHub notifications bot (May 15 2024 at 17:57):

alexcrichton requested fitzgen for a review on PR #8628.

Wasmtime GitHub notifications bot (May 15 2024 at 17:57):

alexcrichton requested wasmtime-fuzz-reviewers for a review on PR #8628.

Wasmtime GitHub notifications bot (May 15 2024 at 17:57):

alexcrichton requested wasmtime-core-reviewers for a review on PR #8628.

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen submitted PR review:

LGTM but I am confused about one thing and have Opinions and Concerns about writing the constant value directly all over the place

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen submitted PR review:

LGTM but I am confused about one thing and have Opinions and Concerns about writing the constant value directly all over the place

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

This should be 65536 right? If you instead want the max address it should be 10 * 65536 - 1.

Also lets use named constants for the Wasm page size so that it is easier to find uses and update in the future for the custom-page-sizes proposal. We have it defined in wasmtime_types already, FWIW, and this could use that here: https://github.com/bytecodealliance/wasmtime/blob/c477424f45871563be02eba14815ba3446158441/crates/types/src/lib.rs#L1493

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

ditto regarding named constant

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

named constant, even tho it's just a test. we want to make sure we are actually testing sizes that wasm could potentially grow to, and not accidentally just testing rounding behavior or something.

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

ditto in this file's changes

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

Also confused about 65535 here since that isn't a multiple of the page size, so this will disallow any non-zero-length memories, assuming we don't do rounding elsewhere in this PR?

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

        /// The maximum runtime size of each linear memory in the pooling
        /// allocator, in bytes.
        pub pooling_max_memory_size: Option<usize>,

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

    /// The maximum byte size that any WebAssembly linear memory may grow to.

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

ditto

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

Also confused about 65535 vs 65536 here.

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

And ditto about named constants on everything here.

Wasmtime GitHub notifications bot (May 15 2024 at 18:28):

fitzgen created PR review comment:

okay I am going to stop saying "ditto about named constants" now and assume you will go over the diff yourself to catch all of them at this point

Wasmtime GitHub notifications bot (May 15 2024 at 18:45):

github-actions[bot] commented on PR #8628:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "fuzzing", "wasmtime:api", "wasmtime:config"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

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

Learn more.
</details>

Wasmtime GitHub notifications bot (May 15 2024 at 18:45):

github-actions[bot] commented on PR #8628:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

[ ] If you added a new Config method, you wrote extensive documentation for
it.

<details>

Our documentation should be of the following form:

```text
Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

Example

Optional example here.
```

</details>

[ ] If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

<details>

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

</details>

[ ] If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (May 16 2024 at 14:45):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 16 2024 at 14:45):

alexcrichton created PR review comment:

Ah yeah 65535 is a typo here, and everywhere else, but I'm hesitant to go through all the work to import a constant in all these places since it's all in tests anyway and all the values are basically random constants. In the future the wasm page size won't be constant as well so it feels weird to proliferate usage of the constant... I can change these to >>16 and <<16 though where appropriate perhaps to reduce the risk of typos.

Wasmtime GitHub notifications bot (May 16 2024 at 15:13):

alexcrichton created PR review comment:

Could you expand on this? I could see a comment going here but I'm not sure otherwise what you're looking for in terms of a constant?

Wasmtime GitHub notifications bot (May 16 2024 at 15:13):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 16 2024 at 15:26):

alexcrichton updated PR #8628.

Wasmtime GitHub notifications bot (May 16 2024 at 15:29):

alexcrichton commented on PR #8628:

I've updated with <<16 and >>16 to reduce the risk of typos, but I'm curious what you think of my pushback on using a name constant everywhere. I'm basically hesitant to add a bunch of process to tests where all the constants are basically already random

Wasmtime GitHub notifications bot (May 16 2024 at 16:19):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 16 2024 at 16:19):

fitzgen created PR review comment:

If the goal is to test behavior with N wasm pages of capacity, then we should make sure we are actually doing that (as opposed to testing with unaligned capacity) and using a constant helps in this scenario. I feel like the typos are evidence that we can mess this up.

Additionally, having a constant that we can grep for all uses of will make it easier to support the custom-page-sizes proposal down the line: we have a list of uses that we can turn into method calls if necessary, and a list of tests that we might want to update or copy and tweak.

Wasmtime GitHub notifications bot (May 16 2024 at 16:21):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 16 2024 at 19:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 16 2024 at 19:35):

alexcrichton created PR review comment:

The meaning of this test has sort of been updated and lost a bit over time as I've been tweaking settings. This isn't specifically testing for unalignment or anything like that it's just one size being bigger than the other and various bits and pieces have been lost to translation over time.

I'm gonna go ahead and merge this as-is but we can of course update tests afterwards. I'm basically just somewhat doubtful if it's worth the effort to try to document all tests for precisely what they're doing page-wise and such. I don't think any of the tests really need to be updated with custom-page-sizes since they're all using wasm memories of 64k pages and the tests for custom-page-sizes are going to be pretty orthogonal anyway. Much of the time the page limits are just to reduce VM memory reservations anyway and don't have much to do with page sizes.

Wasmtime GitHub notifications bot (May 17 2024 at 03:58):

alexcrichton requested pchickey for a review on PR #8628.

Wasmtime GitHub notifications bot (May 17 2024 at 03:58):

alexcrichton requested wasmtime-default-reviewers for a review on PR #8628.

Wasmtime GitHub notifications bot (May 17 2024 at 03:58):

alexcrichton updated PR #8628.

Wasmtime GitHub notifications bot (May 17 2024 at 03:58):

alexcrichton has enabled auto merge for PR #8628.

Wasmtime GitHub notifications bot (May 17 2024 at 04:20):

alexcrichton merged PR #8628.

Last updated: Nov 22 2024 at 16:03 UTC