wasmparser: VisitOperator API

[Robbepop]

Closes #695.

@alexcrichton Review should follow the commits. I added commit messages to make understanding of the changes simpler. Large parts of the code have been automatically generated and bulk transformed.

The major downside of this approach is that the VisitOperator trait is really ... huge and there currently is no way for users to implement just parts of it. However, that should probably not matter much for the projects of the BytecodeAlliance. I would not recommend default implementing the methods. Instead users should take care of this work.

A minor downside is that maintainers of the wasmparser crate now have to sync changes in the Operator variants for the identifiers of the VisitOperator trait. Therefore we should strife to create a proc. macro based solution in the future but I deemed it out of scope for this already big PR.

[alexcrichton]

This is looking good, thanks! One worry I have is that this is introducing two ways to do the same thing that we need to keep in sync an up-to-date. I think that the overhead here is somewhat low because it's easy to spot mistakes and such but I want to make sure that everything is at least thoroughly tested. For example I don't think there should be two different paths that read the binary encoding of an operator. The style implemented in the operator validator where Operator is deleted to visit methods I think makes sense.

I unfortunately can't open validator/operators.rs in a browser to provide more detailed feedback:

• Could the VisitOperator trait have a method visit_operator which delegates to the appropriate visit method internally? This could then be used by the operator validator I believe.
• Passing around resources: &I seems pretty onerous. If you'd like I think we could move ownership of the resources into the operator validator instead of having it separately owned in the function validator.
• If possible I'd ideally like to remove the dependence on Operator if we can. Given this PR is there any reason for it to stick around?

For benchmarks the in-repository benchmark isn't really that good I think, do you have more representative numbers on a more idiomatic module perhaps? I suspect that they're still wins, but it may be bigger than 10% since the modules in this repository are all test files which have few instructions.

[alexcrichton]

This should totally work but kinda feels like it defeats the purpose of the VisitOperator trait

Ah this is just a convenience method if Operator sticks around, I don't expect high-perf cases to call it (e.g. validation). And yes this would be a default method so only one location would have to worry about defining it.

Maybe we could go a smooth transition using deprecation of the "old" techniques first and in the next release remove them?

This crate does not provide a strong API stability guarantee, there's a whole lot of major versions and new things get added all the time. This basically isn't a factor when designing this crate. Instead we've been designing this crate as "keep it up to day with abstractions that all fit together", which is why given this trait I'm not sure we still need an Operator enum since it doesn't really make sense any more. If you'd prefer to leave it in that's ok because I agree it's work to remove, but I would at least want to try to remove duplication of decoding if not other duplicate pieces of functionality. Everything should still be tested in this crate at the very least.

There benchmarks show that compiling and validating it using wasmi via wasmparser takes roughly 6ms which is a lot and also the reason why I started this investigation

Is that 6ms before this change or after this change? Are you able to see other bottlenecks in wasmparser?

[alexcrichton]

I'm ok deferring dealing with Self::Output as a Result to later myself. Having to do ?? isn't the end of the world. Ideally though I'd like to move to something that looks like:

type Ok;
type Err: From<BinaryReaderError>;

which I think should solve your use case?

Otherwise I have posted Robbepop#2 to simplify the operator validator (in my mind at least). The main goal of the operator validator is to be as absolutely simple and close to the spec as we can manage. This is critical for correctness so having extra layers of abstractions and distinctions like push_ctrl vs push_control can be confusing. I also took the opportunity to stop passing around &resources everywhere and all "meaty" methods are now just on OperatorValidatorTemp which has struct field access to the resources.

[alexcrichton]

Er sorry and to clarify with Robbepop#2 I personally feel this is good to go and iteration can continue in-tree.

[Robbepop]

which I think should solve your use case?

Yes that would be fine for my use case.

Thank you for the review and for creating the issue which allows us to merge this PR and defer the rest of the work. I understand your concern with keeping the operator validator as simple as possible.

I will review your changes and push them to update this PR for merge.
Your simplifications looks fine to me and actually improve readability - cool!

[Robbepop]

@alexcrichton I just benchmarked the PR with your simplifications and noticed that performance drastically went down from 18% improvements to just 12% improvements. Even if you are not convinced by the wasmparser benchmarks this difference is quite significant unfortunately ...

Usually while working on this PR I was implementing everything in steps and checking performance after each step to assert constant improvement. However, this was not possible with your changes as everything was squashed into a single commit. Hard to guess where the difference is coming from but I feel this should probably be dealt with as follow up. After all the entire essence of this new API is for the sake of performance.

[alexcrichton]

No I simply assumed that performance wouldn't change, let me investigate why it changed locally and see if I can't figure it out. I'm with you though in that this is supposed to be performant, I am not trying to compromise on the efficiency here.

[Robbepop]

No I simply assumed that performance wouldn't change, let me investigate why it changed locally and see if I can't figure it out. I'm with you though in that this is supposed to be performant, I am not trying to compromise on the efficiency here.

Cool! For best results try benchmarking using the following profile since it makes a lot of difference using this new API:

[profile.bench]
lto = "fat"
codegen-units = 1

[alexcrichton]

I checked out the commit prior to mine and I added a benchmark that additionally validates the entirety of spidermonkey.wasm. Before after my PR to this PR I get:

bench	before	after
validate	11.9ms	12.0ms
spidermonkey	81.8ms	79.2ms

Taking a look at the profile I wrote this trivial patch:

