CI benchmarks: consider using an adaptive noise threshold

[aochagavia]

The current CI benchmarks use a hardcoded threshold of 0.20%. If there is an instruction count difference greater than 0.20% for any benchmark, the CI job fails and the reviewer is expected to triage the benchmark results.

In a conversation with @nnethercote (known for his effort optimizing the Rust compiler), he strongly adviced to use an adaptive threshold instead (i.e. look at the history of measurements for the specific benchmark and automatically derive a threshold based on them). This is necessary to deal with the fact that even stable benchmarks can suddenly become noisy without a clear cause (see dealing with noise by @Kobzol, who also works benchmarking Rust). I do think it makes sense to keep 0.20% as a hardcoded lower bound.

There is some complexity associated with using an adaptive threshold, though:

1. We need to keep track of past measurements somewhere (will it require setting up additional infrastructure?)
2. We need to come up with a sound way to derive the threshold from the historic data (maybe we can just copy whatever Rust is doing)

IMO there is also a chance we might get away with the current hardcoded approach. Though we have more than 20 benchmarks, they all boil down to 3 basic scenarios (full handshake, resumed handshake, data transfer), and things seem to be working well since we hooked up the benchmarks to the CI. Maybe manually modifying the noise thresholds upon need requires less work than developing and maintaining an adaptive noise setup.

@djc @ctz @cpu since you do most of the PR reviewing and are in that sense users of the CI benchmarks, do you have an opinion on this?

@nnethercote @Kobzol feel free to comment if you have any additional insights to share

[djc]

Do you have a sense of the distribution of results for each of the current benchmarks we currently run?

[aochagavia]

A few weeks ago I did some basic tests to ensure noise is always lower than 0.20% when running the benchmarks on the same code, and when running them after small changes to the code that shouldn't have a performance impact. During that process I found and neutralized a few sources of non-determinism (either by tweaking the benchmarking IO code or by adding iterations to smooth out the noise). I didn't look at the distribution, though. I could gather measurements for the two scenarios (code without changes, code with minimal changes) and plot the distributions, if that helps making a decision here.

[djc]

I was thinking it could be interesting to mine GitHub PR Actions output to see reported variations in actual CI runs to see how variance actually differs between tests.

[ctz]

Should we export the csv results as artifacts? That would easier to do the above, and also do a long-term trend graph.

[ctz]

FWIW I've found the 0.2% threshold fairly reasonable so far -- it seems about right.

But I would say some changes have been -- very roughly -- characterisable as "this removes 1000 instructions" or "this adds 1000 instructions" to any handshake. Those types of changes are more visible in the faster resumption tests, because 1000 is a greater percentage of the smaller base instruction count. And comparing the actual instruction count deltas (say between resumed and full handshakes) is obscured by RESUMED_HANDSHAKE_RUNS (but that doesn't affect the percentage).

I wonder if we could divide down the resumption measurements by RESUMED_HANDSHAKE_RUNS to make this effect more clear?

[epompeii]

1. We need to keep track of past measurements somewhere (will it require setting up additional infrastructure?)

You could use Bencher: https://github.com/bencherdev/bencher
There's a free hosted version at https://bencher.dev so you wouldn't need to worry about infra.

2. We need to come up with a sound way to derive the threshold from the historic data (maybe we can just copy whatever Rust is doing)

Bencher handles this out of the box with statistical thresholds, similar to rust-perf.

I'm the maintainer of Bencher and user of rustls, so I would be more than happy to help get things set up!

[Kobzol]

I haven't used bencher personally, but I too wanted to suggest that using an existing database + GUI might be a good idea. I have seen (and worked on) benchmarking infrastructure being reimplemented over and over again and it's a lot of work.

If an integration with some external service (e.g. bencher) works, it would be great if we could then distill some guide for other Rust projects on how to add their own benchmarking setup.

[aochagavia]

All right! I'll be investigating bencher and come back with an update (or a PR!)

[aochagavia]

Here's an update from the front, so you can think along and bring up suggestions if you have any.

1. Since we want to track icounts over time (i.e. not just comparing two branches in isolation), we will need to make sure the benchmarks always run on a machine with the same CPU. We were planning to use a bare metal server for walltime benchmarks, which I already set up. We can reuse that for the icount benchmarks.
2. To keep overhead at a minimum (and minimize noise in the upcoming walltime benchmarks), I'm working on a thin application that will run benchmarks on demand (one at a time). The alternative would be to configure our server as a GitHub Actions runner, but I'm not sure it would give us enough control (e.g. limiting concurrent jobs to one, only using it for benchmarking / rejecting other kinds of job, etc). Does anyone here have experience with that?
3. Benchmarks should be manually triggered by maintainers and no longer run automatically (otherwise you risk running arbitrary code on the server, which is extra dangerous because the machine is not ephemeral like GH Actions runners). We can program an exception to the rule for pull requests that originate from this repo, since people with write access to the repo are trusted.
4. The UX to trigger a benchmark will likely have to change (probably for the better). I'll open a separate issue for that so we can discuss.
5. To determine the significance threshold, every push to main will trigger a bench run (the results will be stored and the noise threshold will be calculated based on them). Next to storing the results locally, we might push them to bencher.dev for visualization (and maybe also to let them calculate a suitable noise threshold for us).

[epompeii]

1. I think using the same runner for walltime and icount benchmarks is the right call for now. Though, I'm curious about the cost-to-noise-to-throughput tradeoffs between a bare metal server versus a dedicated, persistent VM for icount benchmarks. This is more of a long-term/if a single runner ever becomes a bottleneck question though.
2. Have you looked into bare metal containers? I've only done provisional research into this, but even if there is a slight bit of overhead from using a container (ie having to validate against a seccomp policy for each syscall, etc) that overhead ought to be very consistent. This would then give us a well understood artifact to orchestrate running. I'm aware that GitHub Actions does support custom runners (something like BuildJet https://buildjet.com/for-github-actions), but I don't think it would handle iter-workflow parallelism as we would like, with only a single host/server.
3. Untrusted code execution on the bare metal host can definitely be dangerous. This is another reason for my interested in the bare metal container route. I'm also curious about using NixOS to help with this. My thought was if we could limit the ability to rollback/change configurations to a user with elevated privileges and then run the benchmarks (or benchmark containers) as a non-privileged user, then after every run we just rollback the entire OS to our known good init state (ie https://nixos.org/manual/nixos/stable/#sec-rollback or something). This could also help reduce any variations between runs by getting rid of any latent state.
4. If it was possible to run untrusted code (relatively) safely, would we still need a separate UI? That is, when would this UI need to be used? Feel free to point me to that separate issue if you would prefer to discuss there.
5. Sounds great!

[aochagavia]

For future reference, since this issue was about using an adaptive noise threshold, the rest of the discussion happened / is happening in #1515

[aochagavia]

Closing this, since we have incorporated an adaptive noise threshold in our custom benchmark runner (see https://github.com/rustls/rustls-bench-app)