13

I have this code:

static std::mt19937 rnd;

// ...

static uint32_t rndInt(uint32_t min, uint32_t max) {
    return std::uniform_int_distribution<uint32_t>(min,max)(rnd);
}

Is that good practice or should I store the uniform_int_distribution?

Lightness Races in Orbit
  • 378,754
  • 76
  • 643
  • 1,055
Albert
  • 65,406
  • 61
  • 242
  • 386
  • c.f. [Use std::uniform_int_distribution and define its range later](https://stackoverflow.com/questions/28328948/use-stduniform-int-distribution-and-define-its-range-later) for info on changing the range `param` later on the same instance – underscore_d Sep 26 '18 at 11:14

3 Answers3

9

I doubt that the distribution object is expensive to create and destroy, although I suppose it might do slightly more than just store the parameters min,max. It might precalculate some useful values based on the parameters, for instance in the obvious implementation 2**32 % (max-min+1) is the number of different values from the generator that would be discarded and re-tried.

In principle, a distribution object is allowed to store inside it some bits of entropy that were drawn from the generator on a previous call to operator(), but not needed. Those bits could be used for a later operator() invocation. So if min==0 and max==1, then you can get 32 calls to operator() on the distribution per call on the generator. That's what the reset() function is about, to clear this state.

So if you use the same min/max values repeatedly, then technically you're wasting random bits by using a new distribution each time -- you could perhaps end up with fewer calls to the engine than if you kept the distribution object around. But I doubt it matters, especially since MT is fast.

Steve Jessop
  • 273,490
  • 39
  • 460
  • 699
  • Hehe, yea, mostly what I thought but good to hear it from someone else. – Albert Dec 08 '11 at 15:55
  • Btw., not sure (didn't checked), but if this class is header-only anyway, I would expect that there is probably no overhead at all as it would be all inlined. – Albert Dec 08 '11 at 16:03
  • 1
    @Albert: depends on your definition of "no overhead", I suppose. Even if the con/destructor are inlined, they still have to actually execute. You can imagine that some distribution requires a complicated set-up procedure based on the parameters before it's ready to use, it's just that `uniform_int_distribution` clearly doesn't. – Steve Jessop Dec 08 '11 at 16:05
  • See this discussion: http://stackoverflow.com/a/14858040/259795, where it is suggested that constantly resetting a distribution object (similar to recreating it) could potentially affect the distribution of the generated numbers, depending on how it is implemented. – Tyler Streeter Feb 13 '13 at 17:58
1

I generally do the following:

std::uniform_int_distribution<uint32_t> distrib(16, 97);

Then you may call distrib(rnd) multiple times without regenerating the distribution each time.

The way you are performing the operation forces the distribution to be recreated each time you make the call. If your min and max params are fixed, then create a distribution object and call it, otherwise, stay with what you have.

BTW, I would seed rnd, using time(NULL) or some other seed method.

codewiz51
  • 11
  • 2
  • 1
    `time` is a notoriously bad seed due to its determinism and typical granularity of 1 second. Seeding with one call to `std::random_device{}()` is far preferable. – underscore_d Sep 26 '18 at 11:09
0

Entropy is stored in std::mt19937 which means that you will continue the random sequence, but as noted by Steve Jessop it still has some overhead for creating an object. If you expect this function to be called frequently with the same arguments, then you can cache std::uniform_int_distribution<uint32_t> objects in a map that uses std::pair<uint32_t, uint32_t> as a key.

Sergiy Belozorov
  • 5,856
  • 7
  • 40
  • 73