Adopt package-benchmark
#125
Conversation
Yeah, time based metrics mostly only works if you have dedicated CI runners for them (that's how we do it), the results are way to flakey in general at least on GitHubs runners. But you might still want to run the metrics on the CI and just set thresholds that allow for any time deviation to be ok, then you have them readily accessible if/when running and checking things manually. |
benchmarks/Thresholds/5.7/CertificatesBenchmarks.Parse_WebPKI_Roots.p90.json
Outdated
Show resolved
Hide resolved
# Conflicts: # docker/docker-compose.2204.59.yaml
|
Just one heads-up for the arc numbers - in case you do other tests that use task groups: swiftlang/swift#64636 (still relevant to check for most tests, just FYI) |
@ktoso the issue might be more relevant now when we adopt the benchmark plugin |
|
I'm now quite happy with this PR. I have disabled retain/release deltas as they are currently almost never zero. Likely immortals objects are one reason but this needs more investigation. async code still seems to be quite flaky in the number of retain/release. We aren't actually doing an async work in the test. I have disabled it for now. Also needs more investigation. Maybe async code will never have a stable retain/release count and this is just expected? cc @ktoso @Lukasa are you also fine with the state of this PR? |
There was a problem hiding this comment.
@dnadoba I have one more request. Can we add something to our readme here that links to the benchmark docs for how to get started with the benchmarking? Just a two liner or something like this so people find how to run and debug benchmarks.
|
@FranzBusch added a section to the README |
README.md
Outdated
|
|
||
| ## Benchmarks | ||
|
|
||
| Benchmarks for `swift-certificates` are in a separate Swift Package in the `benchmarks` subfolder of this repository. |
There was a problem hiding this comment.
Oh I have one more nit: Can we name the folder Benchmarks to be in-line with Sources and Tests?
There was a problem hiding this comment.
its on purpose not uppercase as it is a separate package and folders of packages are lowercase.
README.md
Outdated
| Benchmarks depends on the [`jemalloc`](https://jemalloc.net) memory allocation library, which is used by `package-benchmark` to capture memory allocation statistics. | ||
| An installation guide can be found in the [Getting Started article](https://swiftpackageindex.com/ordo-one/package-benchmark/1.11.1/documentation/benchmark/gettingstarted#Installing-Prerequisites-and-Platform-Support) of `package-benchmark`. |
There was a problem hiding this comment.
This part is very prone to get outdated. Can we just link to the index page of the documentation here.
There was a problem hiding this comment.
I have removed the version number and it will now always pointer to the latest version. I really want to keep the direct link to the Getting Started article. I don't think it is very likely they will rename the Getting Started article. The section name "Installing-Prerequisites-and-Platform-Support" may change but that's fine, the link will still work and point to the top of the getting started page.
README.md
Outdated
| Profiling benchmarks in Xcode (or building the benchmarks in release mode) with `jemalloc` is currently not supported and requires disabling `jemalloc`. | ||
| Make sure Xcode is closed and then open it from the CLI with the `BENCHMARK_DISABLE_JEMALLOC=true` environment variable set e.g.: | ||
| ``` | ||
| BENCHMARK_DISABLE_JEMALLOC=true xed . | ||
| ``` |
There was a problem hiding this comment.
Do they have this documented as well? I would prefer if we just keep as much out of our docs as possible.
There was a problem hiding this comment.
No they haven't. I'm fine to remove it if package-benchmark adds it but we can also remove it again in a subsequence PR to unblock more benchmarks that I have in the pipeline. cc @hassila
There was a problem hiding this comment.
We can definitely add the comment about how to run in Xcode for profiling etc, np.
docker/docker-compose.2204.57.yaml
Outdated
| @@ -12,7 +12,8 @@ services: | |||
| test: | |||
| image: swift-certificates:22.04-5.7 | |||
| environment: | |||
| - MAX_ALLOCS_ALLOWED_parse_webpki_roots=200050 | |||
| - SWIFT_VERSION=5.7 | |||
| - MAX_ALLOCS_ALLOWED_parse_webpki_roots=422050 | |||
| - MAX_ALLOCS_ALLOWED_tiny_array_cow_append_contents_of=9050 | |||
There was a problem hiding this comment.
Any reason not to drop the MAX_ALLOCS_ALLOWED variables?
There was a problem hiding this comment.
No, just an oversight. removed them now.
Motivation
Our current allocation tests are using NIOs allocation counter framework which we clone from
swift-niomain. This has served us well for last couple years and there was now better solution. Fast forward to day, we have a new package benchmark plugin from the community that we can use to improve our workflow of writing and running all sorts of benchmarks. It uses standard Packages and can be driven directly from the SwiftPM CLI.Modifications
package-benchmarkhas higher minimum platform version requirements.package-benchmarkto drive the execution of the benchmarksbenchmarks/Thresholds/dev/update-benchmark-thresholdsthat executes the benchmarks locally using docker for all supported swift versions and updates the thresholdsResults
Allocations tests are more accessible, easier to iterate and include more metrics.