-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduce fast and scalable channels #3103
Conversation
3bc29ab
to
39a6f64
Compare
04ada5b
to
80335a9
Compare
75a9947
to
e38ca48
Compare
42f24b7
to
df3aae1
Compare
f0c3f5a
to
851cb9b
Compare
0713a6f
to
6185191
Compare
kotlinx-coroutines-core/jvm/test/channels/ChannelMemoryLeakStressTest.kt
Outdated
Show resolved
Hide resolved
Two critical issues:
Please take a look |
022bc61
to
778b2ac
Compare
kotlinx-coroutines-core/jvm/test/channels/ChannelMemoryLeakStressTest.kt
Show resolved
Hide resolved
ca8d437
to
0016596
Compare
780f158
to
bb3a7b8
Compare
3163c2c
to
8022dab
Compare
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
Signed-off-by: Nikita Koval <[email protected]>
// and the `INTERRUPTED_SEND` token has been installed as a waiter, | ||
// this request finishes with the `onClosed()` action. | ||
if (closed) { | ||
segment.onSlotCleaned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This branch isn't reachable, isn't it?
It the channel is closed, updateCellSend
won't ever return RESULT_SUSPEND
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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]>
1317235
to
dbe32b9
Compare
// physically to avoid memory leaks. | ||
if (closed) { | ||
return onClosed() | ||
} else continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both me and Dmitry prefer having {}
around continue
if it's not a one-liner
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate on the scenario when memory leak occurs if prev pointer is not cleared?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do Lincheck tests fail if you comment this out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, none of ./gradlew :kotlinx-coroutines-core:jvmLincheckTest :kotlinx-coroutines-core:jvmLincheckTestAdditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jvmLincheckTestAdditional
fails with isStressTest = true
} | ||
} | ||
// The cell stores a buffered element. | ||
state === BUFFERED -> if (segment.casState(index, state, DONE_RCV)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks.
I was semi-automatically using diagram from the paper, and CLOSED
is the only state omitted :)
return segment.retrieveElement(index) | ||
} | ||
} | ||
return updateCellReceiveSlow(segment, index, r, waiter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the comment will be like "we go to the slow-path code when the fast path fails"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
return updateCellReceiveSlow(segment, index, r, waiter) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is critical for better inlining. You can comment out the fast path and check that the performance on our benchmarks degrades.
It seems like we only have a question about cleanPrev. |
Signed-off-by: Nikita Koval <[email protected]>
Looks like that was a tough one! Finally merged a bit more than one year in, good job, seems like state of the art! 👏👏👏👏👏 |
No description provided.