Epoll: Avoid redundant EPOLL_CTL_MOD calls #9397

njhill · 2019-07-23T21:34:01Z

Motivation

Currently an epoll_ctl syscall is made every time there is a change to the event interest flags (EPOLLIN, EPOLLOUT, etc) of a channel. These are only done in the event loop so can be aggregated into 0 or 1 such calls per channel prior to the next call to epoll_wait.

Modifications

I think further streamlining/simplification is possible but for now I've tried to minimize structural changes and added the aggregation beneath the existing flag manipulation logic.

A new AbstractChannel#activeFlags field records the flags last set on the epoll fd for that channel. Calls to setFlag/clearFlag update the flags field as before but instead of calling epoll_ctl immediately, just set or clear a bit for the channel in a new bitset in the associated EpollEventLoop to reflect whether there's any change to the last set value.

Prior to calling epoll_wait the event loop makes the appropriate epoll_ctl(EPOLL_CTL_MOD) call once for each channel who's bit is set.

Result

Fewer syscalls, particularly in some auto-read=false cases. Simplified error handling from centralization of these calls.

Motivation Currently an epoll_ctl syscall is made every time there is a change to the event interest flags (EPOLLIN, EPOLLOUT, etc) of a channel. These are only done in the event loop so can be aggregated into 0 or 1 such calls per channel prior to the next call to epoll_wait. Modifications I think further streamlining/simplification is possible but for now I've tried to minimize structural changes and added the aggregation beneath the existing flag manipulation logic. A new AbstractChannel#activeFlags field records the flags last set on the epoll fd for that channel. Calls to setFlag/clearFlag update the flags field as before but instead of calling epoll_ctl immediately, just set or clear a bit for the channel in a new bitset in the associated EpollEventLoop to reflect whether there's any change to the last set value. Prior to calling epoll_wait the event loop makes the appropriate epoll_ctl(EPOLL_CTL_MOD) call once for each channel who's bit is set. Result Fewer syscalls, particularly in some auto-read=false cases. Simplified error handling from centralization of these calls.

netty-bot · 2019-07-23T21:34:27Z

Can one of the admins verify this patch?

normanmaurer · 2019-07-24T05:33:52Z

@netty-bot test this please

normanmaurer · 2019-07-26T06:19:54Z

transport-native-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollChannel.java

        }
+        // schedule a task to clear the EPOLLIN as it is not safe to modify it directly
+        loop.execute(new Runnable() {


nit: couldn't we create this Runnable only once and re-use ?

I don't think it's directly related to the change (was like this before), and it's now hit only when setAutoRead(false) is called from outside the EL, but sure :)

wouldn't the old implementation make more sense since it's such an edge case? if it occurs often, sure makes sense, otherwise if it happens seldom the application will pay the price in memory for every channel.

it's only one field so I think it is ok to cache and so reduce the GC pressure

normanmaurer · 2019-07-26T06:21:22Z

transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java

@@ -59,6 +60,8 @@
    private final FileDescriptor eventFd;
    private final FileDescriptor timerFd;
    private final IntObjectMap<AbstractEpollChannel> channels = new IntObjectHashMap<AbstractEpollChannel>(4096);
+    private final BitSet pendingFlagChannels = new BitSet(); //TODO maybe just regular set of AbstractEpollChannels?


when we deregister the Channel from the EventLoop we also need to ensure we remove the BitSet entry.

njhill · 2019-07-26T17:04:15Z

@normanmaurer have pushed a commit addressing your comments. I made the Runnable lazily constructed, not sure if that's what you had in mind or just a final. And still uncertain about the change since it's an extra object per channel and I guess repeated calls to setAutoRead(false) for a given channel aren't common?

normanmaurer · 2019-08-12T08:14:52Z

@netty-bot test this please

transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java

normanmaurer · 2019-08-14T08:07:42Z

@netty-bot test this please

normanmaurer · 2019-08-14T11:41:09Z

@netty-bot test this please

carl-mastrangelo

LGTM

Motivation Currently an epoll_ctl syscall is made every time there is a change to the event interest flags (EPOLLIN, EPOLLOUT, etc) of a channel. These are only done in the event loop so can be aggregated into 0 or 1 such calls per channel prior to the next call to epoll_wait. Modifications I think further streamlining/simplification is possible but for now I've tried to minimize structural changes and added the aggregation beneath the existing flag manipulation logic. A new AbstractChannel#activeFlags field records the flags last set on the epoll fd for that channel. Calls to setFlag/clearFlag update the flags field as before but instead of calling epoll_ctl immediately, just set or clear a bit for the channel in a new bitset in the associated EpollEventLoop to reflect whether there's any change to the last set value. Prior to calling epoll_wait the event loop makes the appropriate epoll_ctl(EPOLL_CTL_MOD) call once for each channel who's bit is set. Result Fewer syscalls, particularly in some auto-read=false cases. Simplified error handling from centralization of these calls.

This reverts commit 8739886.

…"" This reverts commit b409f8e.

Motivation Currently an epoll_ctl syscall is made every time there is a change to the event interest flags (EPOLLIN, EPOLLOUT, etc) of a channel. These are only done in the event loop so can be aggregated into 0 or 1 such calls per channel prior to the next call to epoll_wait. Modifications I think further streamlining/simplification is possible but for now I've tried to minimize structural changes and added the aggregation beneath the existing flag manipulation logic. A new AbstractChannel#activeFlags field records the flags last set on the epoll fd for that channel. Calls to setFlag/clearFlag update the flags field as before but instead of calling epoll_ctl immediately, just set or clear a bit for the channel in a new bitset in the associated EpollEventLoop to reflect whether there's any change to the last set value. Prior to calling epoll_wait the event loop makes the appropriate epoll_ctl(EPOLL_CTL_MOD) call once for each channel who's bit is set. Result Fewer syscalls, particularly in some auto-read=false cases. Simplified error handling from centralization of these calls.

This reverts commit 2b9f69a.

This reverts commit 250b279.

njhill requested a review from normanmaurer July 23, 2019 21:34

njhill mentioned this pull request Jul 23, 2019

Epoll: Avoid modifying EPOLLIN interest when in ET mode #9348

Closed

normanmaurer added this to the 4.1.39.Final milestone Jul 26, 2019

normanmaurer assigned carl-mastrangelo Jul 26, 2019

normanmaurer requested changes Jul 26, 2019

View reviewed changes

normanmaurer requested a review from carl-mastrangelo July 26, 2019 06:31

normanmaurer mentioned this pull request Jul 26, 2019

Epoll: Avoid modifying EPOLLOUT interest when in ET mode #9347

Closed

Address @normanmaurer's comments

e853a76

normanmaurer requested changes Aug 12, 2019

View reviewed changes

transport-native-epoll/src/main/java/io/netty/channel/epoll/EpollEventLoop.java Outdated Show resolved Hide resolved

Don't use voidPromise with ch.close(...) per @normanmaurer's comment

569e337

normanmaurer modified the milestones: 4.1.39.Final, 4.1.40.Final Aug 13, 2019

normanmaurer approved these changes Aug 15, 2019

View reviewed changes

carl-mastrangelo approved these changes Aug 17, 2019

View reviewed changes

normanmaurer merged commit 8739886 into netty:4.1 Aug 19, 2019

njhill deleted the less_epoll_ctl branch August 19, 2019 16:38

normanmaurer added a commit that referenced this pull request Sep 12, 2019

Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (#9397)"

a8b49b4

This reverts commit 8739886.

normanmaurer added a commit that referenced this pull request Sep 12, 2019

Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (#9397)"

b409f8e

This reverts commit 8739886.

normanmaurer removed this from the 4.1.40.Final milestone Sep 12, 2019

njhill mentioned this pull request Sep 13, 2019

Revert epoll changes #9561

Merged

johnou added a commit to johnou/netty that referenced this pull request Sep 19, 2019

Revert "Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (netty#9397)…

202a739

…"" This reverts commit b409f8e.

njhill mentioned this pull request Sep 24, 2019

Close eventfd shutdown/wakeup race by closely tracking epoll edges #9586

Merged

normanmaurer added a commit that referenced this pull request Dec 11, 2019

Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (#9397) (#9583)"

9fa8b02

This reverts commit 2b9f69a.

normanmaurer added a commit that referenced this pull request Dec 11, 2019

Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (#9397)"

46cff53

This reverts commit 250b279.

njhill mentioned this pull request Dec 11, 2019

Reinstate EPOLL_CTL_MOD syscall avoidance optimization #9872

Open

ihanyong pushed a commit to ihanyong/netty that referenced this pull request Jul 31, 2020

Revert "Epoll: Avoid redundant EPOLL_CTL_MOD calls (netty#9397)"

a099145

This reverts commit 250b279.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Epoll: Avoid redundant EPOLL_CTL_MOD calls #9397

Epoll: Avoid redundant EPOLL_CTL_MOD calls #9397

njhill commented Jul 23, 2019

netty-bot commented Jul 23, 2019

normanmaurer commented Jul 24, 2019

normanmaurer Jul 26, 2019

njhill Jul 26, 2019

johnou Aug 16, 2019

normanmaurer Aug 16, 2019

normanmaurer Jul 26, 2019

njhill commented Jul 26, 2019

normanmaurer commented Aug 12, 2019

normanmaurer commented Aug 14, 2019

normanmaurer commented Aug 14, 2019

carl-mastrangelo left a comment

Epoll: Avoid redundant EPOLL_CTL_MOD calls #9397

Epoll: Avoid redundant EPOLL_CTL_MOD calls #9397

Conversation

njhill commented Jul 23, 2019

netty-bot commented Jul 23, 2019

normanmaurer commented Jul 24, 2019

normanmaurer Jul 26, 2019

Choose a reason for hiding this comment

njhill Jul 26, 2019

Choose a reason for hiding this comment

johnou Aug 16, 2019

Choose a reason for hiding this comment

normanmaurer Aug 16, 2019

Choose a reason for hiding this comment

normanmaurer Jul 26, 2019

Choose a reason for hiding this comment

njhill commented Jul 26, 2019

normanmaurer commented Aug 12, 2019

normanmaurer commented Aug 14, 2019

normanmaurer commented Aug 14, 2019

carl-mastrangelo left a comment

Choose a reason for hiding this comment