Issue 246 continuations wiring (II)

[ThiagoT1]

Important:

1. Sync methods behavior and API are unchanged
2. The only flow control changed is ReadAsync. Other async methods are unchanged.

Properly Wire ReadAsync Continuations.

Every commit tries to stage a step towards the PR objective.

The main point of concern is the RelaxedCPR treatment on cs/src/core/Index/FASTER/FASTER.cs

Fixes #246

[ThiagoT1]

Last commit done in attempt to meet FASTER concurrency model requirements.

All tests passing, including comprehensive ones on dependent libs, like FasterDictionary:

[badrishc]

I am worried about the depth of changes in this PR as they break subtle invariants in the system. We will step back and (1) fix the multi-async bug; (2) make sure your use case gets high performance. I think these goals can be accomplished without modifying the mono-threaded session access invariant.

[badrishc]

Also, FASTER is a latch-free library. We try to not take locks anywhere, except in one case during checkpointing, when forcing a session to move from one version to another. This is achieved by ensuring that only the mono-threaded session accesses/updates any state.

Thus, we need the async continuations to simply be added to the async response queue (which already uses a latch-free ConcurrentQueue implementation), from which the mono-threaded session will remove elements.

[badrishc]

What if the user does RMWAsync followed by ReadAsync with waitForCommit true. We would require the older RMW to complete as well, as part of this wait for commit.

[badrishc]

We do not want to take the overhead of locking the client session. This is the reason FASTER is mono-threaded. All work for a session is done on a single thread, and this PR breaks that contract.

[ThiagoT1]

What if the user does RMWAsync followed by ReadAsync with waitForCommit true. We would require the older RMW to complete as well, as part of this wait for commit.

That should be ensured by ClientSession.WaitForCommitAsync (that went unchanged), no?

[badrishc]

If user does this sequence, the expectation is that all operations until that point are committed:

s.RMWAsync();
await s.ReadAsync(waitForCommit: true);

[ThiagoT1]

We do not want to take the overhead of locking the client session. This is the reason FASTER is mono-threaded. All work for a session is done on a single thread, and this PR breaks that contract.

I see.

However, currently there's other places where there's shared memory, which current synchronization techniques are equivalent to locks, even with the possibility of a context switch. Those are ConcurrentDictionaries and ConcurrentQueues.

Would that be better if we switched the lock to Interlocked calls and a Spinner?

[ThiagoT1]

If user does this sequence, the expectation is that all operations until that point are committed:

s.RMWAsync();
await s.ReadAsync(waitForCommit: true);

This is happening.
RMWAsync will put an entry inside the pending requests collection, that in turn will be monitored by the CompletePendingAsync call inside WaitForCommitAsync

[ThiagoT1]

This is achieved by ensuring that only the mono-threaded session accesses/updates any state.

if this is about FASTER.cs:389 lock (clientSession.ctx), it was added since Native32.Read callbacks do not get executed in a mono-threaded way. And the continuation in the PR is following the same execution path.
Thus, such code will also be executed in a multi threaded way, hence the lock, that by the way also happens inside ConcurrentQuere, albeit by way of Interlocked methods and spinners.

Thus, we need the async continuations to simply be added to the async response queue (which already uses a latch-free ConcurrentQueue implementation), from which the mono-threaded session will remove elements.

This would have ate least these consequences:

1. The AsyncQueue only releases a single awaiter to go, every time an enqueue happens, so
2. Even if this awaiter completed someone elses continuation and stored it somewhere, the actual awaiter would still be hung on DequeueAsync (maybe forever)
3. To counter that, we would need a Timeout (lots of allocation, bcz its a task array with two tasks in it, at least)
4. other option would be to Re-Enqueue the continuation on AsyncQueue. Then we could have endless racing between several awaiters, plus a new Task and StateMachine every loop.

In the end, we would need to use other type of queue, that would signal (by a task) every awaiter, so they can all just discard a continuation thats not theirs and re await for the next completion, until theirs come around, and they can return.

That would produce a task for every completion multiplied by maybe a new state machine for every awaiter loop interation.

[badrishc]

This loop is not identical in behavior to the original code. For example, I notice that InternalRefresh would not be called in this tight loop, resulting in never getting out of CPR. The debug asserts for version correctness are also skipped. We need to ensure that all code paths are functionally identical to the older case.

Why not use an async equivalent of HandleOperationStatus? This keeps the main FasterKV.ReadAsync method small so it can be inlined, leaving the complexity of uncommon cases to a sub-method.

[ThiagoT1]

This loop is not identical in behavior to the original code. For example, I notice that InternalRefresh would not be called in this tight loop, resulting in never getting out of CPR. The debug asserts for version correctness are also skipped. We need to ensure that all code paths are functionally identical to the older case.

You're right. Thank you! The last commit just fixed this.

Why not use an async equivalent of HandleOperationStatus? This keeps the main FasterKV.ReadAsync method small so it can be inlined, leaving the complexity of uncommon cases to a sub-method.

It was done this way because HandleOperationStatus has a lot of cases to thread.

Actually, any status different from Success or NotFound will cause it to be called.
It will even trigger the disk IO if needed.

That's where we need to fork paths.

When the disk IO is needed, the ReadAsync path will do the new wiring, thus we cannot call HandleOperationStatus at this point, and should just prepare for the await.

It's important to notice that the only changed path was the ReadAsync one.

[ThiagoT1]

Btw, current code will error out the Read if 2 consecutive CPR_SHIFT_DETECTED happen.

Should we keep this? I think this could be in error.

What is keeping the overall state to be in CPR_SHIFT_DETECTED twice, while being on other states in between the two tries, in a classic ABA Problem?

[ThiagoT1]

Yesterday's comments handled.

[ThiagoT1]

This loop is not identical in behavior to the original code. For example, I notice that InternalRefresh would not be called in this tight loop, resulting in never getting out of CPR. The debug asserts for version correctness are also skipped. We need to ensure that all code paths are functionally identical to the older case.

You're right. Thank you! The last commit just fixed this.

Why not use an async equivalent of HandleOperationStatus? This keeps the main FasterKV.ReadAsync method small so it can be inlined, leaving the complexity of uncommon cases to a sub-method.

It was done this way because HandleOperationStatus has a lot of cases to thread.

Actually, any status different from Success or NotFound will cause it to be called.
It will even trigger the disk IO if needed.

That's where we need to fork paths.

When the disk IO is needed, the ReadAsync path will do the new wiring, thus we cannot call HandleOperationStatus at this point, and should just prepare for the await.

It's important to notice that the only changed path was the ReadAsync one.

[ThiagoT1]

Btw, current code will error out the Read if 2 consecutive CPR_SHIFT_DETECTED happen.

Should we keep this? I think this could be in error.

What is keeping the overall state to be in CPR_SHIFT_DETECTED twice, while being on other states in between the two tries, in a classic ABA Problem?

[ThiagoT1]

As per last discussions, a new struct is wrapping the compute completion logic, so the user can trigger it.
The completion lock is gone and the user must call ReadAsyncResult.CompleteRead in a single threaded manner.

[ThiagoT1]

BTW,

Thank you @badrishc for the patience and the high level discussions about this issue.

[badrishc]

Is there a good reason to use all the complicated logic in FasterAsyncOperation vs a TaskCompletionSource that we set completed when IO is done?

[badrishc]

We should avoid introducing new code if we can avoid it.

[ThiagoT1]

Yes.
TCS will yield a task (class) while our FAO will yield and back a ValueTask (struct).
Its one allocation less.

Lots of code inside .net core itself are using IValueTaskSource as is FasterAsyncOperation, as to avoid allocating a Task when possible.