Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New select and Mutex algorithms #3020

Merged
merged 1 commit into from
Aug 4, 2022
Merged

New select and Mutex algorithms #3020

merged 1 commit into from
Aug 4, 2022

Conversation

ndkoval
Copy link
Member

@ndkoval ndkoval commented Nov 11, 2021

New select and Mutex algorithms

@ndkoval ndkoval changed the title Introduce new select implementation [DRAFT] Introduce new select implementation Nov 11, 2021
@ndkoval ndkoval marked this pull request as ready for review November 11, 2021 21:40
@ndkoval ndkoval force-pushed the new-channels-select branch 3 times, most recently from 927ebb6 to b9b8dc4 Compare December 6, 2021 13:01
@ndkoval ndkoval force-pushed the new-channels-select branch 2 times, most recently from f3c5772 to b2beac6 Compare December 16, 2021 12:00
@ndkoval ndkoval changed the base branch from new-channels-all to develop December 16, 2021 15:11
@ndkoval ndkoval changed the title [DRAFT] Introduce new select implementation New select and Mutex algorithms Jan 11, 2022
@ndkoval ndkoval requested a review from qwwdfsad January 11, 2022 22:21
kotlinx-coroutines-core/common/src/JobSupport.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/JobSupport.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
@ndkoval ndkoval requested a review from qwwdfsad January 31, 2022 22:36
kotlinx-coroutines-core/common/src/selects/Select.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/selects/OnTimeout.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/Semaphore.kt Outdated Show resolved Hide resolved
reactive/kotlinx-coroutines-reactive/src/Publish.kt Outdated Show resolved Hide resolved
@qwwdfsad
Copy link
Contributor

For the record, rebase on develop has rewritten the author of half of the commits, from a31621a to e38ca48

@qwwdfsad
Copy link
Contributor

qwwdfsad commented Jun 7, 2022

Also, ChannelUndeliveredElementSelectOldStressTest fails, please fix that as well, we would like to avoid having any flaking tests in the mainline

@ndkoval
Copy link
Member Author

ndkoval commented Jun 14, 2022

As for the flacking test, there was a problem in the test itself. Now the tests are green.

@qwwdfsad
Copy link
Contributor

TODO for me: cleanup documentation when it's the time

kotlinx-coroutines-core/api/kotlinx-coroutines-core.api Outdated Show resolved Hide resolved
@@ -592,7 +593,6 @@ public abstract interface class kotlinx/coroutines/channels/ActorScope : kotlinx

public final class kotlinx/coroutines/channels/ActorScope$DefaultImpls {
public static synthetic fun cancel (Lkotlinx/coroutines/channels/ActorScope;)V
public static fun getOnReceiveOrNull (Lkotlinx/coroutines/channels/ActorScope;)Lkotlinx/coroutines/selects/SelectClause1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, should be left as is

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's non-trivial to have a default implementation. I would suggest performing a cast to AbstractChannel and invoking onReceiveOrNull after that, throwing an exception if the channel implementation is not ours.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do an unchecked cast to AbstractChannel, we have discussed this in Slack

kotlinx-coroutines-core/common/src/selects/Select.kt Outdated Show resolved Hide resolved
*
* @suppress **This is unstable API and it is subject to change.**
* @suppress **This is unstable API, and it is subject to change.**
*/
@InternalCoroutinesApi // todo: sealed interface https://youtrack.jetbrains.com/issue/KT-22286
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will leave it as a documentation improvement

@ndkoval ndkoval requested a review from qwwdfsad July 18, 2022 12:00
Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please revisit SelectInstance being sealed as Oleg suggested

if (!_state.compareAndSet(NOT_SELECTED, pairSelectOp)) return@loop
val decision = pairSelectOp.perform(this)
if (decision !== null) return decision
fun trySelectDetailed(clauseObject: Any, result: Any?) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only use of this function is tryResumeSend which only cares about whether it's success or not (=> should rely on trySelect).

If you are not using this function in new channels, please remove it and all around it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation performs the following check, so it cannot be replaced with trySelect:

send.trySelectResult == REREGISTER

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

kotlinx-coroutines-core/common/src/selects/Select.kt Outdated Show resolved Hide resolved
@ndkoval ndkoval requested a review from qwwdfsad July 18, 2022 20:30
if (segment.cas(i, null, cont)) { // installed continuation successfully
cont.invokeOnCancellation(CancelSemaphoreAcquisitionHandler(segment, i).asHandler)
if (segment.cas(i, null, waiter)) { // installed continuation successfully
when (waiter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the code would be much cleaner if we introduced a private sealed interface to represent the possible types of waiters. This would cost us one additional application per slow-path acquire, but get rid of Any, impossible error, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you, but your suggestion requires either CancellableContinuation to implement this sealed interface or adding unsafe casts at every CancellableContinuation usage (in case onlyCancellableContinuationImpl implements the interface). I do not see a good solution, so I would keep the current code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dima, thanks, it's indeed worth doing. I'll try to do it separately in a branch with new channels, so the disturbance will be minimal. I have a prototype for selects only and it looks much more readable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to do something like this (pseudocode):

private sealed interface ResumableComputation {
  fun resume()
  fun disposeOnCancellation(handle: DisposableHandle)
}

private class CancellableContinuationAsResumable(
  val continuation: CancellableContinuation<Unit>
): ResumableComputation {
  ...
}

So, CancellableContinuation would not itself implement anything (though maybe this interface makes sense in general and it should), it would be wrapped in an adapter. The cost is one more allocation on the slow path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better not to do this. With #3084 and continuation reusability, the suspension becomes exceptionally efficient, as it does not allocate new objects. The change you suggest breaks this property, evolving extra work for GC.

kotlinx-coroutines-core/common/src/sync/Mutex.kt Outdated Show resolved Hide resolved
Copy link
Contributor

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address Dmitry's comments and it's good to go

@ndkoval ndkoval force-pushed the new-channels-select branch 2 times, most recently from c7eb4dc to f0c3f5a Compare August 1, 2022 15:47
see the documentation in `Select.kt` and `Mutex.kt` files for details
@ndkoval
Copy link
Member Author

ndkoval commented Aug 3, 2022

I went through Dmitry's comments and checked the ABI manually.

@qwwdfsad qwwdfsad merged commit feea6a5 into develop Aug 4, 2022
@hrach
Copy link
Contributor

hrach commented Aug 4, 2022

👏

qwwdfsad added a commit that referenced this pull request Feb 21, 2023
…block throws an exception

Otherwise, continuation instance is left in REUSABLE_CLAIMED state that asynchronous resumer awaits in an infinite spin-loop, potentially causing deadlock with 100% CPU consumption.
Originally, the bug was reproduced on old (pre-#3020) implementation where this very pattern was encountered: it was possible to fail owner's invariant check right in the supplied 'block'.
This is no longer the case, so the situation is emulated manually (but still is possible in production environments, e.g. when OOM is thrown).
Also, suspendCancellableCoroutineReusable is removed from obsolete BroadcastChannel implementation.

Fixes #3613
qwwdfsad added a commit that referenced this pull request Mar 3, 2023
#3634)

* Release reusability token when suspendCancellableCoroutineReusable's block throws an exception

Otherwise, the continuation instance is left in REUSABLE_CLAIMED state that asynchronous resumer awaits in an infinite spin-loop, potentially causing deadlock with 100% CPU consumption.
Originally, the bug was reproduced on an old (pre-#3020) implementation where this very pattern was encountered: it was possible to fail the owner's invariant check right in the supplied 'block'.
This is no longer the case, so the situation is emulated manually (but still is possible in production environments, e.g. when OOM is thrown).
Also, suspendCancellableCoroutineReusable is removed from obsolete BroadcastChannel implementation.

Fixes #3613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants