Singleton SpinLock: Making Random Thread-Safe

Question

Is this a valid and safe use of .NET's System.Threading.SpinLock?

Why am I doing this?

Random's public methods are not thread-safe. I could be calling from any thread which I do not know until run-time; they are from the Thread Pool, and it could be called fairly frequently (up to a hundred times a second), so I would rather not create a new Random object each time.

public static class SingleRandom
{
    private static Random random;
    private static SpinLock spinLock;

    static SingleRandom()
    {
        random = new Random();
        spinLock = new SpinLock();
    }

    public static int Next()
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        int rv = random.Next();
        if(gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }

    public static int Next(int min, int max)
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        int rv = random.Next(min, max);
        if (gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }

    public static double NextDouble()
    {
        bool gotLock = false;
        spinLock.Enter(ref gotLock);
        double rv = random.NextDouble();
        if (gotLock)
            spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?
        return rv;
    }
}

Answer 1

1. SingleRandom is a really bad name for your class. Single means float in C# context, so your name implies that its a Random which generates float values, which is not true. ThreadSafeRandom or SynchronizedRandom are the examples of better naming.

2. Are you sure that using SpinLock in you case improves performance in any way? Somehow i think that a simple lock will be just as fast, while being a lot more readable.

   public static int Next()
   {
       lock (random)
       {
           return random.Next();
       }
   }
   

3. I am not sure that making your class static is the best way to go. In my experience in such cases its almost always better to make a non-static class and then to either inject its instance or initialize some static readonly field.

Answer 2

First, to re-iterate a point made in another answer:

Making a helper class which has internal state static is a bad idea. You lose a lot of flexibility, re-usability and increase testing pain and gain nothing from it.

The major problem with a static class is: You can't easily mock it for unit testing. If you want to unit test something which uses random numbers it's very helpful to be able to feed it a known sequence of random numbers but with a static class you start adding code which is just used for testing and your tests become brittle if you forget to reset the global state to a known point. Calling new SingleRandom() is hardly much work - if you consider that a problem then maybe an OO language is the wrong choice.

A static class with a state is effectively a singleton which is an anti-pattern. As such your question should not be "what are convincing reason to not use it" but rather "what are convincing reasons to use it".

My other major point is:

My very first thought when reading this was: Classic case of premature optimization.

• Heave you measured that generating the random numbers is the bottle neck?
• Have you measured that you need a spin lock?

I changed your implementation to use standard .NET locking:

public static class StandardLockSingleRandom
{
    private static Random random = new Random();
    private static object _lock = new object();

    public static int Next()
    {
        lock (_lock)
        {
            return random.Next();
        }
    }

    public static int Next(int min, int max)
    {
        lock (_lock)
        {
            return random.Next(min, max);
        }
    }

    public static double NextDouble()
    {
        lock (_lock)
        {
            return random.NextDouble();
        }
    }
}

This implementation has 30% less code than yours and the code which is there is considerably less complex. So your spin lock better stack up some nice performance.

Let's write some quick benchmark:

class Program
{
    static void Main(string[] args)
    {
        const int numRandomCalls = 100000;
        const int numTestLoops = 100;

        for (int p = 1; p <= 16; p <<= 1)
        {
            Console.WriteLine("=========================================================================================================");
            Console.WriteLine("Parallelism = {0}, Count of random numbers per iteration {1}", p, numRandomCalls);
            var parallelOptions = new ParallelOptions { MaxDegreeOfParallelism = p };
            RunAndMeasure("Spin Locked", numTestLoops, () => { Parallel.For(0, numRandomCalls, parallelOptions, (idx) => { SingleRandom.Next(); }); });
            RunAndMeasure("Standard Locked", numTestLoops, () => { Parallel.For(0, numRandomCalls, parallelOptions, (idx) => { StandardLockSingleRandom.Next(); }); });
        }

    }

    private static void RunAndMeasure(string name, int numLoops, Action act)
    {
        var stopWatch = new Stopwatch();
        stopWatch.Start();
        for (int i = 0; i < numLoops; ++i)
        {
            act();
        }
        stopWatch.Stop();
        var total = stopWatch.Elapsed.TotalMilliseconds;
        var perIter = total / numLoops;
        Console.WriteLine("{0}: # Test Loops = {1}, total time {2:.000}ms, {3:.000}ms per iteration", name, numLoops, total, perIter);
    }
}

