Implementation strategies for atomic counter

Question

I basically have a counter variable that is decremented and frequently polled. There are three options to implement that, that I can think of:

Number 1:

private volatile int count;
public void Finished() {
    Interlocked.Decrement(ref count);
}

public bool Done() {
    return count == 0;
}

Number 2:

private int count;
public void Finished() {
    Interlocked.Decrement(ref count);
}

public bool Done() {
    return Volatile.Read(ref count) == 0;
}

Number 3:

struct AtomicInteger {
    private int value;

    public int Get() {
        return Volatile.Read(ref value);
    }
    // other methods
}
// In actual class:
private AtomicInteger count;
public bool Done() {
     return count.Get() == 0;
}

The first one gives a compiler warning because of passing a volatile variable by reference. On the other hand it's clear from the variable definition that the variable is accessed concurrently which is clearly a warning sign to anybody looking at the code. If I ever decide that I actually need a long, this method does not work at all any more.

The second one doesn't give any warnings, but just from reading private int count it's not clear that the variable is used concurrently, so this has to be explicitly stated with a comment (I actually had a bug where I used a normal read in an assertion instead of Volatile.Read - such an easy thing to overlook.) Changing the variable to a long on the other hand does not introduce any additional work.

I do like number 3 and it shouldn't result in any performance overhead compared to the other solutions after inlining. Downside: Code duplication for every primitive. On the other hand the code is simple enough and can be copy-pasted without fear of problems and is unlikely to ever change.

Any comments on advantages/disadvantages of each method that I've overlooked or what would be considered idiomatic in C#?

Answer 1

Given that thread-safety is a very tricky issue, I can't comment on the correctness of this code. If you spot an error, just let me know and it will edit or delete my answer.

As an overview of the purpose of the various memory semantics modifiers in C#, please see:

• SO: Volatile vs. Interlocked vs. lock
• Note: do not take the accepted answer at face-value. Read the reference links (especially from the authors of C# specification and framework) and decide for yourself.

Also recommended:

• Eric Lippert's Blog at MSDN Atomicity, volatility and immutability are different, part three

Also, Joe Duffy's Volatile class, which is part of .NET 4.5

• Sayonara volatile

Also, note the difference between "ought to be" vs. "the way it is / will be implemented". Pragmatically, only the latter matters.

---

Based on feedback from @Euphoric, it appears that OP's Number 2 is indeed the solution that will satisfy OP's requirement.

Wrapping of the code into a class (i.e. Number 3) is a matter of coding style and refactoring.

Answer 2

None of those are actually atomic. The idiomatic approach would be something like

class Countdown {
  private int count;

  public Countdown(int ct) {
    count = ct; 
  }

 public bool Do(Action stuff) { // or whatever good name makes sense; possibly Func<T> returning T not bool
     var isDone = Interlocked.Decrement(ref count) == 0;
     if (!isDone) {
          // possibly try/catch
          stuff();
          return true;
     }

     return false;
  }
}

Answer 3

I would say that number one is idiomatic in C#.

Personally I prefer number two: I like to move the "volatile read" information on the read itself rather than bury it in the fields declaration. Besides the volatile modifier in c# is a strange and misleading thing that is often misunderstood, so I think it is easier for a newbie to just read and understand the Volatile.Read documentation. Finally it is a healthy habit for the more complex cases where you must emphasize the implicit memory fences.

As for number three, it is a matter of coding style. Personally I think it is superfluous unless you use it in many places.

Finally, for those worried about correctness, here is a source (Joe Duffy) that states that the volatile modifier is not needed on fields for mere atomic operations, which means that #1 and #2 are equivalent and correct, and therefore that #3 is. Nevertheless I would replace "== 0" by "<= 0" just in case something goes wrong in two years.