Using Random in Parallel.For

Question

My program performs many simulations, each of which calls for many random numbers to be generated. The serial method is straightforward and works. However, in my pursuit of parallelizing the work I believe I created a more straightforward method than what I could find. The other methods are somewhat dated and something might be possible now that wasn't possible then.

Am I missing something that will make my method susceptible to any of the myriad of multithreading problems? My method uses the ability of a Parallel.For to instantiate a variable for individual thread use and thus it doesn't require another class like the other methods I found. In this case each thread gets its own Random.

Timing:

My method: 4s

Stephen: 14s

Jon: 16s

Clearly I don't know as much as Stephen or Jon so I'm concerned I missed something.

My method:

Random rnd = new Random();
int trials = 1_000_000;

private readonly object globalLock = new object();
ParallelOptions po = new ParallelOptions();
po.MaxDegreeOfParallelism = 4;

await Task.Run(() =>
{
    Parallel.For<Random>(0, trials, po, 
    () => { lock(globalLock){ return new Random(rnd.Next()); } }, 
    (i, loop, local) =>
    {
        for (int work = 0; work < 1000; work++)
        {
            local.Next();
        }

        return local;
    },
        (x) => { }
    );
});

This next method is by Stephen Toub on the MSDN Blog:

public static class RandomGen2
{
    private static Random _global = new Random();
    [ThreadStatic]
    private static Random _local;

    public static int Next()
    {
        Random inst = _local;
        if (inst == null)
        {
            int seed;
            lock (_global) seed = _global.Next();
            _local = inst = new Random(seed);
        }
        return inst.Next();
     }
}

await Task.Run(() =>
{
    Parallel.For(0, trials, i =>
    {
        for (int work = 0; work < 1000; work++)
        {
            RandomGen2.Next();
        }
    });

});

This next method is by Jon Skeet on his blog:

public static class ThreadLocalRandom
{
    private static readonly Random globalRandom = new Random();
    private static readonly object globalLock = new object();

    private static readonly ThreadLocal<Random> threadRandom = new ThreadLocal<Random>(NewRandom);

    public static Random NewRandom()
    {
        lock (globalLock)
        {
            return new Random(globalRandom.Next());
        }
    }

    public static Random Instance { get { return threadRandom.Value; } }

    public static int Next()
    {
        return Instance.Next();
    }
}

await Task.Run(() =>
{
    Parallel.For(0, trials, i =>
    {
        for (int work = 0; work < 1000; work++)
        {
            ThreadLocalRandom.Instance.Next();
        }
    });
});

Update/answer: Brian has pointed out that I was using Jon's method incorrectly. A more correct way would be to call an ThreadLocalRandom.Instance for each Parallel.For loop and use that instance for the internal for loop. This prevents the thread check on each call and instead there is only one thread check per Parallel.For loop. Using Jon's method correctly makes his method faster than the overload of Parallel.For that I was using.

Answer 1

However, in my pursuit of parallelizing the work I believe I found a more straightforward method than what I could find.

It's more straightforward, but wrong.

The other methods are somewhat dated.

What on earth does this even mean?

Am I missing something that will make my method susceptible to any of the myriad of multithreading problems?

The most basic rule of thread safety is: you can't use a not-threadsafe object on multiple threads without a lock. Random is not threadsafe, but you use the same one on every thread to calculate the seed.

Note that Jon and Stephen's "dated" methods correctly lock around the seeding random.

Clearly I don't know as much as Stephen or Jon so I'm concerned I missed something.

First, you should thoroughly internalize the basic rules of thread safety before writing any more multithreaded code.

Second, your attitude was your error. The correct attitude is: Jon and Stephen are both experts and their solutions contain no unnecessary parts. If you think that you've found a solution that lacks a part that their solutions have, then you need to explain why your solution does not need the part that their solution has.

Answer 2

Your code is faster because it is simpler. Your code gives each loop a dedicated instance of Random. Jon and Stephen's code do so as well, but in their code every access to Random must check which thread is in use and then pull the correct instance of Random. Stephen's code is faster than Jon's code because ThreadLocal (which is a wrapper around ThreadStatic) is slightly slower.

However, the nice thing about their code is that their code provides a simple replacement for Random. Your approach puts the responsibility on the parallel code to initialize Random. In a real-world problem, carrying an instance of Random across the various supporting functions is kind of a hassle compared to having a static, thread-safe Random service.

In a real-world tasks, your functionality is probably not dominated by calls to Random. So under normal circumstances the slight performance loss from their code is fine.

I recommend ThreadLocal<T> over ThreadStatic (see ThreadStatic v.s. ThreadLocal<T>: is generic better than attribute? for discussion).

By the way, please never use lock with anything except a dedicated lock object. Like Jon (https://codeblog.jonskeet.uk/2008/12/05/redesigning-system-object-java-lang-object/), I really wish lock did not even support arbitrary objects.

---

Update:
.NET 6 adds Random.Shared, which "provides a thread-safe Random instance that may be used concurrently from any thread. " For new code, Random.Shared offers a simplified way to use Random in a thread-safe manner.