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

CancellableQueueSynchronizer and ReadWriteMutex #2045

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

ndkoval
Copy link
Member

@ndkoval ndkoval commented May 21, 2020

Introduce SegmentQueueSynchronizer abstraction for synchronization primitives and ReadWriteMutex. Please follow the provided documentation and the test suite for details.

@ndkoval ndkoval requested a review from dkhalanskyjb May 21, 2020 15:10
@ndkoval ndkoval changed the base branch from master to develop May 21, 2020 15:10
@ndkoval ndkoval changed the title [DRAFT] SegmentQueueSynchronizer SegmentQueueSynchronizer Jul 4, 2020
@ndkoval ndkoval force-pushed the segment-queue-synchronizer branch 2 times, most recently from 51dc99b to a456122 Compare July 5, 2020 17:13
@ndkoval ndkoval requested a review from elizarov July 5, 2020 17:13
@ndkoval ndkoval force-pushed the segment-queue-synchronizer branch from a456122 to a677bbc Compare July 5, 2020 19:32
@ndkoval ndkoval marked this pull request as ready for review July 5, 2020 19:49
@ndkoval
Copy link
Member Author

ndkoval commented Jul 6, 2020

Here is a link to a successful nightly build with these changes: https://teamcity.jetbrains.com/buildConfiguration/KotlinTools_KotlinxCoroutines_NightlyStress/3009472

arrived.loop { cur ->
// Are we going to be resumed?
// The resumption permit should be refused in this case.
if (cur == parties.toLong()) return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

A small problem here. I believe this should be cur <= parties.toLong(), but I think it's interesting why so and, more importantly, why it didn't cause any problems.

parties - 1 calls happen and go to sleep; then comes the call that is about to wake everyone up but doesn't yet do anything after incrementing the counter; then, 1000 new calls happen, and a == parties + 1000. Then, every thread is cancelled, performs onCancellation and, since arrived is much bigger than parties.toLong(), successfully decrements arrived. Then the resumer comes, sees parties - 1 CANCELLED marks and proceeds to wake up parties - 1 more cells.

This works only because this behavior, while not optimal, doesn't break anything: after the cells are all woken up, this SQS is never used again. However, if resumeMode was set to SYNC, everything would hang in this scenario, but it seems that with the change above it would work fine in this case too.

It should be noted though that without the check for cur == parties.toLong() the code would not be linearizable: as is, it guarantees that once the barrier is broken, it can't be accidentally "un-broken" no matter how many cancellations happen, and so the subsequent calls to arrive all return false in any case.

Copy link
Member Author

@ndkoval ndkoval Dec 22, 2020

Choose a reason for hiding this comment

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

It should be either >= or ==. In my opinion, it is always better to decrement the counter if possible, so that the number of resume invocations is decreased. We should also add a check that the barrier is completed at the beginning of arrive. Thus, the last arrive() works in O(T+K) where K is the number of parties and T is the number of threads (not coroutines).

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

Comments about the first comment.

@ndkoval ndkoval force-pushed the segment-queue-synchronizer branch 2 times, most recently from 8c364c9 to 8137b68 Compare October 23, 2020 05:04
@ndkoval ndkoval force-pushed the segment-queue-synchronizer branch 4 times, most recently from d9e5d6b to 347021d Compare December 10, 2020 07:24
@ndkoval
Copy link
Member Author

ndkoval commented Dec 21, 2020

We should add a comment here once the PR is merged

@ndkoval ndkoval requested review from dkhalanskyjb and qwwdfsad and removed request for elizarov December 23, 2020 03:06
@ndkoval ndkoval removed the postponed label Dec 23, 2020
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

A small batch of suggestions for now.

kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
kotlinx-coroutines-core/common/src/sync/ReadWriteMutex.kt Outdated Show resolved Hide resolved
@ndkoval ndkoval force-pushed the segment-queue-synchronizer branch from 60f9bea to 54d5f05 Compare July 29, 2021 18:03
@ndkoval
Copy link
Member Author

ndkoval commented Sep 30, 2021

It's important not to forget about #2401 when introducing RWMutex

@ndkoval ndkoval changed the title SegmentQueueSynchronizer SegmentQueueSynchronizer and ReadWriteMutex Nov 8, 2021
@GavinRay97
Copy link

Just out of curiosity, why did this die?

It seems well-written/documented (can't comment on the implementation)

@qwwdfsad
Copy link
Member

There were unresolved questions about public API shape and how to expose read/write parts.
We may return to it after #3103

@qwwdfsad qwwdfsad closed this Nov 1, 2022
@qwwdfsad qwwdfsad mentioned this pull request Nov 21, 2022
…k` so that it supports buffered channels and pre-allocates elements.

Signed-off-by: Nikita Koval <ndkoval@ya.ru>
…ments

Signed-off-by: Nikita Koval <ndkoval@ya.ru>
@ndkoval ndkoval reopened this Feb 13, 2023
ndkoval and others added 4 commits February 13, 2023 19:11
…is detected

Signed-off-by: Nikita Koval <ndkoval@ya.ru>
…primitives and `ReadWriteMutex`

Signed-off-by: Nikita Koval <ndkoval@ya.ru>
…primitives and `ReadWriteMutex`

Signed-off-by: Nikita Koval <ndkoval@ya.ru>
@Tmpod
Copy link

Tmpod commented Nov 24, 2023

We may return to it after #3103

That was merged earlier this year (congrats to all btw, big PR!). Has there been any internal progress on this since then?

@qwwdfsad
Copy link
Member

@Tmpod we are evaluating this change on IDEA codebase.
Could you please elaborate on your interest in this primitive? E.g. what use-cases are you aiming to solve with it?

@Tmpod
Copy link

Tmpod commented Nov 25, 2023 via email

@ndkoval ndkoval changed the title SegmentQueueSynchronizer and ReadWriteMutex CancellableQueueSynchronizer and ReadWriteMutex Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants