Skip to content

Fix running benchmarks (Issue 1386) #1393

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 2, 2025
Merged

Fix running benchmarks (Issue 1386) #1393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
954ca9a
Attempted fix
itingliu Jun 30, 2025
6b166c1
Fix URL temporary directory
itingliu Jun 30, 2025
92d2258
Fix invalid redeclaration of 'benchmarks'
itingliu Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Benchmarks/Benchmarks/Formatting/BenchmarkFormatting.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import Dispatch

#if os(macOS) && USE_PACKAGE
import FoundationEssentials
import FoundationInternationalization
#else
import Foundation
#endif
Expand Down
4 changes: 3 additions & 1 deletion Benchmarks/Benchmarks/Internationalization/BenchmarkCalendar.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import FoundationInternationalization
import Foundation
#endif

let benchmarks = {
func calendarBenchmarks() {

Benchmark.defaultConfiguration.maxIterations = 1_000
Benchmark.defaultConfiguration.maxDuration = .seconds(3)
Benchmark.defaultConfiguration.scalingFactor = .kilo
Expand Down Expand Up @@ -229,3 +230,4 @@ let benchmarks = {
}
}
}

2 changes: 1 addition & 1 deletion Benchmarks/Benchmarks/Internationalization/BenchmarkLocale.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import FoundationInternationalization
import Foundation
#endif

let benchmarks = {
func localeBenchmarks() {
Benchmark.defaultConfiguration.maxIterations = 1_000
Benchmark.defaultConfiguration.maxDuration = .seconds(3)
Benchmark.defaultConfiguration.scalingFactor = .kilo
Expand Down
6 changes: 6 additions & 0 deletions Benchmarks/Benchmarks/Internationalization/InternationalizationBenchmark.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import Benchmark

let benchmarks = {
calendarBenchmarks()
localeBenchmarks()
}
Comment on lines +3 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

@itingliu FWIW… it's possible this might lead to some unexpected behaviors. Our calendarBenchmarks function sets some global state:

Benchmark.defaultConfiguration.metrics = [.cpuTotal, .mallocCountTotal, .throughput]

And our localeBenchmarks also sets some global state:

Benchmark.defaultConfiguration.metrics = [.cpuTotal, .wallClock, .throughput, .peakMemoryResident, .peakMemoryResidentDelta]

At this point… I'm not sure we know exactly which function "wins".

I believe our expectation is that the benchmarks defined in localeBenchmarks run with peakMemoryResident and peakMemoryResidentDelta as default metrics. Is that correct? And our expectation is that the benchmarks defined in calendarBenchmarks do not run with peakMemoryResident and peakMemoryResidentDelta as default metrics. Is that correct?

Copy link
Contributor Author

@itingliu itingliu Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

They're executed sequentially, so I think mutating global state is fine. The configurations will just be updated right before the tests run, or am I missing anything?

Copy link
Contributor

@vanvoorden vanvoorden Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The configurations will just be updated right before the tests run, or am I missing anything?

This is where my question was going… to what extent is registering a set of benchmarks decoupled from running a subset of those benchmarks?

Suppose we run our international benchmarks package target… but we focus specifically on a benchmark defined from calendarBenchmarks:

$ swift package --disable-sandbox -c release benchmark run --target "InternationalizationBenchmarks" --filter "nextThousandThursdaysInTheFourthWeekOfNovember"

When we launch our benchmarks… I assume we have to execute both calendarBenchmarks and localeBenchmarks. The next step then is to respect the filter that was specified and only run nextThousandThursdaysInTheFourthWeekOfNovember. At that point… I believe we can see how our benchmark tests are potentially running out of sync from the global state we set when those tests were defined.

In general I believe the pattern I typically see is splitting benchmark targets apart and this question about the global Benchmark.defaultConfiguration state is not very important. It is global state… but we tear it down and rebuild it for every target so it is fresh and clean.

I do not believe this is a major issue… and this is currently a very impactful diff that unblocks engineers from running benchmarks and that is good! But I do believe there might be some unexpected issues currently from running the benchmarks and what specific metrics might be reported. I could maybe think of three possible ideas to work around that:

  • Move BenchmarkCalendar and BenchmarkLocale to separate and independent benchmark targets.
  • Keep BenchmarkCalendar and BenchmarkLocale as one benchmark target but update the way that configurations are passed to benchmarks so we do not depend on global state. Similar to what we do in other places.1
  • Keep BenchmarkCalendar and BenchmarkLocale as one benchmark target and update some header documentation comments where the benchmarks are defined to notify engineers that we might see unexpected behavior when configurations might not be respected.

But I don't think this discussion would have to block this specific diff from landing. I think you can use your best judgement and make the best decision if you want to ship more code on this right now or ship a future diff with a potential workaround in the future.

Footnotes

  1. https://github.com/swiftlang/swift-foundation/blob/swift-6.1.2-RELEASE/Benchmarks/Benchmarks/Internationalization/BenchmarkCalendar.swift#L135-L148 ↩

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 assume we have to execute both calendarBenchmarks and localeBenchmarks. The next step then is to respect the filter that was specified and only run nextThousandThursdaysInTheFourthWeekOfNovember. At that point… I believe we can see how our benchmark tests are running out of sync from the global state we set when those tests were defined.

Since nextThousandThursdaysInTheFourthWeekOfNovember is defined inside localeBenchmarks, localeBenchmarks will need to be run first, so I would expect the configuration set in that benchmark to be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh… I think you are correct! It looks like the default Benchmark constructor does use shared global state:

https://github.com/ordo-one/package-benchmark/blob/1.29.3/Sources/Benchmark/Benchmark.swift#L229

But the configuration itself is a value type:

https://github.com/ordo-one/package-benchmark/blob/1.29.3/Sources/Benchmark/Benchmark.swift#L432

Which is then copied by value in the new benchmark:

https://github.com/ordo-one/package-benchmark/blob/1.29.3/Sources/Benchmark/Benchmark.swift#L238

So it looks like defining benchmarks does capture shared mutable state… but it captures that shared mutable state by value and mutating that shared state in the future does not affect the benchmark after it was defined. My mistake! Sorry for any confusion about that.

6 changes: 3 additions & 3 deletions Benchmarks/Benchmarks/String/BenchmarkString.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import FoundationEssentials
import Foundation
#endif

#if !os(macOS)
#if !FOUNDATION_FRAMEWORK
private func autoreleasepool<T>(_ block: () -> T) -> T { block() }
#endif

Expand Down Expand Up @@ -157,7 +157,7 @@ let benchmarks = {
let str = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."

Benchmark("read-utf8") { benchmark in
let rootURL = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true).appendingPathComponent(UUID().uuidString, isDirectory: true)
let rootURL = URL.temporaryDirectory.appendingPathComponent(UUID().uuidString, isDirectory: true)
#if compiler(>=6)
let fileURL = rootURL.appending(path: "benchmark.txt", directoryHint: .notDirectory)
#else
Expand All @@ -178,7 +178,7 @@ let benchmarks = {
}

Benchmark("read-utf16") { benchmark in
let rootURL = URL(fileURLWithPath: NSTemporaryDirectory(), isDirectory: true).appendingPathComponent(UUID().uuidString, isDirectory: true)
let rootURL = URL.temporaryDirectory.appendingPathComponent(UUID().uuidString, isDirectory: true)
#if compiler(>=6)
let fileURL = rootURL.appending(path: "benchmark.txt", directoryHint: .notDirectory)
#else
Expand Down
2 changes: 1 addition & 1 deletion Benchmarks/Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ print("swift-foundation benchmarks: \(usePackage.description)")
var packageDependency : [Package.Dependency] = [.package(url: "https://github.com/ordo-one/package-benchmark.git", from: "1.11.1")]
var targetDependency : [Target.Dependency] = [.product(name: "Benchmark", package: "package-benchmark")]
var i18nTargetDependencies : [Target.Dependency] = []
var swiftSettings : [SwiftSetting] = []
var swiftSettings : [SwiftSetting] = [.unsafeFlags(["-Rmodule-loading"]), .enableUpcomingFeature("MemberImportVisibility")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This .unsafeFlags(["-Rmodule-loading"] doesn't contribute to this fix, but I don't think it hurts to leave it here. Can definitely remove it if it's too loud.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine to me, since this patch is trying to keep that from happening again.


switch usePackage {
case .useLocalPackage(let root):
Expand Down
Loading