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 fromStoreLimiter
to limit memory consumption. This configuration option has been renamed tomax_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 thanstatic_memory_bound
is now additionally enforced as a minor clean-up as part of this PR as well.<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested fitzgen for a review on PR #8628.
alexcrichton requested wasmtime-fuzz-reviewers for a review on PR #8628.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8628.
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
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
fitzgen created PR review comment:
This should be
65536
right? If you instead want the max address it should be10 * 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
fitzgen created PR review comment:
ditto regarding named constant
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.
fitzgen created PR review comment:
ditto in this file's changes
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?
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>,
fitzgen created PR review comment:
/// The maximum byte size that any WebAssembly linear memory may grow to.
fitzgen created PR review comment:
ditto
fitzgen created PR review comment:
Also confused about
65535
vs65536
here.
fitzgen created PR review comment:
And ditto about named constants on everything here.
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
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>
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 nestedstruct
s).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.</details>
alexcrichton submitted PR review.
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.
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?
alexcrichton submitted PR review.
alexcrichton updated PR #8628.
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
fitzgen submitted PR review.
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.
fitzgen submitted PR review.
alexcrichton submitted PR review.
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.
alexcrichton requested pchickey for a review on PR #8628.
alexcrichton requested wasmtime-default-reviewers for a review on PR #8628.
alexcrichton updated PR #8628.
alexcrichton has enabled auto merge for PR #8628.
alexcrichton merged PR #8628.
Last updated: Dec 23 2024 at 12:05 UTC