Test benchmarks with -Z panic-abort-tests
#111992
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit, tests were invoked in multiple places, especially due to `-Z panic-abort-tests`, and adding a new test kind meant having to chase down and update all these places. This commit creates a new Runnable enum, and its children RunnableTest and RunnableBench. The rest of the harness will now pass around the enum rather than constructing and passing around boxed functions. The enum has two children enums because invoking tests and invoking benchmarks requires different parameters.
The inner function is not needed anymore as it's only called once after the previous commit's refactoring.
Before this commit, both static and dynamic benches were converted to a DynTestFn, with a boxed closure that ran the benchmarks exactly once. While this worked, it conflicted with -Z panic-abort-tests as the flag does not support dynamic tests. With this change, a StaticBenchFn is converted to a StaticBenchAsTestFn, avoiding any dynamic test creation. DynBenchFn is also converted to DynBenchAsTestFn for completeness.
|
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This reverts commit 2050066.
|
Also reverted #103262, since the test now works on targets without unwinding. |
Only a breaking change to the unstable API. The current API doesn't look like something we'd want to stabilize in its current form anyway, so changing it is fine. |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 58d9d8c with merge cef564b64b979c1854403ff17c87475213c57f48... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
TaKO8Ki
added a commit
to TaKO8Ki/rust
that referenced
this pull request
Jun 27, 2023
…ch, r=m-ou-se Test benchmarks with `-Z panic-abort-tests` During test execution, when a `#[bench]` benchmark is encountered it's executed once to check whether it works. Unfortunately that was not compatible with `-Z panic-abort-tests`: the feature works by spawning a subprocess for each test, which prevents the use of dynamic tests as we cannot pass closures to child processes, and before this PR the conversion from benchmark to test was done by turning benchmarks into dynamic tests whose closures execute the benchmark once. The approach this PR took was to add two new kinds of `TestFn`s: `StaticBenchAsTestFn` and `DynBenchAsTestFn` (:warning: **this is a breaking change** :warning:). With that change, a `StaticBenchFn` can be converted into a `StaticBenchAsTestFn` without creating dynamic tests, and making it possible to test `#[bench]` functions with `-Z panic-abort-tests`. The subprocess test runner also had to be updated to perform the conversion from benchmark to test when appropriate. Along with the bug fix, in the first commit I refactored how tests are executed: rather than executing the test function in multiple places across `libtest`, there is now a private `TestFn::into_runnable()` method, which returns either a `RunnableTest` or `RunnableBench`, on which you can call the `run()` method. This simplified the rest of the changes in the PR. This PR is best reviewed commit-by-commit. Fixes rust-lang#73509
|
Looks spurious? @bors retry |
bors
added
S-waiting-on-bors
|
See also #113065. Some PRs have become blocked on dist-mips64el-linux failures. |
|
⌛ Testing commit 58d9d8c with merge 0d9e6916553d0a3ed8b211dc9b4711b2c56cfce5... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
This was referenced Jul 1, 2023
|
Finished benchmarking commit (e5bb341): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 662.02s -> 661.138s (-0.13%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During test execution, when a
#[bench]benchmark is encountered it's executed once to check whether it works. Unfortunately that was not compatible with-Z panic-abort-tests: the feature works by spawning a subprocess for each test, which prevents the use of dynamic tests as we cannot pass closures to child processes, and before this PR the conversion from benchmark to test was done by turning benchmarks into dynamic tests whose closures execute the benchmark once.The approach this PR took was to add two new kinds of⚠️ ). With that change, a
TestFns:StaticBenchAsTestFnandDynBenchAsTestFn(:warning: this is a breaking changeStaticBenchFncan be converted into aStaticBenchAsTestFnwithout creating dynamic tests, and making it possible to test#[bench]functions with-Z panic-abort-tests. The subprocess test runner also had to be updated to perform the conversion from benchmark to test when appropriate.Along with the bug fix, in the first commit I refactored how tests are executed: rather than executing the test function in multiple places across
libtest, there is now a privateTestFn::into_runnable()method, which returns either aRunnableTestorRunnableBench, on which you can call therun()method. This simplified the rest of the changes in the PR.This PR is best reviewed commit-by-commit.
Fixes #73509