Clean and streamline code in Benchmark.swift

[kaandedeoglu]

Summary

Various quality of life improvements in Benchmark.swift

• Remove abundant closure for static let main instance initialization.
• Make the metrics array read-only to the outside world.
• Avoid calculating id & displayName in case of an early-exit.
• Unify checks prior to running benchmarks, remove duplication.

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.

• Ran the ./bin/test script and it succeeded

[franklinsch]

Thanks for doing this! I just have two small comments.

[franklinsch]

I think keeping the setter public would help with flexibility here, for example for clients that want to manually process this array before encoding.

[franklinsch]

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?

[icanzilb]

+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.

[icanzilb]

Other than the name, I think that's a very neat optimization to extract the logic into a separate method 👍🏼

[kaandedeoglu]

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?

[kaandedeoglu]

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!))
    }
}

[franklinsch]

Makes sense, good idea!

[franklinsch]

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.

[franklinsch]

@swift-ci test

[franklinsch]

@swift-ci test