6
\$\begingroup\$

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;
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Jon Skeet took care of Random and threading issues: msmvps.com/blogs/jon_skeet/archive/2009/11/04/… \$\endgroup\$ – Jesse C. Slicer Dec 3 '13 at 3:08
  • \$\begingroup\$ @JesseC.Slicer Ah-ha! Well now I have a bit of a clue of what I want to be testing! \$\endgroup\$ – Mathieu Guindon Dec 3 '13 at 3:10
  • \$\begingroup\$ Hmm his creates a Random for every thread, there could be many threads calling few times so I am not sure I would want that. \$\endgroup\$ – markmnl Dec 3 '13 at 4:00
  • 1
    \$\begingroup\$ why not use a thread local Random... \$\endgroup\$ – ratchet freak Dec 3 '13 at 9:22
  • 2
    \$\begingroup\$ A hundred times per second is not frequent. I benchmarked a simple lock based code, and it ran forty million times per second on an uncontended lock. With ThreadStatic it gets to ninety million per second per core. \$\endgroup\$ – CodesInChaos Dec 3 '13 at 10:03
6
\$\begingroup\$
  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.

\$\endgroup\$
  • \$\begingroup\$ 1. OK, 2. Not yet - and given the lock will mostly be uncontended I think you are right, 3. sounds like more work to me! \$\endgroup\$ – markmnl Dec 3 '13 at 8:07
  • \$\begingroup\$ @markmnl, 3. not nessesary, its just one line of extra code, which might save you a headache in a long run. The main disadvantage of static class is that it forces you to use single Random instance. Imagine that some other developer (or you yourself) decides to reuse your class in his segment of application. What will happen then? You will have his code slowing down your code, simply because you will share the same instance (and therefore the same lock). Non-static class does not have this problem - you will have different instances, unique to your contexts, but not globally. \$\endgroup\$ – Nikita B Dec 3 '13 at 8:40
  • \$\begingroup\$ in this case I offer nothing beyond Random, they can use their own Random, nor do I intend to additional functionality to my SingleRandom not in Random - it is a singleton after all \$\endgroup\$ – markmnl Dec 3 '13 at 9:00
  • \$\begingroup\$ @markmnl, it is a singleton, true, but only because you designed it that way. It does not have to be. "They can use their own random" is not how a developer should think (especisally since it is not "just Random", it is a synchronized wrapper). A developer should always identify re-usable logic and write code accordingly. Static class is not reusable, period. Its just my opinion though, you ofcause is free to do as you see fit. \$\endgroup\$ – Nikita B Dec 3 '13 at 10:32
  • \$\begingroup\$ This is going a bit off topic but re-use is not always desired. In this case I would rather have encapsulation and security as I don't want to allow another developer to have their own version purporting to be SingleRandom. \$\endgroup\$ – markmnl Dec 4 '13 at 6:53
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ For 1 million iterations the spin lock ran 100ms faster on mine (did you build with optimizations?). Nonetheless I agree it would be better to use lock. However I disagree with your first remark (or rather do not understand it) - it is dead easy to unit test a static class, and I have the added flexibility of calling it from anywhere in app without any initialization.. \$\endgroup\$ – markmnl Dec 3 '13 at 8:58
  • \$\begingroup\$ @markmnl: I built it in Release mode VS2010. Calling random 1,000,000 times rather than 100,000 times doesn't change anything for me except all tests take about 10x longer. Updated my answer regarding static. \$\endgroup\$ – ChrisWue Dec 3 '13 at 9:28
  • \$\begingroup\$ Perhaps the difference in our tests comes from you do nothing with the results of call to Random so the operation has been fully optimised away? \$\endgroup\$ – markmnl Mar 9 '14 at 10:14
  • \$\begingroup\$ @markmnl I seriously doubt that this would be an allowed optimization \$\endgroup\$ – ChrisWue Mar 9 '14 at 18:41
1
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ 1) You forgot the constructor call, your code will initialize random.Value to null 2) You could use a field initializer to simplify your code 3) The [ThreadStatic] is faster than ThreadLocal<T>. (factor 3 in a quick test) \$\endgroup\$ – CodesInChaos Dec 3 '13 at 10:14
  • 1
    \$\begingroup\$ 4) Explicit seeding is essential for this code. Your current code will fail if you spin up multiple threads at the same time (within a few milliseconds). All those threads will emit the same sequence. \$\endgroup\$ – CodesInChaos Dec 3 '13 at 10:33
  • \$\begingroup\$ @CodesInChaos ThreadStatic might be faster, but it means you have to deal with initialization yourself. (And it forces your to stay static, which other answers argued against.) \$\endgroup\$ – svick Dec 3 '13 at 18:52
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I wouldn't expect any errors ever - does ConcurrentBag<int> throw an error if you add value equal to one already in the bag? \$\endgroup\$ – markmnl Dec 3 '13 at 3:58
  • \$\begingroup\$ No, it's essentially a concurrent, unordered collection of T. I checked for existing values with the linq GroupBy in the assertion. \$\endgroup\$ – Mathieu Guindon Dec 3 '13 at 4:03
  • \$\begingroup\$ No it doesn't of course Random could eventually return the same number it has before - that would be why you assert failed more often the higher the iterations \$\endgroup\$ – markmnl Dec 3 '13 at 4:03
  • \$\begingroup\$ And the reason it failed when you create new ones is it is seeded with the current time - which will be the same if you create new ones right after one another - so they will return the same sequence \$\endgroup\$ – markmnl Dec 3 '13 at 4:04
  • 1
    \$\begingroup\$ My guts are telling me this test is utter BS - random != unique anyway... sorry I can't be more conclusive :/ \$\endgroup\$ – Mathieu Guindon Dec 3 '13 at 4:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.