Rework MersenneTwisterEngine using Ilya Yaroshenko's faster algorithm #5011

WebDrake · 2017-01-03T23:14:51Z

These patches update MersenneTwisterEngine's implementation to use the new and much faster implementation @9il prepared for mir.random. The standard 64-bit Mersenne Twister implementation is added as a side benefit.

The original Mersenne Twister algorithm (as per the reference C code by Matsumoto and Nishimura) first regenerates the contents of the entire state array, before looping over the resulting values and tempering them in order to generate the actual variates. Once the entire state array has been iterated over, the array's values are then regenerated, and so on and so forth.

Ilya's implementation reworks this idea to intertwine update of a single entry in the state array with the tempering of another value to produce the next variate. This ensures that all the registers concerned stay 'hot' in the CPU's memory and hence ensures significant speedup.

(Just as a mention: as an experiment while adapting this code for phobos I tried splitting apart the lines responsible for updating the internal state array from those responsible for tempering the next variate, so that they could be run sequentially instead of mixed up together; this resulted in a fairly significant speed hit.)

Ilya's code also introduces an extra tempering variable d, used in more recent Mersenne Twister implementations (Boost.Random, hap.random) but previously missing from Phobos' implementation. This is needed in order to implement the standard 64-bit version of the Mersenne Twister, which is added in the second patch of this PR.

The first patch reworks Ilya's code to work as a drop-in replacement for the existing Phobos implementation, which is currently implemented as a forward range (although it should ideally be an input range, but that design flaw should be fixed as a separate issue). There appears to be no significant speed hit from implementing the algorithm as a range rather than as a functor. However, other aspects of the original design have required some rather intrusive changes to Ilya's code in order to get the full speed benefits without introducing breaking change.

The original MersenneTwisterEngine allows for implicit instantiation of generator instances without providing a seed. This is handled by checking in the front and popFront method whether the generator has been properly initialized, and seeding it with the default seed if not. However, these runtime checks on every single call result in a massive speed hit. The current implementation therefore takes a number of steps in order to ensure that the internal state of the generator can be set
to its default-seeded values at compile time:

all the internal state variables have been wrapped up in a nested State struct to facilitate generation;
the internals of the seed and popFront methods have been separated out into CTFE'able private static methods (seedImpl and popFrontImpl) which take a reference to a State instance as input;
a CTFE'able private static defaultState method has been added which generates a State instance matching that generated by instantiating the generator with the defaultSeed.

The defaultState method is then used to initialize the default value of the internal State instance at compile-time, replicating the effect of the original runtime seeding check.

These latter workarounds could be removed in future if the generator were updated to @disable this(); and therefore always require explicit seeding, but this would be a breaking change and so is avoided for now.

WebDrake · 2017-01-03T23:23:46Z

NOTE: the CTFE aspects of the current implementation have some known issues, which AFAICT are dmd's issues rather than those of the implementation per se. I'm submitting for review anyway in the hope that it will help identify a solution or an alternative way of achieving the desired result.

In particular, the default initialization of the internal state field by defaultState works with ldc (checked with v1.1.0-beta6) but appears not to work with dmd (checked with both v2.072.2 and v2.071.2 [the latter matching the frontend version of ldc v1.1.0]): while the state.z value winds up set correctly, all the other values (including those in the data array) wind up being zero.

I've introduced a workaround patch to demonstrate the fundamental correctness of the new MersenneTwisterEngine implementation (it explicitly seeds some generator instances which rely on the "implicit" default seeding). This 3rd patch should be removed before the code is considered merge-ready as otherwise the code cannot be guaranteed to work as a drop-in replacement.

WebDrake · 2017-01-03T23:31:08Z

std/random.d

+       Constructs a MersenneTwisterEngine object by
+       duplicating the state of an existing instance
+    */
+    this(ref typeof(this) that)


A small note on the motivation for this constructor overload (which I should possibly add to the patch description if we decide to keep it). A long-term goal is to remove the .save property of Phobos' random number generators: however, it is still useful to be able to deliberately deep-copy the state of an RNG in order to create a second generator that will produce identical results.

Having a constructor that copies an existing generator provides a way to "wrap up" the details of duplicating a generator in a way that can both be used directly (to construct a new instance) and also wrapped by other methods that might require it (save while it's still needed to avoid breaking change; dup has been proposed in future as a method that provides duplication without creating a forward range).

However, right now, this is creating a new feature that we might not want to support. So it might be better to either make this constructor instance private or to remove it for now and just rework the internals of save.

WebDrake · 2017-01-03T23:50:45Z

Re-pushed with fixes for the (unnecessary) nested import that caused circleci tests to fail, and a couple of other small issues that I noticed while editing the code:

diff --git a/std/random.d b/std/random.d
index 7fab38f..42c6389 100644
--- a/std/random.d
+++ b/std/random.d
@@ -534,8 +534,6 @@ struct MersenneTwisterEngine(UIntType, size_t w, size_t n, size_t m, size_t r,
                              uint l)
     if (isUnsigned!UIntType)
 {
-    import std.range.primitives;
-
     static assert(0 < w && w <= UIntType.sizeof * 8);
     static assert(1 <= m && m <= n);
     static assert(0 <= r && 0 <= u && 0 <= s && 0 <= t && 0 <= l);
@@ -580,7 +578,6 @@ Parameters for the generator.
         private UIntType z = 0;
         private UIntType index = void;
         private UIntType[n] data = void;
-
     }
 
     /// State variables used by the generator;
@@ -629,7 +626,7 @@ Parameters for the generator.
     */
     private static State defaultState() @safe pure nothrow @nogc
     {
-        //if (!__ctfe) assert(false);
+        if (!__ctfe) assert(false);
         State mtState;
         seedImpl(defaultSeed, mtState);
         return mtState;

WebDrake · 2017-01-03T23:53:14Z

I don't understand the current circleci test failures -- they don't look like something to do with the PR ... ? Can someone advise?

wilzbach · 2017-01-04T00:01:57Z

I don't understand the current circleci test failures -- they don't look like something to do with the PR ... ? Can someone advise?

Yep, it's unrelated - the problem is in this line:

curl: (22) The requested URL returned error: 403

(which is caused by sending more than 50 unauthenticated requests to the GitHub API within one hour)
Subsequently base_branch isn't set and the dmd / druntime installation fails.
IIRC there was a fallback to master in case of overusing the API, which seems not to work for 403s.
(the request to GitHub is needed to check against which branch a PR was opened against)

WebDrake · 2017-01-04T00:05:31Z

Anything I can do to re-trigger the tests, or will they re-trigger automatically at some point?

9il · 2017-01-04T07:02:42Z

@WebDrake Could you please add credits and link to Mir Random https://github.com/libmir/mir-random.

wilzbach · 2017-01-04T13:27:31Z

Anything I can do to re-trigger the tests, or will they re-trigger automatically at some point?

Unfortunately CircleCi won't restart itself without external action (the usual git push/sync events and PR close). AFAIK only maintainer can re-trigger a build manually, but it seems someone already did.
In any case I submitted #5012, which should fix this problem by using a "soft fallback".

WebDrake · 2017-01-04T19:06:51Z

std/random.d

+        if (conj < 0)
+            conj = index - m + n;
+        static if (d == UIntType.max)
+            z ^= (z >> u);


Very odd that code coverage is suggesting this line is unused, as it should be for UIntType == uint i.e. for Mt19937.

WebDrake · 2017-01-04T19:08:32Z

std/random.d

        }
-        popFront();
+        static if (is(UIntType == uint))
+            enum UIntType f = 1812433253;


It's odd that this branch is marked as not covered by codecov.io as it should be used for Mt19937.

this is compile time enum. It should not be covered if i am not wrong

Ah, of course. There's still another static if on L758-759 that codecov.io doesn't seem to like, though, which is weird.

I ran the code coverage locally while running phobos unittests:

| private static void seedImpl(UIntType value, ref State mtState) | { | static if (w == UIntType.sizeof * 8) | { 8| mtState.data[$-1] = value; | } | else | { | static assert(max + 1 > 0); 00000000| mtState.data[$-1] = value % (max + 1); | } | static if (is(UIntType == uint)) 00000000| enum UIntType f = 1812433253; | else static if (is(UIntType == ulong)) 8| enum UIntType f = 6364136223846793005; | else | static assert(0, UIntType.stringof ~ " is not supported by MersenneTwisterEngine."); 7488| foreach_reverse (size_t i, ref e; mtState.data[0 .. $-1]) 2488| e = f * (mtState.data[i + 1] ^ (mtState.data[i + 1] >> (w - 2))) + cast(UIntType)(n - (i + 1)); 8| mtState.index = n-1; | | // double popFront() to guarantee both | // `_z` and `_y` are derived from the | // newly set values in `data` 8| MersenneTwisterEngine.popFrontImpl(mtState); 8| MersenneTwisterEngine.popFrontImpl(mtState); | }

The first 00000000 line makes sense, but the second doesn't considering that the else branch is marked as covered 8 times (and is also a compile-time enum).

Very strange. Maybe it is always 64 bit?

I'll check, but I don't think there is any sane way that can be true; Mt19937 (uses uint) and Mt19937_64 (uses ulong) are both instantiated and used.

WebDrake · 2017-01-04T19:45:16Z

std/random.d

+                             uint u, UIntType d,
+                             uint s, UIntType b,
+                             uint t, UIntType c,
+                             uint l)


@9il one question regarding your implementation. Was there any particular reason to replace all the size_t template parameters with uint?

Shift parameters are ok to be uint. No strong reasons.

OK, cool, thanks! I'm trying to find ways in which to minimize the diff so as to make it most informative, but TBH I think having fixed-size template parameters is probably preferable.

Hmm, on checking, the C++11 standard explicitly defines these template parameters as size_t. While it may not make any practical difference, I don't see any reason to diverge from that spec, and it minimizes the different with phobos original, so I'm going to restore the size_t.

WebDrake · 2017-01-04T20:49:07Z

I've pushed a bunch of small fixes and extra tests, including the credit to Ilya and mir.random. Most of these will be squashed into the earlier patches; a couple are extra tests that should be placed before the new-algorithm patches in the final patchset.

WebDrake · 2017-01-04T20:51:04Z

I see circleci is up to another of its evil tricks ... anyway, feedback welcome on the current code.

WebDrake · 2017-01-04T20:52:38Z

std/random.d

    foreach (R; std.meta.AliasSeq!(MT!(uint, 32), MT!(ulong, 32), MT!(ulong, 48), MT!(ulong, 64)))
+    {
        auto a = R();
+        a.seed(a.defaultSeed); // checks that some alternative paths in `seed` are utilized


A note on this change: ideally it would be nice to include checks on a.front (before & after popFrontN(9999)). I don't have any values from a reference implementation to compare to right now, but I'll try to follow up on this soon-ish.

WebDrake · 2017-01-06T01:06:18Z

OK, in the absence of concrete feedback I've squashed together many corrections to minimize the number of patches to review, and re-ordered things to put some new unittests before the transition to the new algorithm.

The template parameters have been restored to match the C++11 standard, including the addition of a missing parameter f (which I've added as a separate [to-squash] patch to clarify the change during review). I'll submit patches to mir.random for this as well once review here is complete.

In the course of working on this PR and finalizing tests, two issues have come to light:

@9il's implementation appears not to fully support cases where the word size w does not match the number of bits in UIntType. This doesn't change anything relative to the existing implementation (in fact overall things get better) but is something it would be nice to fix here if possible. I haven't fully tracked down a fix yet (hence no changes included yet in the PR) but some likely components include:
- ensuring that mtState.y (the cached .front value) takes only the lower w bits of the final value resulting from tempering;
- ensuring that the UIntType-based seedImpl handles things correctly.
while the range-based seed method produces identical results to the previous phobos implementation, neither of them appears to match the sequence-based seeding mechanism available in the C++11 and later standard library. It's possible that Phobos' mechanism is based on an out-of-date implementation. We can consider this a separate issue to be fixed at a later date, but it should be noted. The range-based constructor introduced in this PR should probably be removed for this reason.

Note that the CTFE-related issue mentioned earlier in the PR still applies and still needs working around for the time being.

9il · 2017-01-06T11:05:07Z

@WebDrake, what CPP implementation do you use?

WebDrake · 2017-01-07T16:23:54Z

@9il my comparison is made based on g++ 5.4.0. More specifically, compare the output of the following D code (slightly adapted from one of the phobos unittests):

unittest
{
    alias MT(UIntType, uint w) = MersenneTwisterEngine!(UIntType, w, 624, 397, 31,
                                                        0x9908b0df, 11, 0xffffffff, 7,
                                                        0x9d2c5680, 15,
                                                        0xefc60000, 18, 1812433253);

    foreach (R; std.meta.AliasSeq!(MT!(uint, 32), MT!(ulong, 32), MT!(ulong, 48), MT!(ulong, 64)))
    {
        static if (R.wordSize == 48) static assert(R.max == 0xFFFFFFFFFFFF);
        auto a = R();
        a.seed(a.defaultSeed); // checks that some alternative paths in `seed` are utilized
        import std.stdio : writeln;
        writeln(a.front);  // for mir.random replace these 3 lines with
        a.popFront();      // writeln(a());
        writeln(a.front);  // writeln(a());
        writeln();
    }
}

to this C++11 program:

#include <cinttypes>
#include <iostream>
#include <random>
#include <type_traits>

template<class UIntType, size_t w>
void mt_result ()
{
    std::mersenne_twister_engine<
        UIntType, w, 624, 397, 31,
        0x9908b0df, 11, 0xffffffff, 7,
        0x9d2c5680, 15,
        0xefc60000, 18, 1812433253> gen;

    gen.seed(std::mt19937::default_seed);
    std::cout << gen() << std::endl;
    std::cout << gen() << std::endl;
    std::cout << std::endl;
}

int main ()
{
    mt_result<uint32_t, 32>();
    mt_result<uint64_t, 32>();
    mt_result<uint64_t, 48>();
    mt_result<uint64_t, 64>();
}

I've submitted a PR to update the mir.random Mersenne Twister's template signature to match C++11 precisely. From there it should be easier to make comparisons and pinpoint the source of the problem.

So far as I can work out from comparison to MT spec, the essentials of the problem are that the word size w is not fully taken into account when seeding the generator nor when returning tempered variates.

9il · 2017-01-07T16:26:10Z

I've submitted a PR to update the mir.random Mersenne Twister's template signature to match C++11 precisely. From there it should be easier to make comparisons and pinpoint the source of the problem.

Thanks! Merged

WebDrake · 2017-01-07T19:17:32Z

Thanks to some fast fixes from @9il the word-size issue is now addressed, and a unittest has been added to validate comparison to expected behaviour when the word size occupies less than a full UIntType.

How would everyone like to proceed from here? If I don't have more detailed feedback by tomorrow I think it may be better to squash everything down once more to a clean patchset.

WebDrake · 2017-01-07T19:20:18Z

std/random.d

+    {
+        private UIntType y = void;
+        private UIntType z = 0;
+        private UIntType index = void;


This should be a size_t.

WebDrake · 2017-01-07T19:21:12Z

std/random.d

    static assert(0 <= r && 0 <= u && 0 <= s && 0 <= t && 0 <= l);
    static assert(r <= w && u <= w && s <= w && t <= w && l <= w);
    static assert(0 <= a && 0 <= b && 0 <= c);
+    static assert(n < UIntType.max);


Should probably be a comparison to sizediff_t.max to make sure the index values will always fall within acceptable bounds.

dlang-bot · 2017-01-07T22:03:59Z

Fix	Bugzilla	Description
✓	10900	Mersenne Twister should have a 64-bit (ulong) version

WebDrake · 2017-01-07T22:05:49Z

OK, with the major issues dealt with I've squashed everything down to a clean set of patches that I think can be considered "final", bar feedback and bar the resolution of the CTFE issue related to the WORKAROUND patch.

I'll submit a separate issue report for the range-based seeding concern.

WebDrake · 2017-01-07T22:27:15Z

Range-based seeding mechanism issue is now reported here:
https://issues.dlang.org/show_bug.cgi?id=17068

WebDrake · 2017-01-17T23:28:43Z

since it corrects some behaviour of the Mersenne Twister implementation compared to what was previously in Phobos -- specifically, related to the word size w

I realized this was not noted in the main commit message, so I've added a note on this too. Genuinely the last change I can think of (absent any review feedback).

@JackStouffer thanks for adding the tags :-)

JackStouffer · 2017-01-19T13:57:15Z

Pinging people who are qualified to review this based on git blame @braddr @jpf91 @9il

WebDrake · 2017-02-06T22:58:54Z

Folks, I'm having a bit of a difficult time understanding why these tests are failing. It looks like they are either due to integration issues with the CI or to problems elsewhere in Phobos ... ?

In any case, ping regarding review of this PR?

WebDrake · 2017-02-08T18:16:38Z

Rebased on master. Hopefully should get a test pass now that the CI issues have been dealt with.

WebDrake · 2017-02-08T18:18:51Z

Again, the failure doesn't look like anything to do with this patchset. :-\

andralex

Like! Please mind the nits.

andralex · 2017-02-14T03:52:39Z

std/random.d

+                             UIntType a, size_t u, UIntType d, size_t s,
                             UIntType b, size_t t,
-                             UIntType c, size_t l)
+                             UIntType c, size_t l, UIntType f)


This looks like a breaking change, at least technically. Anything to worry about, i.e. are people likely to instantiate the engine with their own constants?

Yes, technically it's a breaking change. I'd consider it quite unlikely that anyone is using the template directly, though (and if they are it is probably not safe to do so given the missing parameters).

I'll make sure this is noted prominently in the changelog, though.

OK, I'm fine with this. cc @MartinNowak

andralex · 2017-02-14T04:07:24Z

std/random.d


+    /// Bitmasks used in the 'twist' part of the algorithm
+    private enum UIntType lowerMask = (cast(UIntType) 1u << r) - 1;
+    private enum UIntType upperMask = (~lowerMask) & this.max; // ditto


perhaps use a comma instead of repeating private enum UIntType?

andralex · 2017-02-14T04:07:47Z

std/random.d

+    private enum UIntType upperMask = (~lowerMask) & this.max; // ditto
+
+    /// Collection of all state variables
+    /// used by the generator


Please use block comments instead.

andralex · 2017-02-14T04:08:00Z

std/random.d

+        /// last element to first), providing a
+        /// few extra compiler optimizations by
+        /// comparison to the forward iteration
+        /// used in most implementations.


here, there, and everywhere

andralex · 2017-02-14T04:08:50Z

std/random.d

+        if (!__ctfe) assert(false);
+        State mtState;
+        seedImpl(defaultSeed, mtState);
+        return mtState;


eh, we need -cov to cover CTFE...

andralex · 2017-02-14T04:09:18Z

std/random.d

+    */
+    private static void seedImpl(UIntType value, ref State mtState)
+    {
+        mtState.data[$-1] = value;


space around operators

cc @wilzbach

andralex · 2017-02-14T04:09:38Z

std/random.d

        {
-            static assert(max + 1 > 0);
-            mt[0] = value % (max + 1);
+            mtState.data[$-1] &= this.max;


andralex · 2017-02-14T04:10:04Z

std/random.d

        }
-        popFront();
+
+        mtState.index = n-1;


and more spaces throughout

andralex · 2017-02-14T04:11:01Z

std/random.d

+    /// State variables used by the generator;
+    /// initialized to values equivalent to
+    /// explicitly seeding the generator with
+    /// `defaultSeed`


BTW these should not be doc comments, right?

Everything commented with /// was intended as a ddoc comment. I assumed it was better to provide the info and leave it to the doc generator to decide what symbols' doc to expose. (IIRC as private symbols they should not end up in generated docs by default.)

Would you prefer these be regular comments?

@WebDrake they may confuse the reader (was this symbol supposed to be exposed? if not why is this a ddoc comment? etc). At any rate, it may be nice to transform these into block comments. Then we can switch from ddoc to regular comment with a single character change.

OK, makes sense. I'll follow up on this tomorrow.

JackStouffer · 2017-02-14T05:27:03Z

Also, please add a changelog entry.

WebDrake · 2017-02-14T14:30:04Z

Thanks for the detailed feedback, everyone :-) I'll try to follow up by tomorrow evening (today is a busy day).

For amusement, check the first variate of the newly-seeded sequence as well as the 10_000th. Both values in this unittest can be verified by comparison to C++11 by compiling and running the following C++ program: /****************************************************************/ int main () { static_assert(std::mt19937::default_seed == 5489, "Default seed does not match Phobos!"); std::mt19937 gen(std::mt19937::default_seed); std::cout << gen() << std::endl; for (int i = 0; i < 9998; ++i) { gen(); } std::cout << gen() << std::endl; } /****************************************************************/ Note that the `for` loop in this example advances the generator 9998 times compared to the D unittest's `popFrontN(9999)` because the first `gen()` call already advances the generator once.

This is added purely in order to verify that generator behaviour will not change with upcoming code changes. It does not appear to match the behaviour of the superficially similar sequence-based seeding available for the C++11 <random> implementation of the Mersenne Twister.

This patch updates MersenneTwisterEngine's implementation to use the new and much faster implementation Ilya prepared for `mir.random`. The original Mersenne Twister algorithm (as per the reference C code by Matsumoto and Nishimura) first regenerates the contents of the entire state array, before looping over the resulting values and tempering them in order to generate the actual variates. Once the entire state array has been iterated over, the array's values are then regenerated, and so on and so forth. Ilya's implementation reworks this idea to intertwine update of a single entry in the state array with the tempering of another value to produce the next variate. This ensures that all the registers concerned stay 'hot' in the CPU's memory and hence ensures significant speedup. (Just as a mention: as an experiment while adapting this code for phobos I tried splitting apart the lines responsible for updating the internal state array from those responsible for tempering the next variate, so that they could be run sequentially instead of mixed up together; this resulted in a fairly significant speed hit.) In contrast to most (all?) other Mersenne Twister implementations, this one iterates backwards through the state array, which allows for a few extra compiler optimizations that speed up the algorithm. It has thus been necessary to rework the range-based `seed` method to take account of this. Besides the algorithmic change, this patch introduces two new template variables: an extra tempering variable `d`, and the initialization multiplier `f`, which brings the template implementation in line with that of the C++11 standard. These extra template variables are needed in order to effectively implement the standard 64-bit version of the Mersenne Twister, which will be added in a follow-up patch. Finally, this implementation introduces corrections to the handling of the word size `w` to ensure that the right sequences are produced when the word size is less than the number of bits in `UIntType`. Unittests will be added for this in a follow-up patch. ------------------------------------------------------------------------ Where this implementation differs from that in `mir.random`: Ilya's original design has been reworked as a drop-in replacement for the existing Phobos implementation, which is currently implemented as a forward range (although it should ideally be an input range, but that design flaw should be fixed as a separate issue). There appears to be no significant speed hit from reworking Ilya's code as a range rather than as a functor. However, some other aspects of the original design have required rather intrusive changes in order to get the full speed benefits without introducing breaking change. The original MersenneTwisterEngine allows for implicit instantiation of generator instances without providing a seed. This is handled by checking in the `front` and `popFront` method whether the generator has been properly initialized, and seeding it with the default seed if not. However, these runtime checks on every single call result in a massive speed hit. The current implementation therefore takes a number of steps in order to ensure that the internal state of the generator can be set to its default-seeded values at compile time: * all the internal state variables have been wrapped up in a nested `State` struct to facilitate generation; * the internals of the `seed` and `popFront` methods have been separated out into CTFE'able private static methods (`seedImpl` and `popFrontImpl`) which take a reference to a `State` instance as input; * a CTFE'able private static `defaultState` method has been added which generates a `State` instance matching that generated by instantiating the generator with the `defaultSeed`. The `defaultState` method is then used to initialize the default value of the internal `State` instance at compile-time, replicating the effect of the original runtime seeding check. These latter workarounds could be removed in future if the generator were updated to `@disable this();` and therefore always require explicit seeding, but this would be a breaking change and so is avoided for now.

With the required tempering parameter `d` introduced by the previous patch, we can now introduce the standard 64-bit implementation of the Mersenne Twister. See https://en.wikipedia.org/wiki/Mersenne_Twister for an explanation of the chosen constants. Some minimal unittests have been added similar to those already present for the 32-bit `Mt19937`. These can be verified by comparison to C++11 by compiling and running the following C++ program: /****************************************************************/ int main () { static_assert(std::mt19937_64::default_seed == 5489, "Default seed does not match Phobos!"); std::mt19937_64 gen(std::mt19937_64::default_seed); std::cout << gen() << std::endl; for (int i = 0; i < 9998; ++i) { gen(); } std::cout << gen() << std::endl; } /****************************************************************/ Note that the `for` loop in this example advances the generator 9998 times compared to the D unittest's `popFrontN(9999)` because the first `gen()` call already advances the generator once. Fixes Issue dlang#10900 <https://issues.dlang.org/show_bug.cgi?id=10900>.

The word size for `MersenneTwisterEngine` is determined by the template parameter `w`, not `UIntType`. For example, a generator with identical parameters to the standard `Mt19937` but with a `ulong` wordtype should not produce different results to the standard `uint`-based generator. This patch adds unittests for the first and 10_000'th values of the sequences generated by a variety of Mersenne Twister implementations with non-standard template parameter values. The values in this unittest can be validated by comparison to C++11 by compiling and running the following C++ program: /****************************************************************/ template<class UIntType, size_t w> void mt_result () { std::mersenne_twister_engine< UIntType, w, 624, 397, 31, 0x9908b0df, 11, 0xffffffff, 7, 0x9d2c5680, 15, 0xefc60000, 18, 1812433253> gen; gen.seed(std::mt19937::default_seed); std::cout << gen() << std::endl; for (int i = 0; i < 9998; ++i) gen(); std::cout << gen() << std::endl; std::cout << std::endl; } int main () { mt_result<uint32_t, 32>(); mt_result<uint64_t, 32>(); mt_result<uint64_t, 48>(); mt_result<uint64_t, 64>(); } /****************************************************************/ Note that the `for` loop in this example advances the generator 9998 times compared to the D unittest's `popFrontN(9999)` because the first `gen()` call already advances the generator once.

As with the earlier test of range-based seeding for the 32-bit Mt19937, this test is added purely in order to ensure consistency of behaviour in future. It does not appear to match the behaviour of the superficially similar sequence-based seeding available in the C++11 <random> version of the Mersenne Twister. It does however match the behaviour of the `MersenneTwisterEngine` implementation available in `hap.random`, which was derived from the `Boost.Random` implementation, but whose range-based seeding was copied from Phobos. This would suggest that any divergence with C++11 is down entirely to the seeding mechanism.

WebDrake · 2017-02-15T20:50:24Z

Rebased on master, made the requested tweaks, and added a changelog entry.

Ddoc comments for private symbols have been replaced by comments

/*
   Like this
*/

so they can easily be re-ddoc'd in future if that's desirable. Multiline comments that will never be ddoc have been replaced by comments

/* Like
   this */

WebDrake · 2017-02-15T20:51:21Z

Overall changes to std.random:

diff --git a/std/random.d b/std/random.d
index b60101b..f3b1b45 100644
--- a/std/random.d
+++ b/std/random.d
@@ -570,47 +570,59 @@ Parameters for the generator.
     /// The default seed value.
     enum UIntType defaultSeed = 5489u;
 
-    /// Bitmasks used in the 'twist' part of the algorithm
-    private enum UIntType lowerMask = (cast(UIntType) 1u << r) - 1;
-    private enum UIntType upperMask = (~lowerMask) & this.max; // ditto
+    // Bitmasks used in the 'twist' part of the algorithm
+    private enum UIntType lowerMask = (cast(UIntType) 1u << r) - 1,
+                          upperMask = (~lowerMask) & this.max;
 
-    /// Collection of all state variables
-    /// used by the generator
+    /*
+       Collection of all state variables
+       used by the generator
+    */
     private struct State
     {
-        /// State array of the generator.  This
-        /// is iterated through backwards (from
-        /// last element to first), providing a
-        /// few extra compiler optimizations by
-        /// comparison to the forward iteration
-        /// used in most implementations.
+        /*
+           State array of the generator.  This
+           is iterated through backwards (from
+           last element to first), providing a
+           few extra compiler optimizations by
+           comparison to the forward iteration
+           used in most implementations.
+        */
         UIntType[n] data;
 
-        /// Cached copy of most recently updated
-        /// element of `data` state array, ready
-        /// to be tempered to generate next
-        /// `front` value
+        /*
+           Cached copy of most recently updated
+           element of `data` state array, ready
+           to be tempered to generate next
+           `front` value
+        */
         UIntType z;
 
-        /// Most recently generated random variate
+        /*
+           Most recently generated random variate
+        */
         UIntType front;
 
-        /// Index of the entry in the `data`
-        /// state array that will be twisted
-        /// in the next `popFront()` call
+        /*
+           Index of the entry in the `data`
+           state array that will be twisted
+           in the next `popFront()` call
+        */
         size_t index;
     }
 
-    /// State variables used by the generator;
-    /// initialized to values equivalent to
-    /// explicitly seeding the generator with
-    /// `defaultSeed`
+    /*
+       State variables used by the generator;
+       initialized to values equivalent to
+       explicitly seeding the generator with
+       `defaultSeed`
+    */
     private State state = defaultState();
-    // NOTE: the above is a workaround to ensure
-    // backwards compatibility with the original
-    // implementation, which permitted implicit
-    // construction.  With `@disable this();`
-    // it would not be necessary.
+    /* NOTE: the above is a workaround to ensure
+       backwards compatibility with the original
+       implementation, which permitted implicit
+       construction.  With `@disable this();`
+       it would not be necessary. */
 
 /**
    Constructs a MersenneTwisterEngine object.
@@ -652,13 +664,13 @@ Parameters for the generator.
     */
     private static void seedImpl(UIntType value, ref State mtState)
     {
-        mtState.data[$-1] = value;
+        mtState.data[$ - 1] = value;
         static if (this.max != UIntType.max)
         {
-            mtState.data[$-1] &= this.max;
+            mtState.data[$ - 1] &= this.max;
         }
 
-        foreach_reverse (size_t i, ref e; mtState.data[0 .. $-1])
+        foreach_reverse (size_t i, ref e; mtState.data[0 .. $ - 1])
         {
             e = f * (mtState.data[i + 1] ^ (mtState.data[i + 1] >> (w - 2))) + cast(UIntType)(n - (i + 1));
             static if (this.max != UIntType.max)
@@ -667,11 +679,11 @@ Parameters for the generator.
             }
         }
 
-        mtState.index = n-1;
+        mtState.index = n - 1;
 
-        // double popFront() to guarantee both `mtState.z`
-        // and `mtState.front` are derived from the newly
-        // set values in `mtState.data`
+        /* double popFront() to guarantee both `mtState.z`
+           and `mtState.front` are derived from the newly
+           set values in `mtState.data` */
         MersenneTwisterEngine.popFrontImpl(mtState);
         MersenneTwisterEngine.popFrontImpl(mtState);
     }
