Add performance regression tests in CI #4701
Conversation
.github/workflows/regression_ci.yml
Outdated
| git fetch origin main | ||
| git checkout main | ||
|
|
||
| # Regenrate bindings for main branch |
There was a problem hiding this comment.
| # Regenrate bindings for main branch | |
| # Regenerate bindings for main branch |
|
Since I changed the file storage scheme to |
tests/regression/src/lib.rs
Outdated
| @@ -160,18 +186,28 @@ mod tests { | |||
| test_name: test_name.to_string(), | |||
| commit_hash: git::extract_commit_hash(&raw_files[0]), | |||
| }; | |||
|
|
|||
| println!("{}", profile1.test_name); | |||
There was a problem hiding this comment.
Nit: I think you forgot to remove this?
| (profile1, profile2) | ||
| } else if git::is_older_commit(&profile2.commit_hash, &profile1.commit_hash) { | ||
| (profile2, profile1) | ||
| if git::is_mainline(&profile1.commit_hash) ^ git::is_mainline(&profile2.commit_hash) { |
There was a problem hiding this comment.
I'm not really following the logic to return the correct tuple here. What is the result you're trying to return in this function? Why do you need an xor?
There was a problem hiding this comment.
What is the result you're trying to return in this function?
You can think of this as a sort on the commits:
- Whichever commit is mainline appears first
- If is_mainline() is equivalent for both commits, the older commit appears first
This is necessary for the diff functionality to know which version is the standard and which version we want to test for comparison. Between mainline and PR branch, mainline should be the standard for comparison, otherwise the older commit between two PR's which are on the same branch should be the standard for comparison to the new commit.
Why do you need an xor?
The xor returns true if exactly one of the commits is mainline, then we can return whichever commit is mainline first. If both or neither are mainline the condition evaluates to false, then we must check for which commit is older, so we move to the else condition in that case which checks for which commit is older.
I will add comments to this function to make this more clear in the code.
There was a problem hiding this comment.
It just seems like we don't need to be guessing at which commit is our "baseline". Doesn't the caller of these tests always know which commit is the baseline versus which one is the altered code? Like, you should always know I want to know the performance change that occurs from "this commit" to "this commit".
There was a problem hiding this comment.
Initially, the approach was to have the caller identify "baseline" and "altered" as an environment variable upon invocation, similar to your suggestion. However after some discussion, we decided to solely rely on the commit id to auto detect new/old and in CI, branch/mainline. This means that with solely the commit id's available to identify files we have to figure out which commit ID is "baseline" vs "altered", which this logic attempts to solve. We could add back an environment variable for the caller to set "baseline" and "altered" to make the tool more flexible in its usage but if we base profiles on solely their commit id, the current approach is needed.
There was a problem hiding this comment.
Hmmm that's a bummer, I kind of disagree with that approach because this logic looks pretty brittle as it is now. I think it would help for me to know what exactly is our goal here, do we expect this to automatically detect the right commit both when running this locally and running this in CI? Like, is the logic here only for running in CI or is it also trying to work for running the tests locally?
There was a problem hiding this comment.
The logic here attempts to cover both, running locally and in CI. Locally, we would want the older of two commits as "baseline" and in CI, we would want main to be "baseline". I agree with you though that the logic is brittle, especially for local testing since I can imagine scenarios where the caller wants to compare two commits that are not on the same branch or set the newer commit as "baseline". To make the usage a bit more extensible, I think we should add an identifier to let the caller decide, so that the artifacts produced are baseline/commit_hash.test_name.raw and altered/commit_hash.test_name.raw. I would vote to do that in a separate PR though.
There was a problem hiding this comment.
Yeah then I think you should just make this logic to work in CI. So it would just be like:
if (profile1.is_mainline()) {
(profile1, profile2)
} else {
(profile2, profile1)
}
I don't think there's much point in commiting complex logic if you're going to rip it out in a different PR.
There was a problem hiding this comment.
Given the direction that integv2 went, I think it's important to keep things at least theoretically runnable locally. Also makes putting demos together much easier, etc.
There was a problem hiding this comment.
I mean, if I am running this locally, I would probably create a branch with my changes and want to compare that branch to mainline. So even then, I really would only care about the difference between mainline and my branch.
That being said, the consequences of getting the order wrong are very minor. But I do think this code should be cleaned up in the future.
I made a change to disable running this check when changes are made to the regression crate. However, the |
Description of changes:
This PR integrates the regression tests from #4667 into the s2n-tls CI pipeline. The regression_ci.yml workflow would now run on any pull request to main, comparing the performance of all harnesses in tests/regression against the mainline performance. The test will fail if the performance regression exceeds the predefined threshold (currently set to a constant, which will be updated after merging #4698). The GitHub Actions job (example flow) uploads the performance results for the PR, the mainline, and their differences as artifacts, accessible to the developer regardless of the test outcome. This aids in debugging performance issues that may cause test failures.
Call-outs:
This workflow was tested on my personal branch by creating pull requests to my personal branch and verifying the expected outcomes for pass/failure in the performance CI. It should also run on this PR, allowing reviewers to observe the logs in the checks below as 'Performance Regression Test / regression-test (pull_request)'
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.