CI Benchmark failures

[alganet]

Earlier this year I introduced benchmarks (#1602).

The current benchmark workflow is not required for a PR to pass. This was expected since GitHub runners, being shared machines, are not consistent and can vary widely across runs.

However, the failing results might be confusing or frustrating, creating unecessary friction.

We could improve this by performing interleaving, however, that would make benchmarks take longer.

Another approach would be to make historical interleaving. We are already setup for that by having the benchmarks branch, but we don't have enough results yet for it to be meaningful.

I pretend to add --tolerate-failures to phpbench, making it always be green and thus removing the current friction point up until we can do historical interleaving.

Any objections?

[henriquemoody]

Because of the benchmarks we're waiting more to get a full pipeline in PRs. This was to be expected, but since we only see this getting slower over time, especially if we want to make more them more useful, perhaps running them as a scheduled job that fails rather than a PR check that doesn't makes more sense.

If the job indicates a considerable performance degradation, we could still track down the culprit and either solve the issue or bite the bullet.

[alganet]

Because of the benchmarks we're waiting more to get a full pipeline in PRs.

That's mostly psychological though. As I mentioned, they're not blockers. The submit button enables before it completes (if there are approvals).

It takes 5min. We often don't get approvals in 5min.

What I want to change is the "red" failure that happens often and reduces confidence. It's also psychological, no one wants to merge a failing result.

a scheduled job

That's a slippery slope. What if we commit a huge regression in performance, some filesystem loop, only to discover it later?

[henriquemoody]

It takes 5min. We often don't get approvals in 5min.

5m as a long time for the size of our codebase, IMO. It's not unacceptable, but it is long.

That's a slippery slope. What if we commit a huge regression in performance, some filesystem loop, only to discover it later?

I think that's debatable, because a change that brings a huge regression might come unnoticed if we always pass the check, or if the check is always red. If we don't agree, I'd be fine to compromise and add the --tolerate-failures, but I would have preferred that we made that job longer and more deterministic, but running that as a scheduled job.

[alganet]

In order to make it more deterministic, I need more entries in the benchmarks branch in order to have data for the interleaving. It's a chicken-and-egg problem.

Maybe the idea of a scheduled job is the correct one, in order to collect that data.

We can always run benchmarks locally too. Our local machines (if nothing else is running) will be reliable across runs.

// Before changes
vendor/bin/phpbench run --filter=MyNewBench --tag=base
// After changes
vendor/bin/phpbench run --filter=MyNewBench --ref=base

We should aim to always do that, especially for hot paths.

So, here's a draft of what we can do:

• Change ci-perf.yml to "data collection mode" for now. It runs on a schedule, always passes, generates history in benchmarks.
• Change ci-perf.yml to only run checks if the user asks. issue_comment: trigger + mention @TheRespectPanda benchmark on PRs.
• Document how to run and interpret local benchmarks and profiles in CONTRIBUTING.md (similar to testing instructions). Maybe a separate dev doc.

I believe that would solve all the current friction we have with it:

• Data will be collected to make a future deterministic check.
• Benchmarks for sensible PRs can be asked by the issue_comment trigger only if a contributor wants to.
• By documenting continuous performance, we make it a responsability instead of just an automated check.

I can work on the first one (data collection mode) right now to improve our developer experience, and then move on to the other items after the 3.0 release.

Thoughts?

[henriquemoody]

I think that's a great plan!

Maybe at some point we can also create a command that helps us running the benchmarks using the data we collect in the benchmarks branch, just to make it easier