-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Optimize CancellableContinuationImpl.invokeOnCancellation(..)
for Segment
s
#3084
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
Conversation
aeeea94
to
e903287
Compare
As semaphore leverages this optimization, I added a simple sequential benchmark to show the impact. The results are below. WITHOUT optimization:
WITH optimization:
|
001e766
to
1092fca
Compare
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'm quite okay with the general idea. Please postpone it from merge though, I'll evaluate it once channels are properly reviewed
Will be delivered along with #3103 |
1092fca
to
638760f
Compare
c7042e2
to
f8af950
Compare
Let's keep the separation into two commits: the first that fixes/adds benchmarks, and the second that optimizes the cancellation handling mechanism. |
Could you please show before/after on |
9ed3b1b
to
7317fe5
Compare
See below the results on my laptop (MacBook Pro 16-inch, 2021, Apple M1 Max, 64 GB, OpenJDK 64-Bit Server VM Zulu19.32+13-CA). WITHOUT the optimization:
WITH the optimization:
|
These benchmarks do not show performance improvement but clearly show reduced allocations (more than 3x on |
Nice! I'm looking into that |
…annelSinkBenchmark` that supports buffered channels and pre-allocates elements. Signed-off-by: Nikita Koval <[email protected]>
…ments Signed-off-by: Nikita Koval <[email protected]>
7317fe5
to
8202abf
Compare
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.
LGTM. Will wait for the tests to run and merge
Current semaphore implementation uses
Segment
-s for storing waiting continuations. Moreover, the upcoming new channels and mutex algorithms also use segments to store waiters. When suspending, a cancellation handler should be provided viacont.invokeOnCancellation { ... }
-- it cleans up the corresponding slot in the segment and physically removes this segment from the linked list if it becomes full of cancelled cells. However, this cancellation handler requires an allocation every time.To reduce the memory pressure, we can store the segment along with the slot index in
CancellableContinuationImpl
directly, as a cancellation handler instruction; thus, eliminating allocations for the corresponding cancellation handlers. For this purpose, we:Segment
instate
field, similarly toCancelHandler
. On cancellation,Segment.invokeOnCancellation(index, cause)
function is called.decision
integer field, extending its purpose correspondingly.The benchmark below (see the comment) shows a significant allocation rate reduction.