@@ -714,9 +726,9 @@ Parameters for the generator.
             throw new Exception(s);
         }
 
-        // double popFront() to guarantee both `mtState.z`
-        // and `mtState.front` are derived from the newly
-        // set values in `mtState.data`
+        /* double popFront() to guarantee both `mtState.z`
+           and `mtState.front` are derived from the newly
+           set values in `mtState.data` */
         MersenneTwisterEngine.popFrontImpl(mtState);
         MersenneTwisterEngine.popFrontImpl(mtState);
     }
@@ -729,22 +741,24 @@ Parameters for the generator.
         this.popFrontImpl(this.state);
     }
 
-    /// Internal implementation of `popFront()`, which
-    /// can be used with an arbitrary `State` instance
+    /*
+       Internal implementation of `popFront()`, which
+       can be used with an arbitrary `State` instance
+    */
     private static void popFrontImpl(ref State mtState)
     {
-        // This function blends two nominally independent
-        // processes: (i) calculation of the next random
-        // variate `mtState.front` from the cached previous
-        // `data` entry `z`, and (ii) updating the value
-        // of `data[index]` and `mtState.z` and advancing
-        // the `index` value to the next in sequence.
-        //
-        // By interweaving the steps involved in these
-        // procedures, rather than performing each of
-        // them separately in sequence, the variables
-        // are kept 'hot' in CPU registers, allowing
-        // for significantly faster performance.
+        /* This function blends two nominally independent
+           processes: (i) calculation of the next random
+           variate `mtState.front` from the cached previous
+           `data` entry `z`, and (ii) updating the value
+           of `data[index]` and `mtState.z` and advancing
+           the `index` value to the next in sequence.
+
+           By interweaving the steps involved in these
+           procedures, rather than performing each of
+           them separately in sequence, the variables
+           are kept 'hot' in CPU registers, allowing
+           for significantly faster performance. */
         sizediff_t index = mtState.index;
         sizediff_t next = index - 1;
         if (next < 0)
@@ -776,10 +790,10 @@ Parameters for the generator.
         mtState.z = mtState.data[index] = e;
         mtState.index = next;
 
-        // technically we should take the lowest `w`
-        // bits here, but if the tempering bitmasks
-        // `b` and `c` are set correctly, this should
-        // be unnecessary
+        /* technically we should take the lowest `w`
+           bits here, but if the tempering bitmasks
+           `b` and `c` are set correctly, this should
+           be unnecessary */
         mtState.front = z;
     }

WebDrake · 2017-02-15T22:07:06Z

Updated the changelog entry slightly to stress the breaking change w.r.t. the MersenneTwisterEngine template.

JackStouffer · 2017-02-16T15:23:16Z

Auto-merge toggled on

WebDrake · 2017-02-16T19:32:54Z

Thanks everyone! :-)

I realized that the changelog was missing one important note, which I've added in a follow-up PR: #5141

This was an oversight when finalizing dlang#5011.

WebDrake commented Jan 3, 2017

View reviewed changes

WebDrake force-pushed the mir-mt-update branch from 771e4ce to 82e2e83 Compare January 3, 2017 23:49

wilzbach mentioned this pull request Jan 4, 2017

[CircleCi]: use more fallbacks in case of a failure #5012

Merged

WebDrake commented Jan 4, 2017

View reviewed changes

JackStouffer added the Severity:Optimization label Jan 5, 2017

WebDrake force-pushed the mir-mt-update branch from fee72ed to 50b1b8c Compare January 6, 2017 00:53

WebDrake mentioned this pull request Jan 7, 2017

Update MersenneTwisterEngine template signature to match C++11 standard libmir/mir-random#14

Merged

WebDrake commented Jan 7, 2017

View reviewed changes

WebDrake force-pushed the mir-mt-update branch from 439a8b1 to 4bf64ae Compare January 7, 2017 22:03

JackStouffer added this to the 2.074.0 milestone Feb 8, 2017

WebDrake force-pushed the mir-mt-update branch from e455358 to f692517 Compare February 8, 2017 18:15

andralex approved these changes Feb 14, 2017

View reviewed changes

9il approved these changes Feb 14, 2017

View reviewed changes

WebDrake added 6 commits February 15, 2017 20:47

WebDrake force-pushed the mir-mt-update branch from f692517 to 84dc281 Compare February 15, 2017 20:47

Add changelog entry for MersenneTwisterEngine updates

36f1ec3

WebDrake force-pushed the mir-mt-update branch from 84dc281 to 36f1ec3 Compare February 15, 2017 22:06

JackStouffer merged commit 0562111 into dlang:master Feb 16, 2017

WebDrake deleted the mir-mt-update branch February 16, 2017 19:23

WebDrake mentioned this pull request Feb 16, 2017

Add a note about word size to the MersenneTwisterEngine changelog entry #5141

Merged

WebDrake added a commit to WebDrake/phobos that referenced this pull request Feb 16, 2017

Add a note about word size to the MersenneTwisterEngine changelog entry

c0d7271

This was an oversight when finalizing dlang#5011.

wilzbach pushed a commit to wilzbach/phobos that referenced this pull request Feb 22, 2017

