3

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#?

  • Why is the counter "frequently polled" ? If you need to trigger an one-off task as soon as the counter goes to zero, you can just check the return value from Interlocked.Decrement(); if it is zero, call a callback function (or delegate). – rwong Jul 30 '14 at 11:47
  • Other advice will depend on: Is the value initialized to a positive value in a thread-safe manner? Aside from decrements, will there be any other changes to the value? Will there be a possibility of overshoot? (i.e. going negative, which may happen if either (1) there are more decrement operations than the initial count, or (2) some decrement operations decrease the value by more than one) What is the purpose of the "frequent polling", given it is a cause for inefficiency? Why isn't an alternative used? – rwong Jul 30 '14 at 11:50
  • 2
    See: CountdownEvent class (since .NET 4.0), which is probably based on Win32 HANDLE. – rwong Jul 30 '14 at 11:53
  • 2
    My personal choice is actually to wrap all tasks ("threads" in your words) as Task, and use Task.WhenAll(Task[]) – rwong Jul 30 '14 at 13:03
  • 1
    To my knowledge the interlocked increment/decrement operations in .NET are atomic, so you don't need to do anything other than use them, in whatever fashion you like from any thread you want.... you don't need to use any wrapping – Jimmy Hoffa Jul 31 '14 at 2:48
2

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:

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

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.

  • 1
    That code is not atomic. Assume: T1 : decrements value, which ends up 0, goes into if, but doesn't write yet. T2: decrements, gets -1, doesn't enter if, goes to IsDone, gets false. T1: Finishes setting done to true. Maybe <= 0 instead of == 0 could work. – Euphoric Jul 30 '14 at 13:01
  • @Euphoric: According to OP's comments, OP takes responsibility for making sure the number of calls to Decrement() always match up the initial count (by way of initializing exactly that number of threads/workers/tasks and nothing more). But since it's a small change, I have now edited it into my answer. Thanks for your suggestion. – rwong Jul 30 '14 at 13:05
0

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;
  }
}
  • Well yeah obviously two independent function calls are not atomic if put together. You still call a concurrent hashmap atomic even if clearly a put and get call are only independently atomic. – Voo Jul 30 '14 at 11:56
  • @voo - well if your goal is to not get below 0... – Telastyn Jul 30 '14 at 11:57
  • Assume you have a counter that is initialized with n and n threads that sometime during their liftetime call Decrement but only once. The invariant holds just fine without any race condition. – Voo Jul 30 '14 at 11:59
  • @voo - then just use a barrier or the basic wait handles... – Telastyn Jul 30 '14 at 12:44
0

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.

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.