Stream: git-wasmtime

Topic: wasmtime / PR #4984 SSA-building cleanup

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:06):

jameysharp opened PR #4984 from ssa-cleanup to main:

This is a collection of all the cleanups I set aside while working on #4955 and #4966. According to DHAT and hyperfine, these patches together have almost no effect on performance. DHAT says this reduces total memory allocated, max heap size, and bytes written, while increasing instructions retired and bytes read, but all by tiny fractions of a percent.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:06):

jameysharp requested cfallin for a review on PR #4984.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:17):

cfallin created PR review comment:

I think this results in a change in behavior with respect to before: earlier we had an assert that ensured a block could only be sealed once (line 437 in the original); now we silently accept Yes here. That is probably OK -- the effect is nothing if already sealed -- but I just want to make sure we've thought about this and that it doesn't contradict docs elsewhere?

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:17):

cfallin created PR review comment:

I wonder: is this actually necessary? SecondaryMap (or at least, one created with ::with_default()) will return a readonly borrow of some shared "default" value for an Index of a non-existing index, or will dynamically expand as needed for an IndexMut access; the lack of prior expansion is unobservable (as opposed to just-in-time expansion), I think. Or is that not the case?

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:27):

jameysharp created PR review comment:

In terms of public API, the behavior is unchanged. I moved the assert to seal_block; and seal_all_blocks was only calling seal_one_block if the block was not already sealed.

Making the internal helpers just no-op for sealed blocks meant that I could remove a conditional from seal_all_blocks and check only once in mark_block_sealed instead.

As far as private callers go, append_jump_argument calls mark_block_sealed but only on a block it just created, so it definitely isn't sealed.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:36):

jameysharp created PR review comment:

I almost had myself convinced that I could delete declare_block entirely, because as you say, the allocated size of a SecondaryMap is unobservable... almost.

The exception is SecondaryMap::keys, which iterates only over indexes that are currently initialized. And seal_all_blocks uses self.ssa_blocks.keys() to determine which blocks need to be sealed.

So if I don't ensure that the map has grown at least to the declared block's index, then I think there's a sequence of public API calls against a FunctionBuilder that would hit a debug assertion. It's something like create_block, switch_to_block, ensure_inserted_block, and finalize, without declaring any predecessors or calling use_var.

I didn't specifically test that because it was starting to make my head hurt. So maybe it's actually okay, but I didn't want to think about it further.

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:39):

jameysharp updated PR #4984 from ssa-cleanup to main.

Wasmtime GitHub notifications bot (Sep 29 2022 at 23:59):

jameysharp merged PR #4984.

Last updated: Nov 22 2024 at 16:03 UTC