Optimize OperatorsReader control stack

[Robbepop]

Alternative or addition to #2228.

The OperatorsReader control stack that has been introduced in #2134 always contains at least one frame which causes this common path to always require heap allocations. Since the OperatorsReader type is used pervasively during parsing, validation and deconstructing init-expressions (or const-expressions) those heap allocations quickly sum up. Especially large Wasm modules could have hundreds or tousands of init-expressions.

This PR introduces a new ControlStack type that optimizes this by not requiring heap allocations for just a single frame on the control stack.

Inline annotations have been added where necessary to improve performance. Every such annotation has been benchmarked.

Benchmarks comparing main with this PR:

cc @alexcrichton @keithw

[alexcrichton]

Thanks! I'm personally a bit hesitant to go the route of micro optimizations like this just yet mostly insofar that I don't believe this composes with #2228. If doing a pure parse in isolation then this complements #2228 but if doing a parse with a validator this won't be used (and that's the main case that I'd like to optimize for)

[Robbepop]

@alexcrichton thanks for your reply. It seems that I have overlooked the fact that #2228 also provides a solution for init-expressions. When I have reviewed #2228 some days ago I didn't notice that. Though, this PR might still have positive impacts on performance since it inlines the top-most block and simplifies some users that do not care about the initialization of the operators reader - though in all honesty for those users performance might not be important.

Due to the results in #2180 (comment) where boths PRs showed improvements in different test cases I was under the impression that both PRs compose well with each other.

[alexcrichton]

To be clear I agree this PR improves performance, but what it's improving, assuming #2228 is merged, is parse-without-validation performance. The validator won't use this optimization at all because it has its own stack of frames which doesn't have the top frame cached in the structure. Put another way the gains on validate/spidermonkey won't be additive, it's a this-pr-or-that-pr in terms of performance gains.

[Robbepop]

To be clear I agree this PR improves performance, but what it's improving, assuming #2228 is merged, is parse-without-validation performance. The validator won't use this optimization at all because it has its own stack of frames which doesn't have the top frame cached in the structure. Put another way the gains on validate/spidermonkey won't be additive, it's a this-pr-or-that-pr in terms of performance gains.

I get that those PRs focus on different areas.

Though, to Wasmi, parse-without-validation performance matters too, since performance sensitive users can skip validation with this API which is used by some users when they can assert otherwise that they are operating with pre-validated Wasm binaries: https://docs.rs/wasmi/latest/wasmi/struct.Module.html#method.new_unchecked

I can understand if you do not want to go into the direction of this PR. Maybe a different solution is needed.

[alexcrichton]

Ah ok, that makes sense. With the FrameStack trait added in #2228 it should in theory be possible to build the optimization in this PR both in-tree and out-of-tree. The implementation in-tree should be pretty minimal though so seems ok to add too. Would you be up for rebasing this?

[Robbepop]

Ah ok, that makes sense. With the FrameStack trait added in #2228 it should in theory be possible to build the optimization in this PR both in-tree and out-of-tree. The implementation in-tree should be pretty minimal though so seems ok to add too. Would you be up for rebasing this?

Sure, I will rebase on top of #2228 and see how the combined changes impacts benchmarks.

[keithw]

@Robbepop, I didn't realize Wasmi had a new_unchecked mode -- that's cool.

I took a look at how Wasmi is using wasmparser, and I think it may be possible for Wasmi to avoid ever creating awasmparser::OperatorsReader (and its internal Vec<FrameKind>) at all at this point. I think the FuncTranslator can probably implement the FrameStack trait using its ControlStack member, and then call BinaryReader::visit_operator directly. This is similar to what the wasmparser FuncValidator does. I'm hopeful this would get the Wasmi benchmarks back to where they were before #2134.

[Robbepop]

@Robbepop, I didn't realize Wasmi had a new_unchecked mode -- that's cool.

Thanks! For Wasmi the new_unechecked module creation kinda is more useful than it probably is in Wasmtime since in combination with Wasmi's lazy translation it can translate Wasm modules so quickly that caching of "precompiled" serialized artifacts isn't even needed. This is why I care so much about wasmparser performance.

I took a look at how Wasmi is using wasmparser, and I think it may be possible for Wasmi to avoid ever creating awasmparser::OperatorsReader (and its internal Vec<FrameKind>) at all at this point. I think the FuncTranslator can probably implement the FrameStack trait using its ControlStack member, and then call BinaryReader::visit_operator directly. This is similar to what the wasmparser FuncValidator does. I'm hopeful this would get the Wasmi benchmarks back to where they were before #2134.

Yes you are right about how or where Wasmi can make use of the new FrameStack trait. I am eager to see how this impacts performance. Unfortunately, I won't really have time to do this until next week. I will inform you both here or in the other thread once I got there.

[Robbepop]

I think the FuncTranslator can probably implement the FrameStack trait using its ControlStack member, and then call BinaryReader::visit_operator directly.

@keithw I checked today. Is it possible to change the signature of FrameStack::current_frame from Option<&FrameKind> to Option<FrameKind>? This would allow to efficiently convert from other representations of frames into wasmparser::FrameKind. Otherwise, I'd need to use wasmparser::FrameKind which is not very usable for Wasmi's FuncTranslator otherwise.

[keithw]

Sure, no objection from me.

[Robbepop]

@alexcrichton I rebased this PR on main.

Benchmarks show that validation benchmarks are not affected but parse benchmarks are, as expected:

[Robbepop]

I resolved conflicts and rebased on main. cc @alexcrichton