diff --git a/crates/wasmparser/src/binary_reader.rs b/crates/wasmparser/src/binary_reader.rs
index f54a1414..983c0057 100644
--- a/crates/wasmparser/src/binary_reader.rs
+++ b/crates/wasmparser/src/binary_reader.rs
@@ -994,12 +994,19 @@ impl<'a> BinaryReader<'a> {
     ///
     /// If `BinaryReader` has no bytes remaining.
     pub fn read_u8(&mut self) -> Result<u8> {
-        self.ensure_has_byte()?;
-        let b = self.buffer[self.position];
+        let b = match self.buffer.get(self.position) {
+            Some(b) => *b,
+            None => return self.eof_err(),
+        };
         self.position += 1;
         Ok(b)
     }

+    #[cold]
+    fn eof_err<T>(&self) -> Result<T> {
+        Err(BinaryReaderError::eof(self.original_position(), 1))
+    }
+
     /// Advances the `BinaryReader` up to four bytes to parse a variable
     /// length integer as a `u32`.
     ///

and got this speedup:

bench	before	after	with patch
validate	11.9ms	12.0ms	11.4ms
spidermonkey	81.8ms	79.2ms	69.0ms

Basically I can't really reproduce the numbers you're seeing. For performance work I think it would be best to settle on a set of benchmarks to validate (maybe spidermonkey.wasm is all that's needed to be added?) and go from there. Additionally I do not think that benchmarking should be done with LTO enabled. While I understand that it improves performance (and I measured it to improve perf locally) it doesn't reflect what consumers of the crate always use so it can paper over "easily missing" #[inline] annotations for example.

Otherwise I suspect there's quite a lot of low hanging fruit on wasmparser to improve performance, we've never really focused all that much on perf yet. Most of the perf of Wasmtime has been runtime execution since Cranelift takes awhile to compile things. We of course appreciate perf work on wasmparser though!

Are you able to quantify where the slowdown you're seeing is coming from? Did I miss something perhaps? I'm also on an aarch64 machine instead of an x86_64 machine which could make things different.

[Robbepop]

@alexcrichton I conducted benchmarks using the wasmparser benchmark suite, the spidermonkey.wasm and the 780kB .wasm from wasmi with both default and optimized profile.bench. The following tables summarize the results.

main Branch

Benchmark	Default Profile	Opt. Profile	Diff%
parse	47.228 ms	38.095 ms	-19.602%
validate	88.523 ms	87.496 ms	-1.1601%

rf-visit-operator Branch

Benchmark	Default Profile	Opt. Profile	Diff%	Diff% to main (Default)	Diff% to main (Opt.)
parse	51.140 ms	40.412 ms	-20.977%	+8.2832%	+6.0806%
validate	79.736 ms	70.894 ms	-11.089%	-9.9262%	-18.974%

rf-visit-operator Branch (Cleanup Reverted)

Benchmark	Default Profile	Opt. Profile	Diff%	Diff% to main (Default)	Diff% to main (Opt.)
parse	51.335 ms	40.490 ms	-21.125%	+9.3440%	+7.0290%
validate	78.600 ms	68.066 ms	-13.402%	-10.849%	-21.926%

Where Opt. Profile is

[profile.bench]
lto = "fat"
codegen-units = 1

Conclusions

1. The parsing benchmark using the default profile.bench regressed compared to main branch by roughly 8-9%. This is not okay and needs investigation.
2. The main branch does not really profit from the optimized profile.bench with respect to its validate benchmark.
3. There is a small but significant performance difference between the rf-visit-operator branch and its version with the OperatorValidator cleanup reverted for both profiles.
4. The VisitOperator API allows rustc to better optimize the validation procedures.
   • Default Profile: 10% improvement
   • Optimized Profile: 18-21% improvement
5. Using the optimized profile.bench yields roughly 20% performance improvement for all branches for the parse benchmarks. (Unrelated to the rf-visit-operator PR)

Remarks

While I can understand profiling for the default bench profile is important for most users I think it is at least as important to also profile for the profile with maximum amounts of optimizations turned on for the cases were that matters. In wasmi we would never compile a release without this particular profile since it provides up to 400% better performance in some cases with an average performance boost of 100%. Of course wasmi is probably an outlier as an interpreter but certainly not the only performance sensitive project out there.

Update

Using flamegraph I was able to spot a major performance difference in BinaryReader::read_memarg that could explain the difference between the parse benchmark results. I tried out multiple efforts to equalize performance without success. I suppose that the OperatorFactory is probably not the ideal construct. On the other hand we could get rid of enum Operator anyways and therefore also of OperatorFactory et. al.

[alexcrichton]

Using an x86_64 machine I can roughly reproduce these results. In taking a deeper look all I can conclude though is that there's just so much low-hanging fruit I don't think it makes sense at this time to draw broad conclusions. For example the lto build before my patch is indeed ~8% faster than the lto build after my patch, but all the changes in functions seem to just be little layout things here and there where all the hot functions have so many better ways to be optimized than the changes I made.

I'm unable to reproduce your slowdown relative to main, but that probably depends a lot on what precise benchmark you're using. I don't know what parse corresponds to and if it's using things like read_operator that seems fine to regress since that's not intended to be optimized anyway.

My read on this is that this PR should go ahead and get merged and further iteration can happen in-tree. There's lots of little things I see which can be improved in the validator such as outlining error paths to cold functions to enable more inlining and such.

How do you feel about that? Basically taking the current state of this PR as a baseline and continuing to optimize from there.

[Robbepop]

I agree, let's merge and progress in-tree! :)

I totally share your thoughts on the optimization matters and will do some researching post merge.
The parse benchmark corresponds to the benchmark that uses read_operator but does no validation.

[alexcrichton]

Ok sounds good!

I think we probably need to rewrite the parse benchmark with something that does a no-op read of all the operators since read_operator is no longer interesting to benchmark after this change.