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

feat(minor): Avoid rebuilds #207

Merged
merged 5 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
147 changes: 81 additions & 66 deletions Plugins/BenchmarkCommandPlugin/BenchmarkCommandPlugin.swift
Original file line number Diff line number Diff line change
Expand Up @@ -127,37 +127,7 @@ import PackagePlugin
}

let swiftSourceModuleTargets: [SwiftSourceModuleTarget]

// don't build any targets if we're creating a benchmark, otherwise specified targets
if commandToPerform == .`init` {
swiftSourceModuleTargets = []
} else {
if specifiedTargets.isEmpty {
swiftSourceModuleTargets = context.package.targets(ofType: SwiftSourceModuleTarget.self)
} else {
swiftSourceModuleTargets = specifiedTargets
}
}

let filteredTargets = swiftSourceModuleTargets
.filter { $0.kind == .executable }
.filter { benchmark in
let path = benchmark.directory.removingLastComponent()
return path.lastComponent == "Benchmarks" ? true : false
}
.filter { benchmark in
swiftSourceModuleTargets.first(where: { $0.name == benchmark.name }) != nil ? true : false
}
.filter { benchmark in
skipTargets.first(where: { $0.name == benchmark.name }) == nil ? true : false
}

// Build the targets
if outputFormat == .text {
if quietRunning == 0 {
print("Building benchmark targets in release mode for benchmark run...")
}
}
var shouldBuildTargets = true // We don't rebuild the targets when we dont need to execute them, e.g. baseline read/compare

let benchmarkTool = try context.tool(named: "BenchmarkTool")

Expand All @@ -167,39 +137,6 @@ import PackagePlugin
"--format", outputFormat.rawValue,
"--grouping", grouping]

try filteredTargets.forEach { target in
if outputFormat == .text {
if quietRunning == 0 {
print("Building \(target.name)")
}
}

let buildResult = try packageManager.build(
.product(target.name), // .all(includingTests: false),
parameters: .init(configuration: .release)
)

guard buildResult.succeeded else {
print(buildResult.logText)
print("Benchmark failed to run due to build error.")
return
}

// Filter out all executable products which are Benchmarks we should run
let benchmarks = buildResult.builtArtifacts
.filter { benchmark in
filteredTargets.first(where: { $0.name == benchmark.path.lastComponent }) != nil ? true : false
}

if benchmarks.isEmpty {
throw ArgumentParsingError.noMatchingTargetsForRegex
}

benchmarks.forEach { benchmark in
args.append(contentsOf: ["--benchmark-executable-paths", benchmark.path.string])
}
}

metricsToUse.forEach { metric in
args.append(contentsOf: ["--metrics", metric.description])
}
Expand Down Expand Up @@ -286,22 +223,97 @@ import PackagePlugin
print("Must specify exactly zero or one baseline for check against absolute thresholds, got: \(positionalArguments)")
throw MyError.invalidArgument
}
if positionalArguments.count == validRange.upperBound { // dont check if we just read baselines
shouldBuildTargets = false
}
} else {
let validRange = 1 ... 2
guard validRange.contains(positionalArguments.count) else {
print("Must specify exactly one or two baselines for comparisons or threshold violation checks, got: \(positionalArguments)")
throw MyError.invalidArgument
}
if positionalArguments.count == validRange.upperBound { // dont check if we just read baselines
shouldBuildTargets = false
}
}
default:
break
case .read, .list, .delete:
shouldBuildTargets = false
}

positionalArguments.forEach { baseline in
args.append(contentsOf: ["--baseline", baseline])
}
}

// don't build any targets if we don't need to run it for the operation, otherwise specified targets
if commandToPerform == .`init` {
swiftSourceModuleTargets = []
} else {
if specifiedTargets.isEmpty {
swiftSourceModuleTargets = context.package.targets(ofType: SwiftSourceModuleTarget.self)
} else {
swiftSourceModuleTargets = specifiedTargets
}
}

let filteredTargets = swiftSourceModuleTargets
.filter { $0.kind == .executable }
.filter { benchmark in
let path = benchmark.directory.removingLastComponent()
return path.lastComponent == "Benchmarks" ? true : false
}
.filter { benchmark in
swiftSourceModuleTargets.first(where: { $0.name == benchmark.name }) != nil ? true : false
}
.filter { benchmark in
skipTargets.first(where: { $0.name == benchmark.name }) == nil ? true : false
}

// Build the targets
if outputFormat == .text {
if quietRunning == 0 && shouldBuildTargets {
print("Building benchmark targets in release mode for benchmark run...")
}
}

// Build targets in release mode
try filteredTargets.forEach { target in
args.append(contentsOf: ["--targets", target.name])

if shouldBuildTargets {
if outputFormat == .text {
if quietRunning == 0 {
print("Building \(target.name)")
}
}

let buildResult = try packageManager.build(
.product(target.name), // .all(includingTests: false),
parameters: .init(configuration: .release)
)

guard buildResult.succeeded else {
print(buildResult.logText)
print("Benchmark failed to run due to build error.")
return
}

// Filter out all executable products which are Benchmarks we should run
let benchmarks = buildResult.builtArtifacts
.filter { benchmark in
filteredTargets.first(where: { $0.name == benchmark.path.lastComponent }) != nil ? true : false
}

if benchmarks.isEmpty {
throw ArgumentParsingError.noMatchingTargetsForRegex
}

benchmarks.forEach { benchmark in
args.append(contentsOf: ["--benchmark-executable-paths", benchmark.path.string])
}
}
}

try withCStrings(args) { cArgs in
let newPath = benchmarkTool.path
// This doesn't work for external dependents
Expand Down Expand Up @@ -333,6 +345,8 @@ import PackagePlugin
switch waitStatus {
case .success:
break
case .baselineNotFound:
throw MyError.baselineNotFound
case .genericFailure:
print("One or more benchmark suites crashed during runtime.")
throw MyError.benchmarkCrashed
Expand Down Expand Up @@ -362,6 +376,7 @@ import PackagePlugin
case benchmarkThresholdImprovement
case benchmarkCrashed
case benchmarkUnexpectedReturnCode
case baselineNotFound
case invalidArgument
}
}
1 change: 1 addition & 0 deletions Plugins/BenchmarkCommandPlugin/Command+Helpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ enum ExitCode: Int32 {
case thresholdRegression = 2
case benchmarkJobFailed = 3
case thresholdImprovement = 4
case baselineNotFound = 5
}
1 change: 1 addition & 0 deletions Plugins/BenchmarkHelpGenerator/Command+Helpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ enum ExitCode: Int32 {
case thresholdRegression = 2
case benchmarkJobFailed = 3
case thresholdImprovement = 4
case baselineNotFound = 5
}
36 changes: 28 additions & 8 deletions Plugins/BenchmarkTool/BenchmarkTool+Operations.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,7 @@ extension BenchmarkTool {

switch baselineOperation {
case .delete:
benchmarkExecutablePaths.forEach { path in
let target = FilePath(path).lastComponent!.description
targets.forEach { target in
baseline.forEach {
removeBaselinesNamed(target: target, baselineName: $0)
}
Expand Down Expand Up @@ -137,16 +136,36 @@ extension BenchmarkTool {
print("Updated baseline '\(baselineName)'")
}
} else {
fatalError("Could not get first baselinename")
failBenchmark("Could not get first baselinename.", exitCode: .baselineNotFound)
}

case .check:
if checkAbsolute {
guard benchmarkBaselines.count == 1 else {
guard benchmarkBaselines.count == 1,
let currentBaseline = benchmarkBaselines.first,
let baselineName = baseline.first else {
print("Can only do absolute threshold violation checks for a single benchmark baseline, got: \(benchmarkBaselines.count) baselines.")
return
}
if let benchmarkPath = checkAbsolutePath { // load statically defined threshods for .p90

if benchmarks.isEmpty { // if we read from baseline and didn't run them, we put in some fake entries for the compare
currentBaseline.results.keys.forEach { baselineKey in
if let benchmark: Benchmark = .init(baselineKey.name, closure:{_ in}) {
benchmark.target = baselineKey.target
benchmarks.append(benchmark)
}
}
}

benchmarks = benchmarks.filter {
do {
return try shouldIncludeBenchmark($0.name)
} catch {
return false
}
}

if let benchmarkPath = checkAbsolutePath { // load statically defined thresholds for .p90
var thresholdsFound = false
benchmarks.forEach { benchmark in
let thresholds = BenchmarkTool.makeBenchmarkThresholds(path: benchmarkPath,
Expand All @@ -167,14 +186,15 @@ extension BenchmarkTool {
}
}
if !thresholdsFound {
print("")
if benchmarks.count == 0 {
failBenchmark("No benchmarks matching filter selection, failing threshold check.",
exitCode: .thresholdRegression)
}
failBenchmark("Could not find any matching absolute thresholds at path [\(benchmarkPath)], failing threshold check.",
exitCode: .thresholdRegression)
}
}
print("")
let currentBaseline = benchmarkBaselines[0]
let baselineName = baseline[0]

let deviationResults = currentBaseline.failsAbsoluteThresholdChecks(benchmarks: benchmarks)

Expand Down
28 changes: 17 additions & 11 deletions Plugins/BenchmarkTool/BenchmarkTool.swift
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ typealias BenchmarkResults = [BenchmarkIdentifier: [BenchmarkResult]]
@main
struct BenchmarkTool: AsyncParsableCommand {
@Option(name: .long, help: "The paths to the benchmarks to run")
var benchmarkExecutablePaths: [String]
var benchmarkExecutablePaths: [String] = []

@Option(name: .long, help: "The targets")
var targets: [String]

@Option(name: .long, help: "The command to perform")
var command: BenchmarkOperation
Expand Down Expand Up @@ -103,10 +106,6 @@ struct BenchmarkTool: AsyncParsableCommand {
@Option(name: .long, help: "Benchmarks matching the regexp filter that should be skipped")
var skip: [String] = []

var targets: [String] {
benchmarkExecutablePaths.map { FilePath($0).lastComponent!.description }
}

var inputFD: CInt = 0
var outputFD: CInt = 0

Expand Down Expand Up @@ -185,9 +184,7 @@ struct BenchmarkTool: AsyncParsableCommand {
if let baseline = try readBaseline(baselineName) {
benchmarkBaselines.append(baseline)
} else {
if quiet == false {
print("Warning: Failed to load specified baseline '\(baselineName)'.")
}
failBenchmark("Failed to load specified baseline '\(baselineName)'.", exitCode: .baselineNotFound)
}
}
}
Expand Down Expand Up @@ -232,9 +229,18 @@ struct BenchmarkTool: AsyncParsableCommand {
return
}

if let operation = baselineOperation, [.compare, .check].contains(operation), benchmarkBaselines.count > 1 {
try postProcessBenchmarkResults()
return
if let operation = baselineOperation, [.compare, .check].contains(operation) {
if checkAbsolute {
if benchmarkBaselines.count > 0 {
try postProcessBenchmarkResults()
return
}
} else {
if benchmarkBaselines.count > 1 {
try postProcessBenchmarkResults()
return
}
}
}

guard command != .query else {
Expand Down
1 change: 1 addition & 0 deletions Plugins/BenchmarkTool/Command+Helpers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ enum ExitCode: Int32 {
case thresholdRegression = 2
case benchmarkJobFailed = 3
case thresholdImprovement = 4
case baselineNotFound = 5
}
8 changes: 8 additions & 0 deletions Sources/Benchmark/Documentation.docc/GettingStarted.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,14 @@ The following example shows an benchmark suite named `My-Benchmark` with the req
),
```

### Run benchmark plugin in release mode
If you find that the runtime for e.g. baseline processing is long, it's recommended to try running the benchmark
plugin in release mode configuration, e.g.

```
swift package -c release benchmark baseline read myBaseLine
```

### Dedicated GitHub runner instances

For reproducible and good comparable results, it is *highly* recommended to set up a private GitHub runner that is completely dedicated for performance benchmark runs, as the standard GitHub CI runners are deployed on a shared infrastructure the deviations between runs can be significant and difficult to assess.
Expand Down
2 changes: 1 addition & 1 deletion Tests/BenchmarkTests/AdditionalTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ final class AdditionalTests: XCTestCase {
}

var results: [ContinuousClock.Duration] = []
var testIterations = 2_000_000
var testIterations = 100_000
for _ in 0 ..< 3 {
results.append(runWork(testIterations))
testIterations *= 10
Expand Down
4 changes: 2 additions & 2 deletions Thresholds/P90AbsoluteThresholdsBenchmark.P90Malloc.p90.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"syscalls" : 2,
"mallocCountTotal" : 1000
"syscalls" : 6,
"mallocCountTotal" : 1012
}