Simplify EventLoop abstractions for timed scheduled tasks#9470
Simplify EventLoop abstractions for timed scheduled tasks#9470normanmaurer merged 5 commits intonetty:4.1from
Conversation
|
Can one of the admins verify this patch? |
232d0ad to
8e64d04
Compare
|
I pushed a second commit and reworded the original commit message / PR description a little. Sorry that it's still so long! |
|
@netty-bot test this please |
common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/AbstractScheduledEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
Outdated
Show resolved
Hide resolved
transport-native-epoll/src/test/java/io/netty/channel/epoll/EpollTest.java
Outdated
Show resolved
Hide resolved
transport/src/main/java/io/netty/channel/SingleThreadEventLoop.java
Outdated
Show resolved
Hide resolved
transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java
Outdated
Show resolved
Hide resolved
franz1981
left a comment
There was a problem hiding this comment.
LGTM
[Optional] there are other parts where a difference should be used to evaluate deadline reached ie would be nice to address those too, but considering that is outside the scope of this PR is fine to address them separately too
|
@netty-bot test this please |
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
Motivation The epoll transport was updated in netty#7834 to decouple setting of the timerFd from the event loop, so that scheduling delayed tasks does not require waking up epoll_wait. To achieve this, new overridable hooks were added in the AbstractScheduledEventExecutor and SingleThreadEventExecutor superclasses. However, the minimumDelayScheduledTaskRemoved hook has no current purpose and I can't envisage a _practical_ need for it. Removing it would reduce complexity and avoid supporting this specific API indefinitely. We can add something similar later if needed but the opposite is not true. There also isn't a _nice_ way to use the abstractions for wakeup-avoidance optimizations in other EventLoops that don't have a decoupled timer. This PR replaces executeScheduledRunnable and wakesUpForScheduledRunnable with two new methods before/afterFutureTaskScheduled that have slightly different semantics: - They only apply to additions; given the current internals there's no practical use for removals - They allow per-submission wakeup decisions via a boolean return val, which makes them easier to exploit from other existing EL impls (e.g. NIO/KQueue) - They are subjectively "cleaner", taking just the deadline parameter and not exposing Runnables - For current EL/queue impls, only the "after" hook is really needed, but specialized blocking queue impls can conditionally wake on task submission (I have one lined up) Also included are further optimization/simplification/fixes to the timerFd manipulation logic. Modifications - Remove AbstractScheduledEventExecutor#minimumDelayScheduledTaskRemoved() and supporting methods - Uplift NonWakeupRunnable and corresponding default wakesUpForTask() impl from SingleThreadEventLoop to SingleThreadEventExecutor - Change executeScheduledRunnable() to be package-private, and have a final impl in SingleThreadEventExecutor which triggers new overridable hooks before/afterFutureTaskScheduled() - Remove unnecessary use of bookend tasks while draining the task queue - Use new hooks to add simpler wake-up avoidance optimization to NioEventLoop (primarily to demonstrate utility/simplicity) - Reinstate removed EpollTest class In EpollEventLoop: - Refactor to use only the new afterFutureTaskScheduled() hook for updating timerFd - Fix setTimerFd race condition using a monitor - Set nextDeadlineNanos to a negative value while the EL is awake and use this to block timer changes from outside the EL. Restore the known-set value prior to sleeping, updating timerFd first if necessary - Don't read from timerFd when processing expiry event Result - Cleaner API for integrating with different EL/queue timing impls - Fixed race condition to avoid missing scheduled wakeups - Eliminate unnecessary timerFd updates while EL is awake, and unnecessary expired timerFd reads - Avoid unnecessary scheduled-task wakeups when using NIO transport I did not yet further explore the suggestion of using TFD_TIMER_ABSTIME for the timerFd.
Also improve explanatory comments in checkScheduleTaskQueueForNewDelay()
2246a6a to
4d9c489
Compare
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
common/src/main/java/io/netty/util/concurrent/SingleThreadEventExecutor.java
Outdated
Show resolved
Hide resolved
|
@netty-bot test this please |
|
@netty-bot test this please |
1 similar comment
|
@netty-bot test this please |
|
This looks great! |
|
@njhill thanks a lot! Would you be interested to see how we can back port some of the stuff to master ? |
|
@normanmaurer sure I can take a look at that though have a small backlog right now so can't promise how soon... simplifying #9416 was next on my netty list when I get the cycles.. |
Motivation
The epoll transport was updated in #7834 to decouple setting of the timerFd from the event loop, so that scheduling delayed tasks does not require waking up
epoll_wait. To achieve this, new overridable hooks were added in theAbstractScheduledEventExecutorandSingleThreadEventExecutorsuperclasses.However, the
minimumDelayScheduledTaskRemovedhook has no current purpose and I can't envisage a practical need for it. Removing it would reduce complexity and avoid supporting this specific API indefinitely. We can add something similar later if needed but the opposite is not true.There also isn't a nice way to use the abstractions for wakeup-avoidance optimizations in other
EventLoops that don't have a decoupled timer.This PR replaces
executeScheduledRunnableandwakesUpForScheduledRunnablewith two new methods
before/afterFutureTaskScheduledthat have slightly different semantics:Also included are further optimization/simplification/fixes to the timerFd manipulation logic.
Modifications
AbstractScheduledEventExecutor#minimumDelayScheduledTaskRemoved()and supporting methodsNonWakeupRunnableand corresponding defaultwakesUpForTask()impl fromSingleThreadEventLooptoSingleThreadEventExecutorexecuteScheduledRunnable()to be package-private, and have a final impl inSingleThreadEventExecutorwhich triggers new overridable hooksbefore/afterFutureTaskScheduled()NioEventLoop(primarily to demonstrate utility/simplicity)EpollTestclassIn
EpollEventLoop:afterFutureTaskScheduled()hook for updating timerFdnextDeadlineNanosto a negative value while the EL is awake and use this to block timer changes from outside the EL. Restore the known-set value prior to sleeping, updating timerFd first if necessaryResult
I did not yet further explore the suggestion of using
TFD_TIMER_ABSTIMEfor the timerFd.