Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adopt package-benchmark #2534

Merged
merged 1 commit into from Oct 8, 2023
Merged

Adopt package-benchmark #2534

merged 1 commit into from Oct 8, 2023

Conversation

FranzBusch
Copy link
Contributor

Motivation

We want to migrate our allocation and later on also our performance tests to use the package-benchmark plugin. This plugin makes writing benchmarks way easier than our current setup. Furthermore, debugging benchmarks is also possible from within Xcode now.

Modification

This PR adds the setup for the benchmarking infrastructure and connects it with out

Result

Allocations tests are more accessible and easier to iterate.

@@ -0,0 +1,9 @@
.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we nest Benchmarks in IntegrationTests instead of the root of the repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally did not nest it since I expect us to migrate all of them at some point and having it at the top level is way nicer. This also follows what we do in other packages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's a good reason given IntegrationTests contains more than just allocation tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still would like to have them at the top level. Similar to how we should move the performance tests out of the NIOPerformanceTester and into Benchmarks once we migrate them. @Lukasa @dnadoba WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with having them at the top level.

Benchmarks/Benchmarks/NIOPosixBenchmarks/Benchmarks.swift Outdated Show resolved Hide resolved
Benchmarks/Benchmarks/NIOPosixBenchmarks/TCPEcho.swift Outdated Show resolved Hide resolved
Benchmarks/Benchmarks/NIOPosixBenchmarks/TCPEcho.swift Outdated Show resolved Hide resolved
scalingFactor: .mega
)
) { benchmark in
try runTCPEcho(numberOfWrites: 10_000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scalingfactor 1M but writing 10K? Perhaps consider numberOfWrites to be scalingfactor if you want to make sense of output from --scale later. Otherwise probably skip setting scalingfactor here, as it's really intended for inner loops in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I set the scaling factor because with the default the p90 was to flaky for allocations. With .mega scaling factor they seem really stable. I just pushed a commit that uses the scaledIterations.upperBound for the number of writes

# Motivation
We want to migrate our allocation and later on also our performance tests to use the `package-benchmark` plugin. This plugin makes writing benchmarks way easier than our current setup. Furthermore, debugging benchmarks is also possible from within Xcode now.

# Modification
This PR adds the setup for the benchmarking infrastructure and connects it with out

# Result
Allocations tests are more accessible and easier to iterate.
@FranzBusch FranzBusch enabled auto-merge (squash) October 8, 2023 20:08
@FranzBusch FranzBusch merged commit d782de8 into apple:main Oct 8, 2023
6 of 8 checks passed
@FranzBusch FranzBusch deleted the fb-benchmark branch October 8, 2023 20:31
@FranzBusch FranzBusch added the needs-no-version-bump For PRs that when merged do not need a bump in version number. label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-no-version-bump For PRs that when merged do not need a bump in version number.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants