adopt package-benchmark
#41
Conversation
| .mallocCountTotal, | ||
| .mallocCountLarge, | ||
| .mallocCountTotal, | ||
| .memoryLeaked, | ||
| .allocatedResidentMemory, |
There was a problem hiding this comment.
I think for now only mallocCountTotal is fine
| ] | ||
|
|
||
| Benchmark("Set Request", configuration: .init(metrics: defaultMetrics)) { benchmark in | ||
| try await withThrowingTaskGroup(of: Void.self) { group in |
There was a problem hiding this comment.
Can we create a separate file where we have those benchmarks as methods. Similar to what we do in certificates and nio?
| Benchmark("Set Request", configuration: .init(metrics: defaultMetrics)) { benchmark in | ||
| try await withThrowingTaskGroup(of: Void.self) { group in | ||
|
|
||
| let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: 1) |
There was a problem hiding this comment.
Let's pass the EL to those new functions and let's statically initialize an EL outside of the let benchmarks. Take a look at what we do in NIO for it.
|
|
||
| for _ in benchmark.scaledIterations { | ||
| let getValue: String? = try await memcacheConnection.get("foo") | ||
| assert(getValue == setValue, "Value retrieved from Memcache does not match the set value") |
There was a problem hiding this comment.
We don't have to assert here.
| } | ||
| } | ||
|
|
||
| Benchmark("Delete Request", configuration: .init(metrics: defaultMetrics)) { benchmark in |
There was a problem hiding this comment.
Same comments for the other benchmarks
Benchmarks/Package.swift
Outdated
| .target( | ||
| name: "Benchmarks"), | ||
| .testTarget( | ||
| name: "BenchmarksTests", | ||
| dependencies: ["Benchmarks"] | ||
| ), |
There was a problem hiding this comment.
We don't need those two targets
Benchmarks/Package.swift
Outdated
| ) | ||
| // Benchmark of MemcacheBenchmarks | ||
| package.targets += [ | ||
| .executableTarget( |
There was a problem hiding this comment.
Can you just put this above instead of doing the += dance
| scalingFactor: .mega | ||
| ) | ||
| ) { benchmark in | ||
| try await runSetRequest(iterations: benchmark.scaledIterations.lowerBound, eventLoop: eventLoop) |
There was a problem hiding this comment.
We should use the upperBound for all of those instead of the lowerBound
docker/docker-compose.yaml
Outdated
| @@ -34,7 +34,7 @@ services: | |||
| depends_on: | |||
| - runtime-setup | |||
| - memcached | |||
| command: /bin/bash -xcl "swift $${SWIFT_TEST_VERB-test} $${WARN_AS_ERROR_ARG-} $${SANITIZER_ARG-} $${IMPORT_CHECK_ARG-}" | |||
| command: /bin/bash -xcl "swift $${SWIFT_TEST_VERB-test} $${WARN_AS_ERROR_ARG-} $${SANITIZER_ARG-} $${IMPORT_CHECK_ARG-} && cd Benchmarks && swift package benchmark baseline check --check-absolute-path Thresholds/$${SWIFT_VERSION-}/" | |||
There was a problem hiding this comment.
We have to disable the sandbox here as well
Motivation:
As the Swift community is moving towards adopting the Benchmark package for performance evaluations, it is imperative for our
Memcacheclient to remain aligned with best practices. The new Benchmark package not only standardize the performance measurement process but also offers detailed insights that were previously harder to obtain. This PR closes #35.Modifications:
benchmark/Thresholds/dev/update-benchmark-thresholdsscript. This script, using Docker, conducts benchmarks for all supported Swift versions and then refreshes the thresholds.Results:
The integration of the Benchmark package ensures a more thorough performance evaluation for our Memcache client. Emphasizing critical metrics and paving the way for a more accessible, iterative and detailed benchmarking process.