Improve the performance of expireTimeouts() in HashedWheelTimer #12888
Conversation
|
@needmorecode Thanks a lot! |
Motivation: The original logic relies on a variable `remainingRounds` and in order to maintain it, all timeouts in the bucket have to be traversed in a tick. In fact, the complete traversing of the linked list is not necessary. Modification: My idea is to introduce a new variable `currRound` which represent the current round of the timer, and execRound for the execution round of each timeout. `currRound` is added by 1 when the tick starts a new round. Then for each timeout, we compare `currRound` and `execRound` to determine if the task should be executed, and break the loop once `currRound` < `execRound`. Result: By this means, we can reduce the number of traversed nodes, and the performance would be especially improved when the node number is large.
|
My pleasure :) @normanmaurer |
|
@chrisvest This seems to cause a significant regression in HashedWheelTimer behavior. I don't have the exact details of the behavior change, but I have a test in Apache Pulsar that becomes flaky after upgrading to 4.1.85.Final. Here are the instructions to reproduce the issue in the context of Apache Pulsar: apache/pulsar#18599 (comment) |
|
@lhotari Please open a new issue and add details and links there. Easier for us to track it that way. |
Thanks for the quick reply @chrisvest. I'll do so. |
|
@chrisvest I created #13018 . I haven't analysed the problem so that's why the description is vague atm. |
| @@ -115,6 +115,7 @@ public class HashedWheelTimer implements Timer { | |||
| private final AtomicLong pendingTimeouts = new AtomicLong(0); | |||
| private final long maxPendingTimeouts; | |||
| private final Executor taskExecutor; | |||
| private long currRound; | |||
There was a problem hiding this comment.
I am wondering, shouldn't this field be moved to Worker class where it belongs? (otherwise it is potentially shared between multiple threads and may need volatile, seems to be unnecessary)
There was a problem hiding this comment.
There is a logic error in my code. Please refer to #13018 for detail.
Motivation:
The original logic relies on a variable
remainingRoundsand in order to maintain it, all timeouts in the bucket have to be traversed in a tick.In fact, the complete traversing of the linked list is not necessary.
Modification:
My idea is to introduce a new variable
currRoundwhich represent the current round of the timer, andexecRoundfor the execution round of each timeout.currRoundis added by 1 when the tick starts a new round. Then for each timeout, we comparecurrRoundandexecRoundto determine if the task should be executed, and break the loop oncecurrRound<execRound.Result:
By this means, we can reduce the number of traversed nodes, and the performance would be especially improved when the node number is large.