Skip to content

[benchmark] Use Int.random instead of arc4random_uniform. #24091

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

Closed
wants to merge 1 commit into from

Conversation

drodriguez
Copy link
Contributor

This should be more portable, and easier to understand to Swift
programmers that the BSD function.

The usage of the BSD function was breaking the Android CI (https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-android-arm64/448/ and https://ci-external.swift.org/job/oss-swift-RA-linux-ubuntu-16.04-android/2247/). The CI machines have libbsd-devel, but I don’t understand why arc4random_uniform is not being found. I decided for this faster workaround, which makes the code more “swifty”, while I figure out what's happening in the CI machines.

This should be more portable, and easier to understand to Swift
programmers that the BSD function.
@drodriguez drodriguez requested review from atrick and rajbarik April 17, 2019 16:41
@compnerd
Copy link
Member

libbsd is not used

@compnerd
Copy link
Member

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeStubNSDateRefAccess 400 371 -7.2% 1.08x (?)
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview
  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@drodriguez
Copy link
Contributor Author

Even if the original commit was reverted in #24093, I think the improvement should be considered if the commit is tried to be land again.

@rajbarik
Copy link
Contributor

Hi! @drodriguez! Are you waiting on me here? The changes look fine to me. Please commit. Thanks.

@drodriguez
Copy link
Contributor Author

@rajbarik: since the other one was reverted, i cannot apply this one. Maybe you just need to resubmit, and incorporate this piece in your version. That's what Slava seems to imply in #24065.

@palimondo
Copy link
Contributor

I believe this change has incorporated into #24206, so this one can he closed.

@palimondo palimondo closed this Apr 29, 2019
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.

5 participants