Repository: ordo-one/package-benchmark (swift)

Issue:
`swift package benchmark --grouping metric` missing benchmark names
In the current `main`, (March 4th - commit: fa1a95596f5892361f909f5858246e73c2e476dc) - when you use the option `--grouping metric`, the benchmark names are removed.

Examples:
 - the stock `swift package benchmark` [command output](https://gist.github.com/heckj/62f4be896f65ef165c41b2712048416e)
 - `swift package benchmark --grouping metric` [command output](https://gist.github.com/heckj/a49e18bde9a7676b8c699c28b043f831)

Pull request:
## Description

Fixes #83 
Fixes #95

## How Has This Been Tested?

Manual testing.

## Minimal checklist:

- [x] I have performed a self-review of my own code 
- [x] I have added `DocC` code-level documentation for any public interfaces exported by the package
- [ ] I have added unit and/or integration tests that prove my fix is effective or that my feature works

Patch:
diff --git a/Benchmarks/Basic/BenchmarkRunner+Basic.swift b/Benchmarks/Basic/BenchmarkRunner+Basic.swift
index ea43156f..fbe1a81c 100644
--- a/Benchmarks/Basic/BenchmarkRunner+Basic.swift
+++ b/Benchmarks/Basic/BenchmarkRunner+Basic.swift
@@ -20,15 +20,24 @@ func benchmarks() {
                                            maxIterations: Int.max,
                                            thresholds: [.wallClock: BenchmarkResult.PercentileThresholds.strict])
 
+    // A way to define custom metrics fairly compact
+    enum CustomMetrics {
+        static var one: BenchmarkMetric { .custom("CustomMetricOne") }
+        static var two: BenchmarkMetric { .custom("CustomMetricTwo", polarity: .prefersLarger, useScalingFactor: true) }
+    }
+
     Benchmark("Basic",
               configuration: .init(metrics: [.wallClock, .throughput])) { _ in
     }
 
     Benchmark("Scaled metrics",
-              configuration: .init(metrics: BenchmarkMetric.all, scalingFactor: .kilo)) { benchmark in
+              configuration: .init(metrics: BenchmarkMetric.all + [CustomMetrics.two, CustomMetrics.one],
+                                   scalingFactor: .kilo)) { benchmark in
         for _ in benchmark.scaledIterations {
             blackHole(Int.random(in: benchmark.scaledIterations))
         }
+        benchmark.measurement(CustomMetrics.two, Int.random(in: 1 ... 1_000_000))
+        benchmark.measurement(CustomMetrics.one, Int.random(in: 1 ... 1_000))
     }
 
     Benchmark("All metrics",
diff --git a/Plugins/Benchmark/BenchmarkPlugin.swift b/Plugins/Benchmark/BenchmarkPlugin.swift
index d1aa2ab7..72c0cea0 100644
--- a/Plugins/Benchmark/BenchmarkPlugin.swift
+++ b/Plugins/Benchmark/BenchmarkPlugin.swift
@@ -23,7 +23,6 @@ import PackagePlugin
 
 @available(macOS 13.0, *)
 @main struct Benchmark: CommandPlugin {
-
     func withCStrings(_ strings: [String], scoped: ([UnsafeMutablePointer<CChar>?]) throws -> Void) rethrows {
         let cStrings = strings.map { strdup($0) }
         try scoped(cStrings + [nil])
@@ -42,6 +41,7 @@ import PackagePlugin
         let quietRunning = argumentExtractor.extractFlag(named: "quiet")
         let noProgress = argumentExtractor.extractFlag(named: "no-progress")
         let groupingToUse = argumentExtractor.extractOption(named: "grouping")
+        let metricsToUse = argumentExtractor.extractOption(named: "metric")
         let debug = argumentExtractor.extractFlag(named: "debug")
         let scale = argumentExtractor.extractFlag(named: "scale")
         var outputFormat: OutputFormat = .text
@@ -169,6 +169,10 @@ import PackagePlugin
             }
         }
 
+        metricsToUse.forEach { metric in
+            args.append(contentsOf: ["--metrics", metric.description])
+        }
+
         if outputFormat == .text {
             if quietRunning == 0 {
                 print("Build complete!")
@@ -270,8 +274,8 @@ import PackagePlugin
                     // the way the status is extracted portably is with macros - so we just need to
                     // reimplement the logic here in Swift according to the waitpid man page to
                     // get some nicer feedback on failure reason.
-                    if let waitStatus = ExitCode(rawValue:((status & 0xFF00) >> 8)) {
-                        switch waitStatus { 
+                    if let waitStatus = ExitCode(rawValue: (status & 0xFF00) >> 8) {
+                        switch waitStatus {
                         case .success:
                             break
                         case .genericFailure:
diff --git a/Plugins/BenchmarkTool/BenchmarkTool+Operations.swift b/Plugins/BenchmarkTool/BenchmarkTool+Operations.swift
index c8bfe138..e13ba958 100644
--- a/Plugins/BenchmarkTool/BenchmarkTool+Operations.swift
+++ b/Plugins/BenchmarkTool/BenchmarkTool+Operations.swift
@@ -25,6 +25,9 @@ extension BenchmarkTool {
             case let .list(benchmark):
                 benchmark.executablePath = benchmarkPath
                 benchmark.target = FilePath(benchmarkPath).lastComponent!.description
+                if metrics.isEmpty == false {
+                    benchmark.configuration.metrics = metrics
+                }
                 benchmarks.append(benchmark)
             case .end:
                 break outerloop
@@ -131,7 +134,6 @@ extension BenchmarkTool {
                     return
                 }
 
-
                 let currentBaseline = benchmarkBaselines[0]
                 let checkBaseline = benchmarkBaselines[1]
                 let baselineName = baseline[0]
diff --git a/Plugins/BenchmarkTool/BenchmarkTool+PrettyPrinting.swift b/Plugins/BenchmarkTool/BenchmarkTool+PrettyPrinting.swift
index 8e22d144..c3661f5b 100644
--- a/Plugins/BenchmarkTool/BenchmarkTool+PrettyPrinting.swift
+++ b/Plugins/BenchmarkTool/BenchmarkTool+PrettyPrinting.swift
@@ -105,7 +105,7 @@ extension BenchmarkTool {
                 adjustmentFunction = result.metrics.scale
             } else {
                 description = useGroupingDescription ? "\(result.description) \(result.metrics.unitDescriptionPretty)"
-                : "\(result.metrics.metric.description) \(result.metrics.unitDescriptionPretty)"
+                    : "\(result.metrics.metric.description) \(result.metrics.unitDescriptionPretty)"
                 adjustmentFunction = result.metrics.normalize
             }
 
diff --git a/Plugins/BenchmarkTool/BenchmarkTool.swift b/Plugins/BenchmarkTool/BenchmarkTool.swift
index 2b65e5ea..3fecb278 100644
--- a/Plugins/BenchmarkTool/BenchmarkTool.swift
+++ b/Plugins/BenchmarkTool/BenchmarkTool.swift
@@ -29,9 +29,10 @@ enum BenchmarkOperation: String, ExpressibleByArgument {
     case query // query all benchmarks from target, used internally in tool
 }
 
-extension Grouping: ExpressibleByArgument { }
-extension OutputFormat: ExpressibleByArgument { }
-extension BaselineOperation: ExpressibleByArgument { }
+extension Grouping: ExpressibleByArgument {}
+extension OutputFormat: ExpressibleByArgument {}
+extension BaselineOperation: ExpressibleByArgument {}
+extension BenchmarkMetric: ExpressibleByArgument {}
 
 typealias BenchmarkResults = [BenchmarkIdentifier: [BenchmarkResult]]
 
@@ -51,28 +52,13 @@ struct BenchmarkTool: AsyncParsableCommand {
 
     @Option(name: .long, help: "The path to export to")
     var path: String?
-/*
-    @Option(name: .long, help: "The baseline to compare with")
-    var compare: String?
-*/
+
     @Option(name: .long, help: "The operation to perform on the specified baselines")
     var baselineOperation: BaselineOperation?
-/*
-    @Option(name: .long, help: "The baseline to check thresholds with")
-    var check: String?
-*/
+
     @Flag(name: .long, help: "True if we should suppress output")
     var quiet: Int
-/*
-    @Flag(name: .long, help: "True if we should update the baseline")
-    var update: Int
 
-    @Flag(name: .long, help: "True if we should delete the baseline")
-    var delete: Int
-
-    @Flag(name: .long, help: "True if we should display available baselines")
-    var listBaselines: Int
-*/
     @Flag(name: .long, help: "True if we should suppress progress in benchmark run")
     var noProgress: Int
 
@@ -82,6 +68,9 @@ struct BenchmarkTool: AsyncParsableCommand {
     @Option(name: .long, help: "The named baseline(s) we should display, update, delete or compare with")
     var baseline: [String] = []
 
+    @Option(name: .long, help: "The metrics to use, overrides whatever the benchmark has specified as desired metrics.")
+    var metrics: [BenchmarkMetric] = []
+
     @Option(name: .long, help: "The grouping to use, 'metric' or 'test'")
     var grouping: Grouping
 
@@ -142,14 +131,6 @@ struct BenchmarkTool: AsyncParsableCommand {
                 }
             }
 
-            // Filter out any results that doesn't match regex filtering
-/*            readBaselines.first?.results.filter
-            readBaselines.forEach { baseline in
-
-            }
-*/
-
-
             // Merge baselines read
             if readBaselines.isEmpty == false {
                 var aggregatedBaseline = readBaselines.first!
diff --git a/Sources/Benchmark/BenchmarkMetric.swift b/Sources/Benchmark/BenchmarkMetric.swift
index 2749cbb8..2ffd7f79 100644
--- a/Sources/Benchmark/BenchmarkMetric.swift
+++ b/Sources/Benchmark/BenchmarkMetric.swift
@@ -72,6 +72,20 @@ public enum BenchmarkMetric: Hashable, Equatable, Codable, CustomStringConvertib
     case deltaPercentage
 }
 
+// We don't want to take polarity and useScalingFactor into consideration as it makes dealing with custom metrics hard
+#if swift(>=5.8)
+    @_documentation(visibility: internal)
+#endif
+public extension BenchmarkMetric {
+    func hash(into hasher: inout Hasher) {
+        hasher.combine(description)
+    }
+
+    static func == (lhs: BenchmarkMetric, rhs: BenchmarkMetric) -> Bool {
+        lhs.description == rhs.description
+    }
+}
+
 public extension BenchmarkMetric {
     /// A constant that states whether larger or smaller measurements, relative to a set baseline, indicate better performance.
     enum Polarity: Codable { // same naming as XCTest uses, polarity is known for all metrics except custom
@@ -186,9 +200,9 @@ public extension BenchmarkMetric {
 #if swift(>=5.8)
     @_documentation(visibility: internal)
 #endif
-extension BenchmarkMetric {
-    init(_ textualMetric: String) {
-        switch textualMetric {
+public extension BenchmarkMetric {
+    init?(argument: String) {
+        switch argument {
         case "cpuUser":
             self = BenchmarkMetric.cpuUser
         case "cpuSystem":
@@ -234,7 +248,7 @@ extension BenchmarkMetric {
         case "writeBytesPhysical":
             self = BenchmarkMetric.writeBytesPhysical
         default:
-            self = BenchmarkMetric.custom(textualMetric)
+            self = BenchmarkMetric.custom(argument)
         }
     }
 }
diff --git a/Sources/BenchmarkSupport/BenchmarkExecutor.swift b/Sources/BenchmarkSupport/BenchmarkExecutor.swift
index d02c339a..ed0e7fce 100644
--- a/Sources/BenchmarkSupport/BenchmarkExecutor.swift
+++ b/Sources/BenchmarkSupport/BenchmarkExecutor.swift
@@ -30,7 +30,7 @@ internal final class BenchmarkExecutor {
     let operatingSystemStatsProducer = OperatingSystemStatsProducer()
 
     // swiftlint:disable cyclomatic_complexity function_body_length
-    func run(_ benchmark: Benchmark, _ targetName: String) -> [BenchmarkResult] {
+    func run(_ benchmark: Benchmark) -> [BenchmarkResult] {
         var wallClockDuration: Duration = .zero
         var startMallocStats = MallocStats()
         var stopMallocStats = MallocStats()
@@ -55,6 +55,8 @@ internal final class BenchmarkExecutor {
         // Create metric statistics as needed
         benchmark.configuration.metrics.forEach { metric in
             switch metric {
+            case .custom:
+                statistics[metric] = Statistics(prefersLarger: metric.polarity == .prefersLarger)
             case .wallClock, .cpuUser, .cpuTotal, .cpuSystem:
                 let units = Statistics.Units(benchmark.configuration.timeUnits)
                 statistics[metric] = Statistics(units: units)
@@ -209,7 +211,7 @@ internal final class BenchmarkExecutor {
         var progressBar: ProgressBar?
 
         if quiet == false {
-            let progressString = "| \(targetName):\(benchmark.name)"
+            let progressString = "| \(benchmark.target):\(benchmark.name)"
 
             progressBar = ProgressBar(count: 100,
                                       configuration: [ProgressPercent(),
diff --git a/Sources/BenchmarkSupport/BenchmarkRunner.swift b/Sources/BenchmarkSupport/BenchmarkRunner.swift
index d4c5866b..e7f89ed2 100644
--- a/Sources/BenchmarkSupport/BenchmarkRunner.swift
+++ b/Sources/BenchmarkSupport/BenchmarkRunner.swift
@@ -93,7 +93,23 @@ public struct BenchmarkRunner: AsyncParsableCommand, BenchmarkRunnerReadWrite {
                 benchmark = Benchmark.benchmarks.first { $0.name == benchmarkToRun.name }
 
                 if let benchmark {
-                    results = benchmarkExecutor.run(benchmark, benchmarkToRun.target)
+                    // Pick up some settings overridden by BenchmarkTool
+                    if benchmarkToRun.configuration.metrics.isEmpty == false {
+                        for metricIndex in 0 ..< benchmarkToRun.configuration.metrics.count {
+                            let metric = benchmarkToRun.configuration.metrics[metricIndex]
+                            if metric == .custom(metric.description) {
+                                if let existingMetric =
+                                    benchmark.configuration.metrics.first(where: { $0.description == metric.description }) {
+                                    benchmarkToRun.configuration.metrics[metricIndex] = existingMetric
+                                }
+                            }
+                        }
+                        benchmark.configuration.metrics = benchmarkToRun.configuration.metrics
+                    }
+
+                    benchmark.target = benchmarkToRun.target
+
+                    results = benchmarkExecutor.run(benchmark)
 
                     guard benchmark.failureReason == nil else {
                         try channel.write(.error(benchmark.failureReason!))


Test patch:
diff --git a/Tests/BenchmarkTests/BenchmarkMetricsTests.swift b/Tests/BenchmarkTests/BenchmarkMetricsTests.swift
index 8d03cb9b..2647cc33 100644
--- a/Tests/BenchmarkTests/BenchmarkMetricsTests.swift
+++ b/Tests/BenchmarkTests/BenchmarkMetricsTests.swift
@@ -36,7 +36,7 @@ final class BenchmarkMetricsTests: XCTestCase {
         .writeBytesLogical,
         .readBytesPhysical,
         .writeBytesPhysical,
-        .custom("test"),
+        .custom("test", polarity: .prefersSmaller, useScalingFactor: false),
         .custom("test2", polarity: .prefersLarger, useScalingFactor: true)
     ]
 
@@ -83,9 +83,12 @@ final class BenchmarkMetricsTests: XCTestCase {
         var description = ""
 
         for metricIndex in 0 ..< textualMetrics.count {
-            let metric = BenchmarkMetric(textualMetrics[metricIndex])
-            description += metric.description
-            XCTAssertEqual(metrics[metricIndex], metric)
+            if let metric = BenchmarkMetric(argument: textualMetrics[metricIndex]) {
+                description += metric.description
+                XCTAssertEqual(metrics[metricIndex], metric)
+            } else {
+                XCTFail("Could not extract metric \(textualMetrics[metricIndex])")
+            }
         }
 
         XCTAssert(description.count > 10)