Introduce fast and scalable channels #3103

ndkoval · 2021-12-20T14:02:11Z

No description provided.

gradle/test-mocha-js.gradle

kotlinx-coroutines-core/build.gradle

kotlinx-coroutines-core/common/test/channels/BroadcastTest.kt

kotlinx-coroutines-core/common/test/channels/ChannelsTest.kt

kotlinx-coroutines-core/jvm/test/channels/ChannelMemoryLeakStressTest.kt

qwwdfsad · 2022-09-13T13:52:17Z

Two critical issues:

No TCO in send due to a bug in the inliner. Ilmir is figuring it out. If I inline sendImpl in send manually, TCO kicks in and everything becomes better
No TCO in iterator's hasNext. If I inline receiveImpl manually, TCO does not kick in and it is a serious problem that has to be investigated.

Please take a look

kotlinx-coroutines-core/common/src/selects/Select.kt

kotlinx-coroutines-core/jvm/test/channels/ChannelMemoryLeakStressTest.kt

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

kotlinx-coroutines-core/common/src/internal/ConcurrentLinkedList.kt

Signed-off-by: Nikita Koval <[email protected]>

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

kotlinx-coroutines-core/jvm/test/lincheck/ChannelsLincheckTest.kt

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

qwwdfsad · 2023-01-17T15:25:43Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+                    // and the `INTERRUPTED_SEND` token has been installed as a waiter,
+                    // this request finishes with the `onClosed()` action.
+                    if (closed) {
+                        segment.onSlotCleaned()


This branch isn't reachable, isn't it?
It the channel is closed, updateCellSend won't ever return RESULT_SUSPEND

It may be reachable. Consider a situation when a channel is marked as closed, but the cell is still empty. In this case, the operation installs INTERRUPTED_SEND in the cell. I missed this part of the algorithm when documenting the code -- will fix that.

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

Signed-off-by: Nikita Koval <[email protected]>

Marked as "fixes 3330" to automatically close the issue yet it's the original PR that fixed it Fixes #3330

…rom 'onUndeliveredElement' to global exception handler and wrap them into dedicated exception

…ancellationDoesNotBreakMutex` Signed-off-by: Nikita Koval <[email protected]>

qwwdfsad · 2023-02-07T15:54:31Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+                    // physically to avoid memory leaks.
+                    if (closed) {
+                        return onClosed()
+                    } else continue


Both me and Dmitry prefer having {} around continue if it's not a one-liner

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

qwwdfsad · 2023-02-08T09:45:26Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+                    // already been cancelled or the cell is poisoned.
+                    // Restart from the beginning in this case.
+                    // To avoid memory leaks, we also need to reset
+                    // the `prev` pointer of the working segment.


Could you please elaborate on the scenario when memory leak occurs if prev pointer is not cleared?

Do Lincheck tests fail if you comment this out?

Nope, none of ./gradlew :kotlinx-coroutines-core:jvmLincheckTest :kotlinx-coroutines-core:jvmLincheckTestAdditional

jvmLincheckTestAdditional fails with isStressTest = true

qwwdfsad · 2023-02-08T09:51:45Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+                }
+            }
+            // The cell stores a buffered element.
+            state === BUFFERED -> if (segment.casState(index, state, DONE_RCV)) {


This CAS should always succeed, shouldn't it?
Probably then it's better to either assert it or to replace it with the regular write

The BUFFERED state can be replaced with CLOSED. I would not recommend applying such optimizations, sticking to the more straightforward code patterns. According to my experience developing this algorithm, such optimizations are often wrong.

I see, thanks.

I was semi-automatically using diagram from the paper, and CLOSED is the only state omitted :)

qwwdfsad · 2023-02-08T09:54:17Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+                return segment.retrieveElement(index)
+            }
+        }
+        return updateCellReceiveSlow(segment, index, r, waiter)


Could you please drop a comment (here and in send as well) before slow-path that explains when we are falling into it?

E.g. // Slow-path: CAS on rendezvous failed and it's time to poison or ...another scenario...

This comment will be hard to maintain and useless. The slow path covers all transitions; that's it. The right question is not when we go to the slow path but what the fast path does.

Sorry, I've meant not to document all scenarios covered by slowpath with an inline comment, but rather a comment that explains when we go to the slow-path ("// There is no rendezvous or fast-path suspend failed")

Then the comment will be like "we go to the slow-path code when the fast path fails"

In this case, it's better to go to the only call of the slow-path function and see when it is invoked. At least, this approach is easy-to-maintain if the fast-path logic changes.

qwwdfsad · 2023-02-08T09:57:02Z

kotlinx-coroutines-core/common/src/channels/BufferedChannel.kt

+            }
+        }
+        return updateCellReceiveSlow(segment, index, r, waiter)
+    }


And on a separate note, could you please elaborate why slowpath-fastpath separation is needed at all?

My intuition is the following:

Fast path is "check few states and try CAS once, otherwise go to slow path" , while slow-path is "check all states and wrap it in a while loop in case CASes fail".
If that's true, a single "slow-path" should be enough, all we win by separation is a loop avoidance (which probably is virtually zero), but we get much less code and less duplicated state management in return

This is critical for better inlining. You can comment out the fast path and check that the performance on our benchmarks degrades.

qwwdfsad · 2023-02-08T17:27:29Z

It seems like we only have a question about cleanPrev.
As soon as it's addressed (and JVM IR transformer is disabled probably), the PR is good to go

Signed-off-by: Nikita Koval <[email protected]>

See #3621 for detailed description and rationale Fixes #3621

LouisCAD · 2023-02-13T12:41:35Z

Looks like that was a tough one!

Finally merged a bit more than one year in, good job, seems like state of the art! 👏👏👏👏👏

See #3621 for detailed description and rationale Fixes #3621

qwwdfsad added the for 1.7 label Jan 12, 2022

ndkoval force-pushed the fast-channels branch 2 times, most recently from 3bc29ab to 39a6f64 Compare January 16, 2022 14:38

ndkoval force-pushed the fast-channels branch 2 times, most recently from 04ada5b to 80335a9 Compare January 28, 2022 14:50

ndkoval force-pushed the fast-channels branch from 028c069 to b6a1c65 Compare March 9, 2022 21:16

qwwdfsad force-pushed the new-channels-select branch from 75a9947 to e38ca48 Compare May 30, 2022 10:04

qwwdfsad force-pushed the new-channels-select branch from 42f24b7 to df3aae1 Compare July 15, 2022 13:29

ndkoval force-pushed the new-channels-select branch 3 times, most recently from f0c3f5a to 851cb9b Compare August 2, 2022 22:09

ndkoval force-pushed the fast-channels branch 3 times, most recently from 0713a6f to 6185191 Compare August 3, 2022 14:06

qwwdfsad self-requested a review August 4, 2022 09:55

qwwdfsad mentioned this pull request Sep 12, 2022

CancellableQueueSynchronizer and ReadWriteMutex #2045

Open

ndkoval changed the base branch from new-channels-select to develop September 13, 2022 12:34

qwwdfsad reviewed Sep 13, 2022

View reviewed changes

qwwdfsad force-pushed the fast-channels branch from 022bc61 to 778b2ac Compare October 18, 2022 10:58

qwwdfsad requested changes Oct 18, 2022

View reviewed changes

qwwdfsad mentioned this pull request Nov 1, 2022

Stabilize isClosedForSend and isClosedForReceive #3517

Merged

ndkoval force-pushed the fast-channels branch from ca8d437 to 0016596 Compare November 4, 2022 11:43

qwwdfsad reviewed Nov 24, 2022

View reviewed changes

kotlinx-coroutines-core/common/src/internal/ConcurrentLinkedList.kt Show resolved Hide resolved

ndkoval force-pushed the fast-channels branch from 780f158 to bb3a7b8 Compare November 26, 2022 13:17

ndkoval mentioned this pull request Nov 28, 2022

Optimize CancellableContinuationImpl.invokeOnCancellation(..) for Segments #3084

Merged

ndkoval force-pushed the fast-channels branch 4 times, most recently from 3163c2c to 8022dab Compare December 19, 2022 11:18

ndkoval added 5 commits January 12, 2023 17:13

Almost non-blocking ConflatedBufferedChannel

e42569c

Signed-off-by: Nikita Koval <[email protected]>

Review fixes

94b6397

Signed-off-by: Nikita Koval <[email protected]>

ConflatedBufferedChannel

b0f4863

Signed-off-by: Nikita Koval <[email protected]>

ConflatedBufferedChannel

184ed7b

Signed-off-by: Nikita Koval <[email protected]>

ConflatedBufferedChannel

306cd58

Signed-off-by: Nikita Koval <[email protected]>

ndkoval requested a review from qwwdfsad January 16, 2023 10:43

ndkoval added 3 commits January 16, 2023 19:33

Remove SegmentBasedQueue

ef2fddb

Signed-off-by: Nikita Koval <[email protected]>

Remove SegmentBasedQueue

7f1c931

Signed-off-by: Nikita Koval <[email protected]>

Fix BroadcastChannel

6eed6a9

Signed-off-by: Nikita Koval <[email protected]>

qwwdfsad reviewed Jan 17, 2023

View reviewed changes

ndkoval and others added 2 commits January 17, 2023 21:58

Review fixes

b6bfa4c

Signed-off-by: Nikita Koval <[email protected]>

Add test that enforces #3330

e114e59

Marked as "fixes 3330" to automatically close the issue yet it's the original PR that fixed it Fixes #3330

qwwdfsad self-requested a review January 25, 2023 10:24

qwwdfsad and others added 3 commits January 25, 2023 15:12

Consolidate onUndeliveredElement handling: do not report exceptions f…

f001b7a

…rom 'onUndeliveredElement' to global exception handler and wrap them into dedicated exception

Add a more readable BufferedChannel.toString() and fix `testStressC…

3c4c3f3

…ancellationDoesNotBreakMutex` Signed-off-by: Nikita Koval <[email protected]>

Fix stacktrace recovery for Channel.send

dbe32b9

qwwdfsad force-pushed the fast-channels branch from 1317235 to dbe32b9 Compare February 7, 2023 12:55

qwwdfsad reviewed Feb 7, 2023

View reviewed changes

qwwdfsad self-requested a review February 8, 2023 10:26

qwwdfsad reviewed Feb 8, 2023

View reviewed changes

qwwdfsad added 2 commits February 8, 2023 12:20

~add a note

8c021f3

~more notes

45d6d66

Review fixes

9040230

Signed-off-by: Nikita Koval <[email protected]>

qwwdfsad merged commit 22f8626 into develop Feb 13, 2023

qwwdfsad mentioned this pull request Feb 13, 2023

Fast and scalable channels algorithm #3621

Closed

qwwdfsad pushed a commit that referenced this pull request Feb 13, 2023

Introduce fast and scalable channels (#3103)

e7cf632

See #3621 for detailed description and rationale Fixes #3621

ndkoval added a commit that referenced this pull request Feb 13, 2023

Introduce fast and scalable channels (#3103)

2db605a

See #3621 for detailed description and rationale Fixes #3621

ndkoval self-assigned this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce fast and scalable channels #3103

Introduce fast and scalable channels #3103

ndkoval commented Dec 20, 2021

qwwdfsad commented Sep 13, 2022

qwwdfsad Jan 17, 2023

ndkoval Jan 17, 2023

qwwdfsad Feb 7, 2023

qwwdfsad Feb 8, 2023

ndkoval Feb 8, 2023

qwwdfsad Feb 8, 2023 •

edited

Loading

ndkoval Feb 10, 2023

qwwdfsad Feb 8, 2023

ndkoval Feb 8, 2023

qwwdfsad Feb 8, 2023

qwwdfsad Feb 8, 2023

ndkoval Feb 8, 2023

qwwdfsad Feb 8, 2023

ndkoval Feb 8, 2023

ndkoval Feb 8, 2023

qwwdfsad Feb 8, 2023

ndkoval Feb 8, 2023

qwwdfsad commented Feb 8, 2023

LouisCAD commented Feb 13, 2023

Introduce fast and scalable channels #3103

Introduce fast and scalable channels #3103

Conversation

ndkoval commented Dec 20, 2021

qwwdfsad commented Sep 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qwwdfsad commented Feb 8, 2023

LouisCAD commented Feb 13, 2023

qwwdfsad Feb 8, 2023 •

edited

Loading