Support a `Range` as threshold-tolerance

[MahdiBM]

I keep finding myself writing benchmarks like this:

    Benchmark(
        "...",
        configuration: .init(
            ...
            thresholds: [
                .throughput: .init(relative: [.p90: 4])
            ]
        )
    ) { benchmark in
            ...
    }

While having static threshold files containing something like:

{
  "throughput": 772500
}

These are all running on dedicated-vcpu machines, but still I need to declare thresholds to tolerate noise.

The bad part is how I need to calculate the static threshold number and the percentage:
I need to run the benchmark a few times (eh, fine) then read all the results and figure out what was the minimum and what was the maximum that ever happened.
For example in this case those were 757K and 786K.
Then I have to round those numbers to something that I think will be appropriate; In this case: 750K and 795K.
Now I have to take average of them: 772500.
Then I have to see how many %s is 750K far from the 772500: 3%.
So then I can decide I can use 4 as the percent in thresholds just in case because I've seen this package not like it when the diff is 3% and you've also set 3 in the thresholds, and based on my experience it doesn't do fractional percentages which mean it rounds the percentages to something that might cross the thresholds line.

What I propose is for this package to simply support a range of numbers as threshold. While that doesn't solve every single problem I mentioned above, it does simplify the process by a lot.
The API could look like:

            thresholds: [
                .throughput: .init(range: [.p90: 750_000..<795_000])
            ]

[MahdiBM]

Another way the API can go which i think would be the better way, is to declare the threshold tolerance in the static file instead.
So something like:

{
  "throughput": {
    "min": 750000,
    "max": 795000
  }
}

This way we might be able to slowly deprecate having "threshold-tolerances" in the code, which IMO doesn't make a great deal of sense.

[MahdiBM]

@hassila I could try to implement this but I need to first know your opinion 🙂

[hassila]

@thanks @MahdiBM, let me think a bit about it - low bandwidth at the moment unfortunately at the moment with some internal deadlines and summer, may be a little while before replying. I believe this may already be possible with the static thresholds which do in fact store the thresholds in a file - have you played anything with those?

[MahdiBM]

@hassila right, I'm talking about the same static threshold files. I'm using those. I'm just proposing a way to improve them.

What I'm thinking in the long run:

• The thresholds which is really thresholdTolerances should be removed from Configuration.
  • Not the optimal place for it.
  • I think it also makes the compare command have to build again just to look at the threshold-tolerances.
• Then we'll have the same static threshold files.
  • The same as today, you can assign numbers, like "throughput": 16000000.
  • New: you can assign ranges too, like: "throughput": {"min": 750000, "max": 795000}.
  • New: you can assign relative too as relative percentage, like: "throughput": {"base": 770000, "relative": 5.5}.
  • Ranges and Relative should be able to work together. So users can have both a base/relative and min/max in the object.
• Then we can deprecate thresholds from configuration to remove in the next major version.
• We can add a temporary flag to the compare command something like --skip-building-just-to-load-thresholds-from-code.
• We can add new flags like thresholds update --run-count 10 --relative --range so it runs the benchmark 10 times in the CI, and calculates the biggest range for those and updates the threshold files.
  • Currently this is the most painful part of using this package for me: putting together static thresholds with proper tolerances.
• <one more point I forgot about ... let's see when I'll remember> I think not having p90 in the file name, and having it in the JSON as a field, is probably better as well. In the future this package might support p99, p50 etc... and we'll start to have problems with the file count.

I probably should split these up into multiple issues, but want to know your initial opinion first.

Feel free to reply whenever you can, I might be able take care of the implementation if we can agree on something for now. We can adjust if/when I put up the PRs and something isn't working as well as we thought.

[MahdiBM]

I could also put a mini proposal on the forums and see what others think about these. That's probably the best way since this package is very widely used.

[MahdiBM]

@hassila Sorry for the ping again, but I've put together 4 new PRs: #328 #329 #330 #331

No pressure though, you can review them on your own schedule. I simply had a personal need for these features for https://github.com/MahdiBM/swift-dns, and that's why I've gone ahead with implementing them.

