4.x: Improve the logic of AsyncLock

[akarnokd]

This PR improves the AsyncLock class' logic:

• Avoid the cost of allocating a queue upfront in case there is no actual concurrency involved.
• Clearing that queue is more efficient by setting the reference to null.
• Enqueueing an Action when the "lock" is not held should not go through the queue because it can be executed immediately.
• Changed the lock to this (as queue can now be null). I've seen other places with guard = new object() but that's an allocation and this is available. Not sure what the policy is given that AsyncLock is public. If it was internal or have such copy, I'm sure such shortcuts could be safely taken.

[danielcweber]

Never ever lock on "this"! Code using the AsyncLock in a private field might use it as a lock as well and we're gonna run into some serious trouble.

[akarnokd]

That's why I noted it in the PR text. Adding a guard = new object() resolves this with the obvious added cost of allocation. If there was an internal version of this lock and the discipline to not lock on it by Rx code, that would be better. I'll update the PR to use a guard.

[danielcweber]

Some small review, see it there. Otherwise, I think work on AsyncLock is greatly appreciated. I had a look into this before and was considering an overload that takes an Action<T> and a state of T, to save more allocations of closures throughout the code. What do you think?

[danielcweber]

We're in a lock, why the local variable?

[akarnokd]

A habit from lock-free programming: touch fields as less as possible around atomic accesses.

[danielcweber]

I don't think there's much to gain, it just makes the code less readable.

[akarnokd]

I had a look into this before and was considering an overload that takes an Action and a state of T, to save more allocations of closures throughout the code. What do you think?

The best would be not to use AsyncLock at all. It is a general purpose trampolining mechanism which could be inlined in a completely lock-free fashion most of the time.

[danielcweber]

How complex would the inline be? Should it really be repeated throughout the code?

[akarnokd]

Something along the lines of #499, but the general structure is something like this:

void Trampoline<TState>(ref int wip, ConcurrentQueue<TState> queue, 
        TState item, Action<TState> genericBody) 
{
    if (Interlocked.CompareExchange(ref wip, 1, 0) == 0) {
        genericBody(item);
        if (Interlocked.Decrement(ref wip) == 0) {
            return;
        }
    }
    else 
    {
        queue.Enqueue(item);
        if (Interlocked.Increment(ref wip) != 1) {
           return;
        }
    }
    var missed = 1;
    for (;;) {
        if (queue.TryDequeue(out var v)) {
            genericBody(v);
            continue;
        }

        missed = Interlocked.Add(ref wip, -missed);
        if (missed == 0) {
            break;
        }
    }
}

where TState should contain all relevant information so that genericBody could be a static Action and not a per-invocation allocated delegate. Note though that the contents of for(;;) can be something quite complicated beyond reading the contents of a queue: see this and this.

[danielcweber]

Ok, but you wouldn't inline that throughout the whole codebase, would you?

[akarnokd]

I did in RxJava a lot because there are no structs or refs to support a generic function like the example above. The second reason is that one can never know if the JIT will actually inline the generic function or not, making it less efficient due to TState and Action<TState> having different types at different locations resulting in a so-called megamorphic dispatch.

would you?

Yes I would specialize such trampolining for each operator I upgrade.

[danielcweber]

Yes I would specialize such trampolining for each operator I upgrade.

Why does it need to be specialized for every operator ?

[akarnokd]

If we want optimized operators, they have to be adapted to the special circumstances.

[danielcweber]

Let's discuss then if you really see the need to inline trampolining for some operator. AsyncLock may not be the prettiest thing but I think with a overload that takes Action<T> and state T we can get rid of a lot of closures and delegate allocations.

[akarnokd]

AsyncLock is a public API class, right? Changing its API surface would break users outside the library. You should be introducing a new class with the desired properties.

[danielcweber]

You can always add an overload.

[akarnokd]

You can always add an overload.

But then the internal structure has to accomodate for a TState object: Queue<TStateAndAction> struct TStateAndAction { TState state, Action action } for every use case, wether or not it is required. Plus, dequeueing a struct is more involved as the whole outer struct has to be defaulted to cleanup all possible references.

[danielcweber]

Are we going for this ?

[akarnokd]

Not in this PR.

[danielcweber]

Ok