Basically:

• Obtain 100,000 random numbers at varying degree of parallelism
• Run each test 100 times and build the average

On my machine which is an i7 with hyperthreading enabled (so going much beyond 8 in parallelism is probably not going to change much) results in this output:

=========================================================================================
Parallelism = 1, Count of random numbers per iteration 100000
Spin Locked: # Test Loops = 100, total time 996.986ms, 9.970ms per iteration
Standard Locked: # Test Loops = 100, total time 482.924ms, 4.829ms per iteration
=========================================================================================
Parallelism = 2, Count of random numbers per iteration 100000
Spin Locked: # Test Loops = 100, total time 1144.200ms, 11.442ms per iteration
Standard Locked: # Test Loops = 100, total time 560.377ms, 5.604ms per iteration
=========================================================================================
Parallelism = 4, Count of random numbers per iteration 100000
Spin Locked: # Test Loops = 100, total time 1253.103ms, 12.531ms per iteration
Standard Locked: # Test Loops = 100, total time 601.836ms, 6.018ms per iteration
=========================================================================================
Parallelism = 8, Count of random numbers per iteration 100000
Spin Locked: # Test Loops = 100, total time 1592.358ms, 15.924ms per iteration
Standard Locked: # Test Loops = 100, total time 802.485ms, 8.025ms per iteration
=========================================================================================
Parallelism = 16, Count of random numbers per iteration 100000
Spin Locked: # Test Loops = 100, total time 1603.059ms, 16.031ms per iteration
Standard Locked: # Test Loops = 100, total time 811.937ms, 8.119ms per iteration

So your spin lock implementation is considerably more complex and it takes twice the time.

What do we learn: Do not optimize things before you have measured them.

Answer 3

since you want thread safety you can just use a ThreadLocal<Random>

public static class SingleRandom
{
    private static ThreadLocal<Random> random;

    static SingleRandom()
    {
        random = new ThreadLocal<Random>(() =>
        {
            return new Random();
        });
    }

    public static int Next()
    {
        int rv = random.Value.Next();
        return rv;
    }

    public static int Next(int min, int max)
    {
        int rv = random.Value.Next(min, max);
        return rv;
    }

    public static double NextDouble()
    {
        double rv = random.Value.NextDouble();
        return rv;
    }
}

you can also explicitly define the seed for each Random by passing a Func to the ThreadLocal constructor

Answer 4

I'll be humbly honest, understanding exactly what useMemoryBarrier does is way, way beyond my current understanding of C#, multithreading and memory management, so I'll leave that part to other answerers.

I'm not sure this test is valid, but I ran it several times and it always passed - I started with 10, then 100, then 1000, then 10000 (failed a couple times with 10K iterations) - the test starts consistently failing with 100000 iterations, so a couple 100 times/second shouldn't be much of a problem.

// arrange
var results = new ConcurrentBag<int>();
var iterations = 1000;

// act
Parallel.For(0, iterations, o =>
{
    results.Add(SingleRandom.Next());
});

// assert
Assert.AreEqual(iterations, results.GroupBy(e => e).Count());

I ran the same test with some var rnd = new Random() and 1000 iterations failed very, very often. Bottom line, looks pretty safe to me.

Couple nitpicks:

private static Random random = new Random();
private static SpinLock spinLock = new SpinLock();

Statically initializing these static fields eliminates the need for a static constructor, provided that you're going to be using that SingleRandom type at one point.

if (gotLock) spinLock.Exit(false); // ASSUMPTION not IA64, TODO what if ARM?

I find this Exit(false) call is better off as a one-liner together with the gotLock condition. The alternative would be this:

if (gotLock)
{
    spinLock.Exit(false);
}

Building your if block on more than a single line without those {} curly braces isn't a recommended practice - everything that defines a scope, should be enclosed with curly braces; this isn't much more typing either:

if (gotLock) { spinLock.Exit(false); }

var gotLock = false;
var rv = random.Next();

Implicit typing, IMHO, makes more room for the significant code.

That's about all I can say here - I'll only add that I would have named rv as result.