[MahdiBM]

Okay, Now that I've actually had the time to test my own PRs in practice, here's a report:

• I do really like the range/relative thresholds in static files.
  • More user-friendly to me. I think a min-max range is more practical than a base number +- some-number/some-percent.
  • Cuts a minimum of 15s of CI time since no benchmark-build is necessary for a thresholds check anymore.
• The thresholds update mechanism for range/relative is also an improvement.
  • Don't need to manually put together threshold-tolerances for benchmarks that are a bit flaky even on dedicated-cpu machines.

What didn't quite go as well as I liked it to, is that you need to run the thresholds update command for a long period of time for it to be effective.

I tried a 12 hour (ended up being 13.5 hours) run, with 1200 times running the benchmarks and updating the range/relative thresholds, and only then the result is good. The benchmarks would still fail with ~1 deviation every like 7 runs. I haven't tried like 6 hour periods, that could also be enough. Or even 3. Those also might suffice.

What I tried is like 30 mins / 1 hour runs, and the result there was kind of weird and surprising.
The thresholds update command would run the benchmark 100 times in an hour and widen the threshold-tolerances each time, but then when you ran the benchmark to see how the new thresholds would perform, a lot of the thresholds would prove not good enough and the benchmark run would rather-consistently fail.

I'm really curious to know if there is any code that makes this possible for the thresholds update command to be less flaky, but then when you'd run the actual benchmark, the results would be too far off that would exceed the tolerances.

There is a fine chance it's not even about "when running 100 benchmarks one after the other, the Hetzner machine is hot and ready so it runs the benchmarks more consistently".
Because when you run a benchmark check it'd consistently be out of the thresholds, then that would mean the first run of thresholds update command should already catch how a normal benchmark check would behave, since the machine is not warmed up. Not sure still.
How I run the benchmark check is in this CI file. You can see the actual benchmark run is done through the baseline update command. The update-thresholds CI file is also here.

Sorry for the long message but if I find out the reason behind the difference between the behaviors of baseline update and thresholds update command, then maybe I could make the benchmarks even more stable.

I still very much prefer to run the CI for 12 hours than to have to waste 6 hours of my time trying to set up thresholds for 1 new benchmark.
Sure 12 hours is a lot, but I could trigger the CI before sleep, and practically it ends up saving me a lot of bandwidth that I didn't have to spend setting up thresholds (and again maybe even 6 or 3 is enough, haven't tried).

[MahdiBM]

So another report of how things are right now, after a while.
I've been trying to make my benchmarking setup as seamless as possible, and I think I've made a ton of progress towards that.

How my benchmarking setup works right now:

• Use static threshold files with range thresholds.
• When needed, a benchmark is run on e.g. a PR.
  • If the benchmark fails, the CI automatically commits an update of the static range thresholds with the deviated baseline to a new branch.
  • This is so if the deviations are acceptable, I can simply merge that new branch into the actual branch in a matter of seconds.
• The same goes for the "Run 100+ times and update the thresholds" mechanism.

This setup is so nice that I've even been thinking about writing an article about it. One issue would be that I'd like the changes to have been reviewed/merged if I'm writing an article about them.

For the record I also found quite a few bugs with my implementation, but I've already ironed them all out in the 4th PR.

This setup truly makes writing benchmarks a lot less of a hassle, and is much more encouraging to write benchmarks.
Now I don't need to worry and be like "ah I should add a new benchmark for this but then I'll need to allocate 6 hours wrestling with benchmarks today, adjusting thresholds" which is HUGE deal to me. Even if I set up a suboptimal range-threshold, the next time that the benchmark fails I can trivially update the range thresholds by simply merging that branch that the CI automatically creates when the new baseline deviates. I truly CANNOT stress how much less friction the current setup has compared to before.

And these all are accompanied with the other refinements I've done for the benchmarking setup, most of which when i was trying to set up Vapor's CI. Like outputting a nice little beautiful comment to the PR when things fail.