Skip to content
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

Clean up NioEventLoop #9799

Merged
merged 2 commits into from
Nov 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ protected boolean runAllTasks(long timeoutNanos) {
return false;
}

final long deadline = ScheduledFutureTask.nanoTime() + timeoutNanos;
final long deadline = timeoutNanos > 0 ? ScheduledFutureTask.nanoTime() + timeoutNanos : 0;
long runTasks = 0;
long lastExecutionTime;
for (;;) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,14 @@ public int get() throws Exception {
}
};

private static final long AWAKE = -1L;
private static final long NONE = Long.MAX_VALUE;

// nextWakeupNanos is:
// -1 when EL is awake
// Long.MAX_VALUE when EL is waiting with no wakeup scheduled
// 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(-1L);
private final AtomicLong nextWakeupNanos = new AtomicLong(AWAKE);
Copy link
Contributor

@franz1981 franz1981 Nov 25, 2019

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good followup PR (which is also true for EpollEventLoop.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

private boolean pendingWakeup;
private volatile int ioRatio = 50;

Expand Down Expand Up @@ -181,21 +184,21 @@ NativeDatagramPacketArray cleanDatagramPacketArray() {

@Override
protected void wakeup(boolean inEventLoop) {
if (!inEventLoop && nextWakeupNanos.getAndSet(-1L) != -1L) {
if (!inEventLoop && nextWakeupNanos.getAndSet(AWAKE) != AWAKE) {
// write to the evfd which will then wake-up epoll_wait(...)
Native.eventFdWrite(eventFd.intValue(), 1L);
}
}

@Override
protected boolean beforeScheduledTaskSubmitted(long deadlineNanos) {
// Note this is also correct for the nextWakeupNanos == -1 case
// Note this is also correct for the nextWakeupNanos == -1 (AWAKE) case
return deadlineNanos < nextWakeupNanos.get();
}

@Override
protected boolean afterScheduledTaskSubmitted(long deadlineNanos) {
// Note this is also correct for the nextWakeupNanos == -1 case
// Note this is also correct for the nextWakeupNanos == -1 (AWAKE) case
return deadlineNanos < nextWakeupNanos.get();
}

Expand Down Expand Up @@ -304,7 +307,7 @@ public int registeredChannels() {
}

private int epollWait(long deadlineNanos) throws IOException {
if (deadlineNanos == Long.MAX_VALUE) {
if (deadlineNanos == NONE) {
return Native.epollWait(epollFd, events, timerFd, Integer.MAX_VALUE, 0); // disarm timer
}
long totalDelay = deadlineToDelayNanos(deadlineNanos);
Expand Down Expand Up @@ -332,7 +335,7 @@ private int epollWaitTimeboxed() throws IOException {

@Override
protected void run() {
long prevDeadlineNanos = Long.MAX_VALUE;
long prevDeadlineNanos = NONE;
for (;;) {
try {
processPendingChannelFlags();
Expand Down Expand Up @@ -365,7 +368,7 @@ protected void run() {

long curDeadlineNanos = nextScheduledTaskDeadlineNanos();
if (curDeadlineNanos == -1L) {
curDeadlineNanos = Long.MAX_VALUE; // nothing on the calendar
curDeadlineNanos = NONE; // nothing on the calendar
}
nextWakeupNanos.set(curDeadlineNanos);
try {
Expand All @@ -382,7 +385,7 @@ protected void run() {
} finally {
// Try get() first to avoid much more expensive CAS in the case we
// were woken via the wakeup() method (submitted task)
if (nextWakeupNanos.get() == -1L || nextWakeupNanos.getAndSet(-1L) == -1L) {
if (nextWakeupNanos.get() == AWAKE || nextWakeupNanos.getAndSet(AWAKE) == AWAKE) {
pendingWakeup = true;
}
}
Expand All @@ -394,24 +397,25 @@ protected void run() {
if (ioRatio == 100) {
try {
if (strategy > 0 && processReady(events, strategy)) {
prevDeadlineNanos = Long.MAX_VALUE;
prevDeadlineNanos = NONE;
}
} finally {
// Ensure we always run tasks.
runAllTasks();
}
} else {
} else if (strategy > 0) {
final long ioStartTime = System.nanoTime();

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

this (100 - ioRatio) / ioRatio has to be called on each iteration? / seems quite heavy

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
} else {
runAllTasks(0); // This will run the minimum number of tasks
}
if (allowGrowing && strategy == events.length()) {
//increase the size of the array as we needed the whole space for the events
Expand Down