Skip to content

Conversation

@rfourquet
Copy link
Member

@rfourquet rfourquet commented Sep 27, 2017

I did more or less what was suggested here for files in "random/", i.e. remove @inline annotations and check if performance is degraded. According to "BaseBenchmarks/src/random", there are two regressions:

  1. randn, for which I guess it's too critical, so I re-added @inline
  2. randstring: by maybe about 30%. It was not obvious which set of @inline are contributing to the regression, but I would believe that it's not a time critical function, so that it would be OK. (EDIT: not anymore on master, see post below)

I didn't run the whole BaseBenchmarks suite locally, so will run it here to check for other possible regressions: @nanosoldier runbenchmarks(ALL, vs=":master")

@rfourquet rfourquet added the randomness Random number generation and the Random stdlib label Sep 27, 2017
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@rfourquet
Copy link
Member Author

Interesting, I had performed the benchmark tests about 2 weeks ago, and in the meatime, the results changed: randstring is now not a regression anymore, and the new regressions, for randcycle, randperm and shuffle, are easily fixable by re-adding one @inline annotation. Will be easier to sell this PR :)
@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@timholy
Copy link
Member

timholy commented Sep 28, 2017

LGTM. Nice work!

@ararslan ararslan merged commit 41f3a8b into master Sep 29, 2017
@ararslan ararslan deleted the rf/rand/rm-inline branch September 29, 2017 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

randomness Random number generation and the Random stdlib

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants