Skip to content

Conversation

@NevinBR
Copy link
Contributor

@NevinBR NevinBR commented Aug 13, 2020

In preparation for #33455, this PR adds benchmarks for generating random floating-point numbers in the interval 0..<1.

NevinBR referenced this pull request in NevinBR/swift Aug 13, 2020
BenchmarkInfo(name: "RandomDoubleLCG", runFunction: run_RandomDoubleLCG,
tags: [.api], legacyFactor: 2),
BenchmarkInfo(name: "RandomDoubleUnitDef", runFunction: run_RandomDoubleUnitDef,
tags: [.api], legacyFactor: 100),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, newly added tests should not have a legacyFactor.

Copy link
Contributor Author

@NevinBR NevinBR Aug 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not an expert in this area, I was just copying the existing benchmarks with the intention and hope that the results would be directly comparable therewith.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I’ve removed the legacy factors from the new benchmarks.

@stephentyrone
Copy link
Contributor

As long as we're adding more tests here, I would like to also add the following cases:

  • an exact binade; 1 ..< 2 is fine.
  • a ragged example that's unbalanced around zero. Something like -(1.0.nextDown) ..< 0.126 will suffice.

@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@NevinBR NevinBR changed the base branch from master to main October 1, 2020 13:27
Also removed unneeded legacy factors from new benchmarks, and reformatted the `RandomValues` array literal to make it more readable.
@NevinBR
Copy link
Contributor Author

NevinBR commented Oct 1, 2020

As long as we're adding more tests here, I would like to also add the following cases:

  • an exact binade; 1 ..< 2 is fine.
  • a ragged example that's unbalanced around zero. Something like -(1.0.nextDown) ..< 0.126 will suffice.

Okay, I’ve added benchmarks for those ranges.

I also notice that most of these functions are identical except for the range, the RNG, and the number of iterations. So I’m thinking of making a helper function to avoid the repetition. I’m not super familiar with benchmarking though—is there any reason I shouldn’t do that?

@stephentyrone
Copy link
Contributor

Okay, I’ve added benchmarks for those ranges.

Thank you!

I also notice that most of these functions are identical except for the range, the RNG, and the number of iterations. So I’m thinking of making a helper function to avoid the repetition. I’m not super familiar with benchmarking though—is there any reason I shouldn’t do that?

I think that's perfectly reasonable. Do make sure that it doesn't fall off a performance cliff, since we want to be benchmarking the RNG and transform, not the benchmark overhead, but I don't expect that it will.

@NevinBR
Copy link
Contributor Author

NevinBR commented Oct 1, 2020

Does this look like a good approach?

@inline(never)
public func run_RandomDoubleUnitDef(_ N: Int) {
  randomDoubleHelperDef(N, 0..<1)
}

@inline(never)
public func run_RandomDoubleBinadeDef(_ N: Int) {
  randomDoubleHelperDef(N, 1..<2)
}

@inline(never)
public func run_RandomDoubleAsymDef(_ N: Int) {
  let range = -(1.0.nextDown) ..< 0.126
  randomDoubleHelperDef(N, range)
}

@inline(__always)
private func randomDoubleHelperDef(
  _ N: Int, _ range: Range<Double>, reps: Int = 1_000
) {
  for _ in 0 ..< N {
    var x = 0.0
    for _ in 0 ..< reps {
      x += Double.random(in: range)
    }
    blackHole(x)
  }
}

(And similar for LCG)

@stephentyrone
Copy link
Contributor

Seems reasonable to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants