Epoll: Avoid modifying EPOLLOUT interest when in ET mode#9347
Epoll: Avoid modifying EPOLLOUT interest when in ET mode#9347
Conversation
|
Can one of the admins verify this patch? |
|
how often is that path of code called though? isn't it triggered only when writability changes which doesn't happen for most of the use cases(as in - your output buffers are full)? |
|
@ninja- good question and something I also wondered :) my guess is also that this would only benefit certain situations/workloads. Hopefully @normanmaurer @Scottmitch and others have better intuition about this. But it's a simple change and doesn't have downsides that I'm aware of, so even if its applicability is narrow it might be worthwhile. Also I just found out that when registered, the |
@normanmaurer @Scottmitch I think I may have realized the downside, which could invalidate this as a possibility. I will aim to verify, but I guess this change would mean virtually all registered channels are included in every event array even when only a subset have a "real" event (edge), since they would all be considered "ready". So when there are many channels this would result in a higher cost of iterating over them all on every wakeup. Does that sound right? Sorry for the noise if so... |
Motivation An inbound analog of netty#9347. This is only applicable when autoRead is turned off. AFAIU it shouldn't be necessary to toggle the EPOLLIN flag via successive calls to epoll_ctl between reads. Modifications Don't clear EPOLLIN flag when in ET mode with autoRead == false. Don't process EPOLLIN events when there is no read pending, just set the maybeMoreDataToRead flag. Result Fewer syscalls when using epoll transport without autoRead. I have not done any performance tests but expect the saving might be significant in cases where read() isn't often called in channelRead(...) or channelReadComplete(...) - i.e. two syscalls per read.
|
Scratch the above comment, it seems the concerns were misplaced and original assumptions correct (at least that particular concern/assumption, I expect there's other things I've missed/misunderstood!). Have also opened #9348 with similar change for |
|
@netty-bot test this please |
|
@njhill I will have a look but as suspected this should not have a lot of impact as it only happens when the channel is not writable or an async connect is happening. |
|
Thanks @normanmaurer, I guess the appeal is more the simplification potential, i.e. if using ET we never need to call |
Motivation The EPOLLOUT event is used to indicate fd writability following write buffer exhaustion. We currently update the epoll fd per channel on the fly to change the corresponding interest flag, but based on my (possibly incomplete) understanding it shouldn't be necessary in edge-triggered mode. We could just leave it set and avoid the extra epoll_ctl syscalls. Modifications In AbstractEpollChannel, set the EPOLLOUT flag from the outset and intercept existing calls to set/clear the EPOLLOUT channel flag when in ET mode, instead using a separate boolean to indicate that a flush is pending. This particular approach is a bit hacky imo but involves minimal changes to existing code. I mainly wanted to check the general idea is valid. Result Fewer syscalls on event loop when in ET mode (default).
|
@netty-bot test this please |
1 similar comment
|
@netty-bot test this please |
|
@normanmaurer I think this is still relevant since it could eliminate further So given this, yes no need for you to waste time reviewing :) Probably better to just do it when removing LT-related logic (if/when that's done, per #9349) since as mentioned the main value imo is the simplification of never having to set/clear it. |
|
@njhill IMHO we should close this one for now.... WDYT ? |
|
@normanmaurer sure, as mentioned we can reconsider it when removing LT mode! |
Motivation This is another go at netty#9347, which makes more sense now that LT is removed. Modifications Always set EPOLLOUT flag for sockets in the interest list and use a new boolean flushPending field in AbstractEpollChannel to track whether there is a flush pending. Result Fewer epoll_ctl syscalls on event loop in case of send backpressure
Motivation
The
EPOLLOUTevent is used to indicate fd writability following write buffer exhaustion. We currently update the epoll fd per channel on the fly to change the corresponding interest flag, but based on my (possibly incomplete) understanding it shouldn't be necessary in edge-triggered mode. We could just leave it set and avoid the extraepoll_ctlsyscalls.Modifications
In
AbstractEpollChannel, set theEPOLLOUTflag from the outset and intercept existing calls to set/clear it when in ET mode, instead using a separate boolean to indicate that a flush is pending.This particular approach is a bit hacky imo but involves minimal changes to existing code. I mainly wanted to check the general idea is valid.
Result
Fewer syscalls on event loop when in ET mode (default).