Wasmtime GitHub notifications bot (Aug 19 2021 at 17:56): Wasmtime GitHub notifications bot (Aug 19 2021 at 17:56):alexcrichton opened PR #3215 from dynamic-mem-grow to main:
Dynamic memories aren't really that heavily used in Wasmtime right now
because for most 32-bit memories they're classified as "static" which
means they reserve 4gb of address space and never move. Growth of a
static memory is simply making pages accessible, so it's quite fast.With the memory64 feature, however, this is no longer true since all
memory64 memories are classified as "dynamic" at this time. Previous to
this commit growth of a dynamic memory unconditionally moved the entire
linear memory in the host's address space, always resulting in a new
Mmapallocation. This behavior is causing fuzzers to time out when
working with 64-bit memories because incrementally growing a memory by 1
page at a time can incur a quadratic time complexity as bytes are
constantly moved.This commit implements a scheme where there is now a tunable setting for
memory to be reserved at the end of a dynamic memory to grow into. This
means that dynamic memory growth is ideally amortized as most calls to
memory.growwill be able to grow into the pre-reserved space. Some
calls, though, will still need to copy the memory around.This helps enable a commented out test for 64-bit memories now that it's
fast enough to run in debug mode. This is because the growth of memory
in the test no longer needs to copy 4gb of zeros.<!--
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 (Aug 19 2021 at 18:22):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 18:22):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 18:22):cfallin created PR review comment:
Would it make sense to consider an exponential growth scheme (like
Vec, continuing your analogy)? Something simple like doubling (or a factor of1.5or pick your favorite value) would give us amortized-constant growth cost.
Wasmtime GitHub notifications bot (Aug 19 2021 at 18:22):cfallin created PR review comment:
Also, I note a reference to
new_heapabove but I don't see it anywhere -- maybe this was meant to be a helper to adjust the request size somehow?
Wasmtime GitHub notifications bot (Aug 19 2021 at 19:05):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 19:05):alexcrichton created PR review comment:
I was wondering if I should have some sort of fancier selection of the growth algorithm here, but I ended up deciding against that and going with something simple for now, but likely to work for awhile since the default reservation size is 2gb.
I fear I don't personally know enough about how memory64 is expected to be used to know what the best option here is. I'd be afraid with a factor-based growth that you're always reserving 2x the virtual memory which may be a problem for some users. I'm semi-hoping that with a constant amount this is good enough to fly under some radars but immediately bad enough to get flagged by other radars to get a better solution here.
That being said I am also sort of primarily situation-solving a fuzz-bug found where we timed out in memory growth here. I'm hoping that the eventual ask is that even for 64-bit memories we still want "static" heaps where the static size is limited to >=4gb. For example we'd reserve 8gb and never move it but we wouldn't grow beyond 8gb (or something like that). In that scenario we wouldn't even hit the dynamic heap path since the maximum size would prove that unnecessary.
Overall how do you feel about sitting on a simple add-the-constant-in strategy for now? I think it may be best to write some of this up in the
Configdocumentation, basically saying "if this doesn't work for you please let us know and we can figure out a better solution"?
Wasmtime GitHub notifications bot (Aug 19 2021 at 19:06):alexcrichton updated PR #3215 from dynamic-mem-grow to main.
Wasmtime GitHub notifications bot (Aug 19 2021 at 19:20):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 19:20):cfallin created PR review comment:
Sure, a simple constant-growth scheme could be fine for now, if we're unsure about usage patterns and resource limits; we can always refine later.
Re: fuzzing specifically, I can foresee the fuzzer working out that there is a fixed increment beyond which the growth occurs, and then just incrementing by that amount each time in order to find another path to the timeout. This concern goes away if the overall limit imposed by fuzzing (at the process VM limit level or within wasmtime proper) is only a small multiple of the extra growth space; e.g. if we grow with an extra 2GB and the limit during fuzzing is 8GB then we can only grow four times, so that's fine.
Zooming out a bit, I do agree that setting a max growth limit and reserving that much VM space upfront is more likely the reasonable approach in the long run; it's hard for me to think of a heap-size limit that is large enough to be plausibly used in some use-case (very large movie-editing software, or in-memory cache, or...) at a few hundred GB or so, yet also too large to just reserve the address space upfront. But let's cross that bridge when we get there!
Wasmtime GitHub notifications bot (Aug 19 2021 at 22:39):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Aug 19 2021 at 22:39):alexcrichton created PR review comment:
Ok yeah I think I'll document our concerns here and probably go with this. I actually chose the 2gb default because it's bigger than our fuzz limit of 1gb so the fuzzers are guaranteed to never find this corner case. Or at least our fuzzers won't find it... But I agree that it's just pushing the problem back, not actually solving anything as of yet.
Wasmtime GitHub notifications bot (Aug 20 2021 at 14:59):alexcrichton updated PR #3215 from dynamic-mem-grow to main.
Wasmtime GitHub notifications bot (Aug 20 2021 at 15:54):alexcrichton merged PR #3215.
Last updated: Nov 22 2024 at 16:03 UTC