Combining Interlocked.Increment and Volatile.Write

Question

I've been looking at the source code of System.Reactive (here), and it's taken me down a rabbit hole to this place, where there is a Volatile.Read followed by Interlocked.CompareExchange, on the same variable:

if (Volatile.Read(ref _runDrainOnce) == 0
    && Interlocked.CompareExchange(ref _runDrainOnce, 1, 0) == 0)
{
    //do something
}

As I read it, the logic of this is "if runDrainOnce is 0 and, if it was zero before I changed it to 1". Is there something subtle here? Why is the first check not redundant?

(Even more mind-boggling, there is a lock and a Monitor.Pulse in the same function. Was this the result of a bet? :))

The whole function:

private void Schedule()
{
    // Schedule the suspending drain once
    if (Volatile.Read(ref _runDrainOnce) == 0
        && Interlocked.CompareExchange(ref _runDrainOnce, 1, 0) == 0)
    {
        _drainTask.Disposable = _scheduler.ScheduleLongRunning(this, DrainLongRunning);
    }

    // Indicate more work is to be done by the drain loop
    if (Interlocked.Increment(ref _wip) == 1L)
    {
        // resume the drain loop waiting on the guard
        lock (_suspendGuard)
        {
            Monitor.Pulse(_suspendGuard);
        }
    }
}

I would guess this is about the cost of fences. `Volatile.Read` uses a half fence (acquire) whilst `Interloceked.CompareExchange` is a full fence (acquire and release). It looks like they're trying to avoid the full fence in the case where `_runDrainOnce` isn't 0. — Sean, Nov 05 '20 at 10:16
According to Eric Lippert [here](https://stackoverflow.com/questions/2192124/reference-assignment-is-atomic-so-why-is-interlocked-exchangeref-object-object): "Interlocked.Exchange is written to expect a volatile field and do the right thing" which seems to mean that it's not necessary to do the volatile read first (assuming the same applies to CompareExchange()). I can't see the source, but I'd guess it does a volatile read equivalent inside the implementation. — Matthew Watson, Nov 05 '20 at 10:18

Marc Gravell · Accepted Answer · 2020-11-05T10:41:15.070

Entirely speculative possibility follows. In reality, I'd say that without explicit benchmarking, it probably doesn't matter, and we could simply lose the Volatile.Read test, or even just use a lock. If there was explicit benchmarking, I'd hope for a comment hinting (or linking) at that.

We can infer from the naming (_runDrainOnce) that we only expect this to succeed once, and if something is only going to succeed once, we really don't need the success case to be super optimal - so: having a redundant test in that success path: not a huge problem. In contrast, let's speculate that the failure scenario is called many many times, and so having it fail just doing an acquire-fenced read (without attempting to write) may be beneficial.

The Schedule code is invoked by everything - see OnCompleted, OnError, OnNext, etc - so presumably the intent is to just make sure that the scheduling gets started, as efficiently as possible - so it doesn't touch Intelocked more than necessary (once successfully, and possibly a few times indicating failure, if there is high thread competition initially)

You didn't explicitly ask, but the lock/Pulse is a common pattern for having an idle worker loop waiting on receiving work using Monitor; you need to wake it if it was likely idle, which is when the count was zero and is now non-zero (hence the Interlocked.Increment).

Thanks Marc. It's that phenomenon where there are just one too many words you don't quite understand which makes it so you don't understand the whole sentence. — Benjol, Nov 06 '20 at 10:35

Combining Interlocked.Increment and Volatile.Write

1 Answers1