Skip to content

Only record vreg definitions when fuzzing#66

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
jameysharp:only-fuzz-ssa
Jul 29, 2022
Merged

Only record vreg definitions when fuzzing#66
cfallin merged 1 commit intobytecodealliance:mainfrom
jameysharp:only-fuzz-ssa

Conversation

@jameysharp
Copy link
Copy Markdown
Contributor

The vreg_def_blockparam and vreg_def_inst fields on CFGInfo are only
used in validate_ssa, which in turn is only used in the ssagen fuzz
target.

Since these fields are never read in normal usage, initializing them is
entirely wasted effort. According to valgrind/DHAT, when running
wasmtime compile on the Sightglass Spidermonkey benchmark, removing
these fields saves about 100M instructions, 23k heap allocations
totalling 40MiB, and 47MiB of writes to the heap.

I tested the fuzz target for a few minutes, and also tested a release build of wasmtime. So I've built with the fuzzing feature both on and off, and there weren't any build warnings or errors in either case.

@jameysharp jameysharp requested a review from cfallin July 28, 2022 20:10
Copy link
Copy Markdown
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

Rather than fine-grained cfgs on individual statements, a pattern that I like to follow for easier maintainability and less ifdef-soup (and one that I picked up from Alex during memfd work on Wasmtime) is to make them more coarse-grained: either alternate implementations of structs that are empty, with empty/dummy methods, or alternate modules.

In the extreme, SSAInfo would just move into the ssa module entirely; doing a separate pass to fill it in isn't a huge deal, since this only affects the ssagen-target fuzzing flow and the SSA check is relatively heavyweight anyway. The only reason it's still intertwined with the CFGInfo analysis after this PR's changes appears to be to share the loop over blocks and instructions within a block; those are simple enough that we don't need to deduplicate complexity.

Alternately we could have a separate SSAInfo struct that's empty, for #[cfg(not(feature = "fuzzing"))], and an impl on it, also cfg'd, that has empty methods. But I prefer the above option actually; there's no reason for SSAInfo to live in cfg.rs at all.

The `vreg_def_blockparam` and `vreg_def_inst` fields on CFGInfo are only
used in `validate_ssa`, which in turn is only used in the ssagen fuzz
target.

Since these fields are never read in normal usage, initializing them is
entirely wasted effort. According to valgrind/DHAT, when running
`wasmtime compile` on the Sightglass Spidermonkey benchmark, removing
these fields saves about 100M instructions, 23k heap allocations
totalling 40MiB, and 47MiB of writes to the heap.
Copy link
Copy Markdown
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is much cleaner -- thanks a bunch!

@cfallin cfallin merged commit 1955c6d into bytecodealliance:main Jul 29, 2022
@jameysharp jameysharp mentioned this pull request Jul 29, 2022
jameysharp added a commit to jameysharp/regalloc2 that referenced this pull request Aug 3, 2022
Includes a modest improvement in memory usage and performance by
removing analysis that was only used during fuzzing (bytecodealliance#66).
@jameysharp jameysharp mentioned this pull request Aug 3, 2022
cfallin pushed a commit that referenced this pull request Aug 3, 2022
Includes a modest improvement in memory usage and performance by
removing analysis that was only used during fuzzing (#66).
@jameysharp jameysharp deleted the only-fuzz-ssa branch August 9, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants