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

Add Benchmarks for metrics #106

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

freef4ll
Copy link

Motivation

Adding benchmark tests in order to aid development and avoid performance regressions.

Benchmark is added in the same fashion as for swift-nio and swift-certifcates.

Modifications

Benchmarks are added for incrementing a counter, gauge, histogram and an export of 5000 metrics from the esystem.

How to execute benchmark

cd Benchmarks
swift package benchmark

In Xcode can open the project like and use Instruments to profile:

open --env BENCHMARK_DISABLE_JEMALLOC=true Package.swift

Result

Benchmarks are available, and performance difference between v1 is great which only managed to increment 2-3k times per second.

Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Thanks for opening this up. Super useful!

There are a few things different to the certificate and nio setup that we should change:

  1. Update the docker files to also run the benchmarks
  2. For now only collect total allocation metrics
  3. Provide a similar script to update all benchmark thresholds locally
  4. Split the benchmark code into the benchmark definition and separate files for the benchmarks under test. NIO and certificates do the same.
  5. Add documentation to the README how to run those.

Benchmarks/Package.resolved Outdated Show resolved Hide resolved
@freef4ll
Copy link
Author

Docker scripts have been updated, but having issues with thresholds:

+ export PATH=/root/.tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ PATH=/root/.tools:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+ cd Benchmarks
+ swift package --disable-sandbox --scratch-path .build/5.8/ --allow-writing-to-package-directory benchmark --format metricP90AbsoluteThresholds --path Thresholds/5.8/
error: 'benchmarks': unknown package 'swift-prometheus' in dependencies of target 'PrometheusBenchmarks'; valid packages are: 'code', 'package-benchmark'

Split the benchmark code into the benchmark definition and separate files for the benchmarks under test

I have done this, and there are 2 implementations for Counter to see on which is the preferred one (one captures the allocation but is slower due to return of the closure).

@freef4ll freef4ll requested a review from FranzBusch October 13, 2023 15:23
@freef4ll
Copy link
Author

I have fixed the SPM error and thresholds are being generated fine now; just need to decide of which code style implementations to keep.

@freef4ll
Copy link
Author

@FranzBusch , could you have a look at the PR code style that is preferred? Counter #2 doesn't capture the allocation of the counter it self and is slower.

@freef4ll freef4ll requested a review from FranzBusch October 25, 2023 13:00
@FranzBusch
Copy link
Contributor

@swift-server-bot add to allowlist

@FranzBusch
Copy link
Contributor

@swift-server-bot test this please

@freef4ll
Copy link
Author

freef4ll commented Oct 26, 2023

I'm not sure I understand on what happened with https://ci.swiftserver.group/job/swift-prometheus-soundness-prb/31/ and what other PR state https://ci.swiftserver.group/job/swift-prometheus-soundness-prb/buildTimeTrend ; especially runs 27 and 25 which report the same result.

EDIT: fixed

Copy link
Contributor

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

LGTM from my side! @fabianfett you mind giving this a look?

@hassila
Copy link

hassila commented Feb 23, 2024

Anything we can do to help move this forward?

@ktoso
Copy link
Collaborator

ktoso commented Feb 26, 2024

From my perspective this looks fine, any concerns @FranzBusch @fabianfett ?

@ktoso
Copy link
Collaborator

ktoso commented Feb 28, 2024

@swift-server-bot test this please

@ktoso
Copy link
Collaborator

ktoso commented Feb 28, 2024

Failures because of missing baseline: Could not find any matching absolute thresholds at path [Thresholds/swift-5.7.3-RELEASE/], failing threshold check.

@fabianfett I think had some opinions before we merged here?

@ktoso ktoso requested a review from fabianfett February 28, 2024 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants