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
Clean and streamline code in Benchmark.swift #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I just have two small comments.
@@ -51,7 +49,7 @@ public class Benchmark: Encodable { | |||
#endif | |||
|
|||
/// The list of metrics included in this benchmark. | |||
public var metrics: [BenchmarkMetric] = [] | |||
public fileprivate(set) var metrics: [BenchmarkMetric] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think keeping the setter public
would help with flexibility here, for example for clients that want to manually process this array before encoding.
@@ -130,3 +122,9 @@ public func benchmark<E, Result>(wrap event: @autoclosure () -> E, benchmarkLog | |||
return try body() | |||
} | |||
} | |||
|
|||
private extension BenchmarkMetric { | |||
static func canBenchmark(log: Benchmark) -> Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads a bit odd to me, because it could be inferred as benchmarking the log. I’m wondering if flipping the extension here would read nicer:
private extension Benchmark {
func shouldLogMetric(_ metric: BenchmarkMetric) -> Bool {
…
}
}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on shouldLogMetric
- I think the method answers more whether the metric should be logged instead of if it's being "able to" (i.e. "can") be logged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the name, I think that's a very neat optimization to extract the logic into a separate method 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an even better way to approach it, thanks for the pointer!
Even so, I think the argument's type should be BenchmarkMetric.Type
instead of BenchmarkMetric
since we want to get identifier
, which is a static var
on BenchmarkMetric
.
So how about this:
private extension Benchmark {
func shouldLogMetricType(_ metricType: BenchmarkMetric.Type) -> Bool {
return isEnabled && (metricsFilter == nil || metricType.identifier.hasPrefix(metricsFilter!))
}
}
// At the call site
if log.shouldLogMetricType(E.self) {
...
}
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better would be the following, but unfortunately we can't use it since we never get a concrete instance of type E
in any of the free benchmark
methods, but rather only as a generic constraint..
private extension Benchmark {
func shouldLogMetric<E: BenchmarkMetric>(_ metric: E) -> Bool {
return isEnabled && (metricsFilter == nil || E.identifier.hasPrefix(metricsFilter!))
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, good idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Kaan, this looks great! Swift CI is getting set up in this repo for automated testing, so I will merge this after things are configured.
@swift-ci test |
@swift-ci test |
Summary
Various quality of life improvements in Benchmark.swift
static let main
instance initialization.Make themetrics
array read-only to the outside world.id
&displayName
in case of an early-exit.Testing
Unit tests still pass, no functionality change intended.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded