Skip to content

Simplify EventLoop abstractions for timed scheduled tasks#9470

Merged
normanmaurer merged 5 commits intonetty:4.1from
njhill:decouple_timer_changes
Aug 21, 2019
Merged

Simplify EventLoop abstractions for timed scheduled tasks#9470
normanmaurer merged 5 commits intonetty:4.1from
njhill:decouple_timer_changes

Conversation

@njhill
Copy link
Copy Markdown
Member

@njhill njhill commented Aug 16, 2019

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 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.

@netty-bot
Copy link
Copy Markdown

Can one of the admins verify this patch?

@njhill
Copy link
Copy Markdown
Member Author

njhill commented Aug 16, 2019

I pushed a second commit and reworded the original commit message / PR description a little. Sorry that it's still so long!

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

Copy link
Copy Markdown
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

njhill added 4 commits August 19, 2019 12:26
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()
@njhill njhill force-pushed the decouple_timer_changes branch from 2246a6a to 4d9c489 Compare August 19, 2019 20:18
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

1 similar comment
@normanmaurer
Copy link
Copy Markdown
Member

@netty-bot test this please

@normanmaurer
Copy link
Copy Markdown
Member

This looks great!

@normanmaurer normanmaurer merged commit a22d4ba into netty:4.1 Aug 21, 2019
@normanmaurer
Copy link
Copy Markdown
Member

@njhill thanks a lot! Would you be interested to see how we can back port some of the stuff to master ?

@njhill njhill deleted the decouple_timer_changes branch August 21, 2019 16:26
@njhill
Copy link
Copy Markdown
Member Author

njhill commented Aug 21, 2019

@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..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants