migrate rand() to new random number library

[chardan]

Migrates call sites to the new random number library. Should affect any rand() calls, as well as calls to Boost random and other random number generation. Also includes enhancements to Ceph random number library to allow parametric parameters to the default calls, along with simplifications to the implementation (could make a separate PR if desired).

Please check to see if I have migrated things appropriately, there were lots of moving parts in some of these!

Thank you!

[joscollin]

retest this please

[joscollin]

@chardan

See this:

/home/jenkins-build/build/workspace/ceph-pull-requests/src/common/WeightedPriorityQueue.h:319: undefined reference to `ceph::__ceph_assert_fail(char const*, char const*, int, char const*)'
/home/jenkins-build/build/workspace/ceph-pull-requests/src/common/WeightedPriorityQueue.h:319: undefined reference to `ceph::__ceph_assert_fail(char const*, char const*, int, char const*)'

[amitkumar50]

NIT: tab left

[chardan]

Jenkins retest this please.

[cbodley]

i really like this template and how it avoids duplicating interfaces for int/real 👍

i just think the naming is backwards - i'd expect the struct to be named default_distribution, with a helper typedef named default_distribution_t. that's the pattern you see from the standard library, ex. std::enable_if

[cbodley]

was this change from limits::min() back to 0 here intentional? we discussed this case previously here

i understand your point that min=0 makes the interface closer to the rand() function that it aims to replace, but i think that a more explicit interface would be less confusing and prone to bugs in the long run

so first, i'd argue for removing the generate_random_number(max) overloads entirely, and require the caller to pass (0, max) instead:

• it only costs 2 extra characters of typing, and there's no confusion at the call site about the range of output values
• it eliminates confusion about whether this single-argument overload is the same as generate_random_number(min, max) but with a default argument for max=limits::max() (which was my first guess)
• it avoids the case of invalid input, like what happens if you call generate_random_number(-5)?

i see this generate_random_number() overload as a useful way to avoid typing out all of the std::numeric_limits<> crap, but setting min=0 seems arbitrary to me - if anything, it should use the full range of values available to the given type. that said, there may be alternative ways to save typing, like providing aliases for some common distributions, and making first-class overloads for generate_random_number(dist) that take them. then you could do something like this: generate_random_number(positive_ints);

regardless, i think we should add overloads that take a distribution, and implement the (min, max) versions in terms of that. i still don't like the way we're using thread_local for distributions that could just as easily be on the stack and stored in registers. (i'm not sure exactly what the standard says about thread_local storage, but there's bound to be some overhead there)

[chardan]

Hi @cbodley,

There's a lot here to unpack, but here we go!

1. Thanks for pointing out that thread_local distributions aren't required-- a mini-benchmark suggests that making static improves the performance of the generator!

2. I'm planning to add support for passing distributions to functions; notes below.

3. Thank you for pointing out the change in range "back" to zero. This was actually a bug fix, and I've added a test for it. The correct behavior of this library is as follows:

nullary (niladic) function: range [0... max]
unary function: range [0... n]
binary function: range [a... b], a >= b

(This excludes engine parameters, etc., merely the range parameters.)

The primary purpose of the library is found in the first two forms. The third form is actually the gravy. The first form is meant as an improved replacement for rand(). The second is for handling the overwhelmingly most common case (in Ceph, and probably most other programs) of eliminating code in the form:

rand() % bound

...for the well-known reasons that this can badly skew the output's distribution, and that there is an assumption on the engine's span.(1) That makes this form particularly important, and not something I think should be removed.

I agree with you that it's arbitrary that the unary parameter is max rather than min, but I offer that in addition to capturing the most common use of rand() in our code, there's considerable precendent for having the unary form represent a maximum bound. Although not universal, it's a popular convention-- for example:
Perl: https://perldoc.perl.org/functions/rand.html
Python: https://docs.python.org/3/library/random.html#random.randrange
Ruby: https://ruby-doc.org/core-2.2.0/Random.html
Java: (see nextInt()): https://docs.oracle.com/javase/8/docs/api/java/util/Random.html
Erlang: (>= 1): http://erlang.org/doc/man/random.html

Finally, note that the default parameters for (as example) std::uniform_int_distribution also range from [0, max] and that bad input is undefined behavior, which we are consistent with:
http://en.cppreference.com/w/cpp/numeric/random/uniform_int_distribution/uniform_int_distribution
With respect to passing bad input, that results in undefined behavior (just as it does if you pass such input to <random>). Note that using the binary form in no way prevents this, ie. these are undefined behavior (naturally, these may be variables, rather than constants):
generate_random_number(10, 1);
generate_random_number(0, -1);

We could avoid this by checking the value of the parameters at runtime and raising an exception, but I don't think we'll see this often enough in practice to warrant the almost certain performance impact. But, it could definitely be discussed as another PR.

While definitely concerned that you got bitten by this, I'm not convinced that it will be a common source of bugs, or a universal cause for surprise. If you are still feeling very strongly about this, we can move the discussion to the list, but I hope that I've assuaged your worries here. To strengthen this, in addition to the canonical example in the unit tests stating the expected behavior, I've added explicit tests.

I greatly appreciate your pointing out that thread_local storage isn't needed for distributions-- which improved the performance of the library!

I'd like to implement your suggestion of allowing distributions as parameters. I actually spent all day on it and unfortunately the SFINAE is going to be a headache in C++11-- I've juuust about got some of it working, but it needs to go into a separate PR for my own sanity. (Plus, it's now holding up this changeset.)

Thank you again for your thoughtful input, discussion, and time!

1. Brown, Walter. N3551, "Random Number Generation in C++". "https://isocpp.org/files/papers/n3551.pdf".

[chardan]

Jenkins retest this please.

[chardan]

So, how are people feeling? Shall I try to break this into some smaller changesets, or is it better all together? For instance, I could separate the library changes, adjustments to Ceph's code, and modifications to the unit tests.

[chardan]

I'm going to find a way to break this into smaller bites!

[chardan]

Refer-to:
#20670