Add a note about word size to the MersenneTwisterEngine changelog entry

7c537a0

This was an oversight when finalizing dlang#5011.

Uh oh!

Rework MersenneTwisterEngine using Ilya Yaroshenko's faster algorithm #5011

Rework MersenneTwisterEngine using Ilya Yaroshenko's faster algorithm #5011

Uh oh!

Conversation

WebDrake commented Jan 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WebDrake commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebDrake commented Jan 3, 2017

Uh oh!

WebDrake commented Jan 3, 2017

Uh oh!

wilzbach commented Jan 4, 2017

Uh oh!

WebDrake commented Jan 4, 2017

Uh oh!

9il commented Jan 4, 2017

Uh oh!

wilzbach commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebDrake commented Jan 4, 2017

Uh oh!

WebDrake commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WebDrake commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

9il commented Jan 6, 2017

Uh oh!

WebDrake commented Jan 7, 2017

Uh oh!

9il commented Jan 7, 2017

Uh oh!

WebDrake commented Jan 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Jan 7, 2017

Uh oh!

WebDrake commented Jan 7, 2017

Uh oh!

WebDrake commented Jan 7, 2017

Uh oh!

WebDrake commented Jan 17, 2017

Uh oh!

JackStouffer commented Jan 19, 2017

Uh oh!

WebDrake commented Feb 6, 2017

Uh oh!

WebDrake commented Feb 8, 2017

WebDrake commented Jan 3, 2017 •

edited

Loading

WebDrake commented Jan 6, 2017 •

edited

Loading