views:

4477

answers:

7

Let's say that a class has a public int counter field that is accessed by multiple threads. This int is only incremented or decremented.

To increment this field, which approach should be used, and why?

  • lock(this.locker) this.counter++;
  • Interlocked.Increment(ref this.counter);
  • Change the access modifier of counter to public volatile

Now that I've discovered volatile, I've been removing many lock statements and the use of Interlocked. But is there a reason not to do this?

+2  A: 

Interlocked functions do not lock. They are atomic, meaning that they can complete without the possibility of a context switch during increment. So there is no chance of deadlock or wait.

I would say that you should always prefer it to a lock and increment.

Volatile is useful if you need writes in one thread to be read in another, and if you want the optimizer to not reorder operations on a variable (because things are happening in another thread that the optimizer doesn't know about). It's an orthogonal choice to how you increment.

This is a really good article if you want to read more about lock-free code, and the right way to approach writing it

http://www.ddj.com/hpc-high-performance-computing/210604448

Lou Franco
+11  A: 

Using volatile won't help when you need to increment - because the read and the write are separate instructions. Another thread could change the value after you've read but before you write back.

Personally I almost always just lock - it's easier to get right in a way which is obviously right than either volatility or Interlocked.Increment. As far as I'm concerned, lock-free multi-threading is for real threading experts, of which I'm not one. If Joe Duffy and his team build nice libraries which will parallelise things without as much locking as something I'd build, that's fabulous, and I'll use it in a heartbeat - but when I'm doing the threading myself, I try to keep it simple.

Jon Skeet
+3  A: 

lock(...) works, but may block a thread, and could cause deadlock if other code is using the same locks in an incompatible way.

Interlocked.* is the correct way to do it ... much less overhead as modern CPUs support this as a primitive.

volatile on its own is not correct. A thread attempting to retrieve and then write back a modified value could still conflict with another thread doing the same.

Rob Walker
+12  A: 

"volatile" does not replace Interlocked.Increment! It just makes sure that the variable is not cached, but used directly.

Incrementing a variable requires actually three operations:

  1. read
  2. increment
  3. write

Interlocked.Increment performs all three parts as a single atomic operation.

Michael Damatov
+2  A: 

Read the Threading in C# reference. It covers the ins and outs of your question. Each of the three have different purposes and side effects.

spoulson
+48  A: 

Worst (won't actually work)

Change the access modifier of counter to public volatile

As other people have mentioned, this on it's own isn't actually safe at all. The point of volatile is that multiple threads running on multiple CPU's can and will cache data and re-order instructions.
If it's not volatile, and CPU A increments a value, then CPU B may not actually see that incremented value until some time later, which may cause problems.
If it is, this just ensures the 2 CPU's see the same data at the same time. It doesn't stop them at all from interleaving their reads and write operations which is the problem you are trying to avoid.

Second Best:

lock(this.locker) this.counter++;

This is safe to do (provided you remember to lock everywhere else that you access this.counter).
It prevents any other threads from executing any other code which is guarded by locker. Using locks also, prevents the multi-cpu reordering problems as above, which is great.

The problem is, locking is slow, and if you re-use the locker in some other place which is not really related then you can end up blocking your other threads for no reason.

Best

Interlocked.Increment(ref this.counter);

This is safe, as it effectively does the read, increment, and write in 'one hit' which can't be interrupted. Because of this it won't affect any other code and you don't need to remember to lock elsewhere either. It's also very fast (as MSDN says, on modern CPU's this is often literally a single CPU instruction).

I'm not entirely sure however if it gets around other CPU's reordering things, or if you also need to combine volatile with the increment.

Footnote: What volatile is actually good for.

As volatile doesn't prevent these kind of multithreading issues, what's it for? A good example is say you have 2 threads, one which always writes to a variable (say queueLength), and one which always reads from that same variable.

If queueLength is not volatile, thread A may write 5 times, but thread B may see those writes as being delayed (or even potentially in the wrong order).

A solution would be to lock, but you could also in this situation use volatile, which would ensure that thread B will always see the most up-to-date thing that thread A has written, which would be faster and result in cleaner code.

Orion Edwards
"I'm not entirely sure ... if you also need to combine volatile with the increment." They cannot be combined AFAIK, as we can't pass a volatile by ref. Great answer by the way.
Hosam Aly
Thanx much! Your footnote on "What volatile is actually good for" is what I was looking for and confirmed how I want to use volatile.
Jacques Bosch
+1: CLR via C# has a section on volatile which boils down to precisely this. The term for the situation that volatile solves is called cache coherency (http://en.wikipedia.org/wiki/Cache_coherence).
SnOrfus
A: 

I did some test to see how the theory actually works: kennethxu.blogspot.com/2009/05/interlocked-vs-monitor-performance.html. My test was more focused on CompareExchnage but the result for Increment is similar. Interlocked is not necessary faster in multi-cpu environment. Here is the test result for Increment on a 2 years old 16 CPU server. Bare in mind that the test also involves the safe read after increase, which is typical in real world.

D:\>InterlockVsMonitor.exe 16
Using 16 threads:
          InterlockAtomic.RunIncrement         (ns):   8355 Average,   8302 Minimal,   8409 Maxmial
    MonitorVolatileAtomic.RunIncrement         (ns):   7077 Average,   6843 Minimal,   7243 Maxmial

D:\>InterlockVsMonitor.exe 4
Using 4 threads:
          InterlockAtomic.RunIncrement         (ns):   4319 Average,   4319 Minimal,   4321 Maxmial
    MonitorVolatileAtomic.RunIncrement         (ns):    933 Average,    802 Minimal,   1018 Maxmial
Kenneth Xu