Wasmtime GitHub notifications bot (Jul 17 2023 at 14:50): Wasmtime GitHub notifications bot (Jul 17 2023 at 14:50):alexcrichton opened PR #6736 from alexcrichton:fix-fuzz-oom to bytecodealliance:main:
This commit is a fix for a fuzz failure that came in the other day where a module took more than 2GB of memory to compile, triggering the fuzzer to fail. Investigating locally it looks like the problem lies in the basic-block padding added recently. Turning on that option increases the memory usage of Cranelift from ~70M to ~2.5G. The setting in the test was to add
1<<14bytes of padding between all basic blocks, and with a lot of basic blocks that can add up quite quickly.The solution implemented here is to implement a per-function limit of the amount of padding that can be injected. I've set it to 1MB for now which is hopefully enough to continue to stress the various bits of the
MachBufferwhile not blowing limits accidentally while fuzzing.After this commit the fuzz test case in question jumps from 70M to 470M with basic block padding enabled, which while still large is a bit more modest and sticks within the limits of the fuzzer.
<!--
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
-->
Wasmtime GitHub notifications bot (Jul 17 2023 at 14:50):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6736.
Wasmtime GitHub notifications bot (Jul 17 2023 at 14:50):alexcrichton requested abrown for a review on PR #6736.
Wasmtime GitHub notifications bot (Jul 17 2023 at 15:08):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jul 17 2023 at 15:08):bjorn3 created PR review comment:
I think this would limit it to a single conditional branch where the branch destination doesn't fit and thus a veneer needs to be inserted on AArch64. Maybe bump it to 1 << 21?
Wasmtime GitHub notifications bot (Jul 17 2023 at 16:19):alexcrichton requested pchickey for a review on PR #6736.
Wasmtime GitHub notifications bot (Jul 17 2023 at 16:19):alexcrichton updated PR #6736.
Wasmtime GitHub notifications bot (Jul 17 2023 at 16:19):alexcrichton requested wasmtime-core-reviewers for a review on PR #6736.
Wasmtime GitHub notifications bot (Jul 17 2023 at 16:21):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Jul 17 2023 at 16:21):alexcrichton created PR review comment:
That's a good point yeah and was something I was worried about as well. I'm a bit worried though that bumping this to 21 (predictably) doubles the memory usage.
I've instead opted for a different strategy now:
- Each function gets at most 64M of padding
- Padding is only enabled for fuzz test cases with <10 functions
That should help continue to explore interesting cases while avoiding the large memory increases seen prior I think
Wasmtime GitHub notifications bot (Jul 17 2023 at 21:25):bjorn3 submitted PR review.
Wasmtime GitHub notifications bot (Jul 17 2023 at 21:25):bjorn3 created PR review comment:
*doesn't
Wasmtime GitHub notifications bot (Jul 18 2023 at 14:18):alexcrichton updated PR #6736.
Wasmtime GitHub notifications bot (Jul 18 2023 at 23:09):abrown submitted PR review:
I'm fine with this but wonder if @cfallin might have an opinion: limiting the Cranelift functions to a padded 64MB makes it possible that some of the larger islands never get fuzzed? IIRC, don't we have 2GB islands on x86?
Wasmtime GitHub notifications bot (Jul 19 2023 at 14:23):alexcrichton updated PR #6736.
Wasmtime GitHub notifications bot (Jul 19 2023 at 14:24):alexcrichton has enabled auto merge for PR #6736.
Wasmtime GitHub notifications bot (Jul 19 2023 at 15:44):alexcrichton merged PR #6736.
Last updated: Nov 22 2024 at 16:03 UTC