What is the reason for Task.IsCompleted to use cached flags?

Question

I was looking at some of the implementation details of Task in System.Threading.Tasks (.NET standard 2.0) and I came across this interesting piece of code:

internal volatile int m_stateFlags;

...

public bool IsCompleted
{
  get
  {
    int stateFlags = m_stateFlags; // enable inlining of IsCompletedMethod by "cast"ing away the volatiliy
    return IsCompletedMethod(stateFlags);
  }
}

// Similar to IsCompleted property, but allows for the use of a cached flags value
// rather than reading the volatile m_stateFlags field.
private static bool IsCompletedMethod(int flags)
{
  return (flags & TASK_STATE_COMPLETED_MASK) != 0;
}

I understand from reading C# reference guide that volatile is there to prevent compiler/runtime/hardware optimizations that would result in re-ordering of read/write to the field. Why would in this specific case specify the field as volatile just to ignore volatility when reading the field by assigning it to a variable?

It seems intended but the reasoning behind the intent is unclear to me. Also, is "cast"ing away the volatilty a common practice? In which scenarios would I want to do it vs. scenarios I absolutely want to avoid it?

Any information to help me understand this bit of code clearer is greatly appreciated.

Thanks,

Dissonant

It is very naughty code, surely written before .NET 4.0's Volatile.Read() was stable. Which was a bug fix for Thread.VolatileRead(), which mistakenly takes a full memory barrier. Jitters give the volatile keyword extra semantics on processors with a weak memory model (arm, itanium), gives a read acquire semantics. Doesn't make sense in this property getter, but does in the other two places it is used. Same for OptionsMethod() btw. Short from the high cost, it also avoid races when the flags are tested multiple times. Consistency is a good thing. – Seve

Answer 1

volatile is a syntax sugar of Volatile.Read and Volatile.Write. It ensures that any CPUs will see the same data at the same time.

So it makes only one read of the volatile variable. Then this may be used multiple times in property, method (losing the volatile nature). So it looks like it takes a snapshot of the variable and do several computations on its value.

internal volatile int m_stateFlags;

public bool IsCompleted
{
  get
  {
      int stateFlags = m_stateFlags;
      return IsCompletedMethod(stateFlags);
  }
}

Parricide

You'd need to take care using this technique with reference types, if at all – Nidus

Answer 2

So, some time ago JIT didn't know how to inline methods if any of their actual arguments is volatile. But later it was fixed in a PR: Allow inlining with volatile actual argument exprs #7332

That local copy of m_stateFlags was needed for a performance reason: a method call is quite expensive and JIT was not able to inline that call.

So, now you can create a PR and remove that redundant copy because it's still there and the fix is there as well!

Staggs

Answer 3

Some more comments from .NET Framework Reference Source:

"cast away" volatility to enable inlining of OptionsMethod.

Read the volatile m_stateFlags field once and cache it for subsequent operations.

Get a cached copy of the state flags. This should help us to get a consistent view of the flags if they are changing during the execution of this method.

So as you figured out already, it's apparently used there for three things:

1. enable inlining of certain methods (apparently accessing volatile field blocks inlining?),
2. reduce amount of volatile read calls (it might be just done purely for the third reason though),
3. ensure that the value has not changed during execution of this method (e.g. from a different thread).

Now onto your questions.

Why would in this specific case specify the field as volatile just to ignore volatility when reading the field by assigning it to a variable?

Task.m_stateFlags is obviously intended to be updated as instantly as possible, from any thread. IsCompletedMethod is called from multiple places (e.g. Start(TaskScheduler scheduler)), where you wouldn't like the m_stateFlags to change value in the middle of method execution. IsCompleted looks like just a slick "wrapper" for the IsCompletedMethod.

Is "cast"ing away the volatilty a common practice? In which scenarios would I want to do it vs. scenarios I absolutely want to avoid it?

I do not have a lot of experience with it, neither seen lot of usages, but based on link given by the @StepUp and the Task usages - you'd want to make your field volatile if you want to keep it synced between processors and threads.

That sounds pretty similar to Interlocked and lock() use cases, doesn't it?

Panatella