Clean up NioEventLoop #9799
Clean up NioEventLoop #9799
Conversation
Motivation The event loop implementations had become somewhat tangled over time and work was done recently to streamline EpollEventLoop. NioEventLoop would benefit from the same treatment and it is more straighforward now that we can follow the same structure as was done for epoll. Modifications Untangle NioEventLoop logic and mirror what's now done in EpollEventLoop w.r.t. the volatile selector wake-up guard and scheduled task deadline handling. Some common refinements to EpollEventLoop have also been included - to use constants for the "special" deadline/wakeup volatile values and to avoid some unnecessary calls to System.nanoTime() on task-only iterations. Result Hopefully cleaner, more efficient and less fragile NIO transport implementation.
|
Can one of the admins verify this patch? |
|
@netty-bot test this please |
1 similar comment
|
@netty-bot test this please |
| // other value T when EL is waiting with wakeup scheduled at time T | ||
| private final AtomicLong nextWakeupNanos = new AtomicLong(-1L); | ||
| private final AtomicLong nextWakeupNanos = new AtomicLong(AWAKE); |
There was a problem hiding this comment.
is outside the scope of this PR, but why not an AtomicLongFieldUpdater?
It would allow to control its layout if profiling shows that's highly contended (and it seems the case 2 me, with many producers)
There was a problem hiding this comment.
Sounds like a good followup PR (which is also true for EpollEventLoop.
There was a problem hiding this comment.
Sure I can do it in a follow-on. I used AtomicLong here to be consistent with EpollEventLoop. That was originally an atomic updater but @normanmaurer suggested changing it to AtomicLong in a prior PR here: #9605 (comment)... I am fine with either though!
| if (strategy > 0 && processReady(events, strategy)) { | ||
| prevDeadlineNanos = Long.MAX_VALUE; | ||
| if (processReady(events, strategy)) { | ||
| prevDeadlineNanos = NONE; | ||
| } | ||
| } finally { | ||
| // Ensure we always run tasks. | ||
| final long ioTime = System.nanoTime() - ioStartTime; | ||
| runAllTasks(ioTime * (100 - ioRatio) / ioRatio); |
There was a problem hiding this comment.
this (100 - ioRatio) / ioRatio has to be called on each iteration? / seems quite heavy
There was a problem hiding this comment.
@franz1981 maybe also for a separate PR since it's always been like this AFAIK... though I think if we want to calculate an accurate percentage of the measured time then a div will have to be done at some point, unless we want do a floating point multiply (and conversion to/from fp).
Also we had been thinking of getting rid of this altogether, and always process all tasks (with some limit on number of outer iterations) - see #9590 (comment). After more thought I feel like that makes more sense actually... thoughts?
| // AWAKE when EL is awake | ||
| // NONE when EL is waiting with no wakeup scheduled | ||
| // other value T when EL is waiting with wakeup scheduled at time T | ||
| private final AtomicLong nextWakeupNanos = new AtomicLong(AWAKE); |
There was a problem hiding this comment.
AtomicLongFiedUpdater as well
| final long ioStartTime = System.nanoTime(); | ||
| try { | ||
| processSelectedKeys(); | ||
| } finally { | ||
| // Ensure we always run tasks. | ||
| final long ioTime = System.nanoTime() - ioStartTime; | ||
| runAllTasks(ioTime * (100 - ioRatio) / ioRatio); | ||
| ranTasks = runAllTasks(ioTime * (100 - ioRatio) / ioRatio); |
There was a problem hiding this comment.
(100 - ioRatio) / ioRatio should be done on each iteration?
|
Imho all of the comments should be a follow up
… Am 25.11.2019 um 19:02 schrieb Nick Hill ***@***.***>:
@njhill commented on this pull request.
In transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java:
> try {
- if (strategy > 0 && processReady(events, strategy)) {
- prevDeadlineNanos = Long.MAX_VALUE;
+ if (processReady(events, strategy)) {
+ prevDeadlineNanos = NONE;
}
} finally {
// Ensure we always run tasks.
final long ioTime = System.nanoTime() - ioStartTime;
runAllTasks(ioTime * (100 - ioRatio) / ioRatio);
@franz1981 maybe also for a separate PR since it's always been like this AFAIK... though I think if we want to calculate an accurate percentage of the measured time then a div will have to be done at some point, unless we want do a floating point multiply (and conversion to/from fp).
Also we had been thinking of getting rid of this altogether, and always process all tasks (with some limit on number of outer iterations) - see #9590 (comment). After more thought I feel like that makes more sense actually... thoughts?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@njhill @normanmaurer Agree about the follow-up: it seems already ok as it is (it's a port indeed)! |
|
Thanks @normanmaurer @franz1981, I also agree about the follow-up... my question in the comment above was kind of off-topic not meant for this PR! I will open another later to discuss elimination of |
|
@njhill thanks a lot... Please also back port to master. |
Motivation
The event loop implementations had become somewhat tangled over time and work was done recently to streamline
EpollEventLoop.NioEventLoopwould benefit from the same treatment and it is more straightforward now that we can follow the same structure as was done for epoll.Modifications
Untangle
NioEventLooplogic and mirror what's now done inEpollEventLoopw.r.t. the volatile selector wake-up guard and scheduled task deadline handling.Some common refinements to
EpollEventLoophave also been included - to use constants for the "special" deadline/wakeup volatile values and to avoid some unnecessary calls toSystem.nanoTime()on task-only iterations.Result
Hopefully cleaner, more efficient and less fragile NIO transport implementation.