Timer.measure return generic value

[vanvoorden]

Background

The current implementation of Timer.measure takes a body closure that returns no parameter. If body performs some work (or produces some value) that needs to pass a precondition, we can perform that validation in the closure:

timer.measure {
  let result = computeResult()
  precondition(
    performExpensiveValidation(result)
  )
}

If performExpensiveValidation needs to do a lot of work, we are (implicitly) capturing that work along with our measure (which might not be what we want).

An alternative is to capture result and save to a local variable outside of scope:

var result: Result?
timer.measure {
  result = computeResult()
}
precondition(
  performExpensiveValidation(result!)
)

This works… but another possible approach is to just change Timer.measure to return a value (which might be Void). Then, we can do something even simpler:

let result = timer.measure {
  computeResult()
}
precondition(
  performExpensiveValidation(result)
)

Changes

mutating func measure<T>(_ body: () -> T) -> T {
  ...
  let result = body()
  ...
  return result
}

Test Plan

Local tests (from swift-collections-benchmark) pass… but AFAIK none of these unit tests are specifically testing Timer.measure. We could choose to add those unit tests here.

For an "integration" test, we can try pointing a fork of swift-collections to this fork (vanvoorden/swift-collections@ce9db34) and then build to confirm our metrics are still collecting correctly.

swift run -c release benchmark run results.json --cycles 5 --tasks \
"OrderedSet<Int> equality different instance" \
"OrderedSet<Int> equality same instance" \
"OrderedSet<Int>.SubSequence equality different instance" \
"OrderedSet<Int>.SubSequence equality same instance"

swift run -c release benchmark render results.json results.png --amortized false --tasks \
"OrderedSet<Int> equality different instance" \
"OrderedSet<Int> equality same instance" \
"OrderedSet<Int>.SubSequence equality different instance" \
"OrderedSet<Int>.SubSequence equality same instance"

Checklist

• I've read the Contribution Guidelines
• My contributions are licensed under the Swift license.
• I've followed the coding style of the rest of the project.
• I've added tests covering my changes (if appropriate).
• I've verified that my change compiles and works correctly, and does not break any existing tests.
• I've updated the documentation (if appropriate).

[vanvoorden]

@lorentey Any thoughts about this? I've been using this patch locally for a new project. It seems to be working fine for me… is there anything unsafe about this approach?

[lorentey]

Hm; my reason for not declaring measure this way was that doing that would make it an non-inlinable (i.e., unspecializable) generic function, which has the potential to interfere with measurements in unexpected ways.

To resolve this worry, we could analyze generated code to figure out if that's actually the case (and then watch out for potential future regressions), or we could simply implement this as a second variant, in parallel to the existing measure!

I doubt we could overload the measure name without trouble (although it would be worth a try!), so the second option could require choosing an alternate name for the new variant.

[vanvoorden]

doing that would make it an non-inlinable

https://github.com/apple/swift-collections-benchmark/blob/main/Sources/CollectionsBenchmark/Basics/Timer.swift#L63

@inline(never)
  public mutating func measure(_ body: () -> Void)

@lorentey Hmm… not sure I understand… it already is not inlined by design?