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

Remove edge-triggered support for epoll and just always use level-tri… #14031

Merged
merged 3 commits into from May 3, 2024

Conversation

normanmaurer
Copy link
Member

…ggered

Motivation:

We supported edge-triggered and level-triggered modes for our epoll transport. This made things more complex while not really providing much value.

Modifications:

  • Remove edge-triggered support and just use level-triggered all the time

Result:

Less complexity. Fixes #14007

…ggered

Motivation:

We supported edge-triggered and level-triggered modes for our epoll transport. This made things more complex while not really providing much value.

Modifications:

- Remove edge-triggered support and just use level-triggered all the time

Result:

Less complexity. Fixes #14007
Copy link
Contributor

@bryce-anderson bryce-anderson left a comment

Choose a reason for hiding this comment

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

That is a nice cleanup but I wonder why was ET ever added if there is no benefit? In my minds eye there are less system calls under some scenarios but it's also possible that we just never hit those in practice.

Comment on lines +568 to +570
if (shouldStopReading(config)) {
clearEpollIn();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, this whole if block is quite repetitive so maybe it would be a good candidate for a new definition of epollInFinally(Config).

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it but I found it more readable this way :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough reason. It's not a lot of code in any case.

@normanmaurer
Copy link
Member Author

That is a nice cleanup but I wonder why was ET ever added if there is no benefit? In my minds eye there are less system calls under some scenarios but it's also possible that we just never hit those in practice.

I added it because in theory it should be less expensive. That said it not really gains us much because of how netty works. We still use ET for eventfd and timerfd tho.

@normanmaurer normanmaurer merged commit dacd5c0 into 4.2 May 3, 2024
16 of 17 checks passed
@normanmaurer normanmaurer deleted the remove_edge branch May 3, 2024 19:46
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.

None yet

2 participants