Fix running benchmarks (Issue 1386) #1393
Conversation
|
@swift-ci please test |
| @@ -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")] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Seems fine to me, since this patch is trying to keep that from happening again.
|
@itingliu What happens if we try: $ BENCHMARK_DISABLE_JEMALLOC=true swift package --disable-sandbox -c release benchmark run --filter "URL-Template-expansion"I think this diff would still have the same availability error from |
Without |
| let benchmarks = { | ||
| calendarBenchmarks() | ||
| localeBenchmarks() | ||
| } |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
BenchmarkCalendarandBenchmarkLocaleto separate and independent benchmark targets. - Keep
BenchmarkCalendarandBenchmarkLocaleas 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
BenchmarkCalendarandBenchmarkLocaleas 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh… I think you are correct! It looks like the default Benchmark constructor does use shared global state:
But the configuration itself is a value type:
Which is then copied by value in the new benchmark:
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.
|
|
Fix the issues I encountered when trying to run benchmarks from the package.
With these changes I can now run benchmarks inside
Benchmarksfolder with the following commandUSE_PACKAGE=1 BENCHMARK_DISABLE_JEMALLOC=true swift package --disable-sandbox -c release benchmark run --filter "URL-Template-expansion"Specifically
MemberImportVisibilityso system Foundation doesn't get automatically pulled in when any of the dependencies import Foundation in their source codeInternationalizationBenchmark.swiftfor all benchmarks files inside/Internationalization/folder. Previously I was seeinginvalid redeclaration of 'benchmarks'becauseBenchmarkCalendar.swiftandBenchmarkLocale.swiftare placed inside the same folder, and both of them declarelet benchmarks, and that isn't supported by Benchmark package.NSTemporaryDirectorywith the one from swift-foundation package.