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

Don't read from timerfd and eventfd on each EventLoop tick #9192

Merged
merged 1 commit into from May 31, 2019

Conversation

@normanmaurer
Copy link
Member

commented May 28, 2019

Motivation:

We do not need to issue a read on timerfd and eventfd when the EventLoop wakes up if we register these as Edge-Triggered. This removes the overhead of 2 syscalls and so helps to reduce latency.

Modifications:

  • Ensure we register the timerfd and eventfd with EPOLLET flag
  • If eventfd_write fails with EAGAIN, call eventfd_read and try eventfd_write again as we only use it as wake-up mechanism.

Result:

Less syscalls and so reducing overhead.

Co-authored-by: Carl Mastrangelo carl@carlmastrangelo.com

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

/cc @johnou

@carl-mastrangelo
Copy link
Member

left a comment

One nit, but LGTM.

Don't read from timerfd and eventfd on each EventLoop tick
Motivation:

We do not need to issue a read on timerfd and eventfd when the EventLoop wakes up if we register these as Edge-Triggered. This removes the overhead of 2 syscalls and so helps to reduce latency.

Modifications:

- Ensure we register the timerfd and eventfd with EPOLLET flag
- If eventfd_write fails with EAGAIN, call eventfd_read and try eventfd_write again as we only use it as wake-up mechanism.

Result:

Less syscalls and so reducing overhead.

Co-authored-by: Carl Mastrangelo <carl@carlmastrangelo.com>

@normanmaurer normanmaurer force-pushed the eventfd_timerfd branch from ddbc875 to 4d83585 May 28, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member Author

commented May 28, 2019

@netty-bot test this please (test failure unrelated)

@njhill

njhill approved these changes May 28, 2019

Copy link
Member

left a comment

LGTM, though I'm not an epoll syscall expert

@franz1981
Copy link
Contributor

left a comment

Nice one!

@normanmaurer normanmaurer self-assigned this May 28, 2019

@normanmaurer normanmaurer requested a review from ejona86 May 30, 2019

netty_unix_errors_throwChannelExceptionErrorNo(env, "eventfd_write() failed: ", errno);
uint64_t val;

for (;;) {

This comment has been minimized.

Copy link
@johnou

johnou May 30, 2019

Contributor

is it possible to get into some type of weird busy loop here?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 31, 2019

Author Member

nope...

This comment has been minimized.

Copy link
@johnou

johnou May 31, 2019

Contributor

ok so eventfd_write isn't going to continuously return -1 and eventfd_read 0.

@normanmaurer normanmaurer merged commit f6cf681 into 4.1 May 31, 2019

3 checks passed

pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details

@normanmaurer normanmaurer deleted the eventfd_timerfd branch May 31, 2019

normanmaurer added a commit that referenced this pull request May 31, 2019

Don't read from timerfd and eventfd on each EventLoop tick (#9192)
Motivation:

We do not need to issue a read on timerfd and eventfd when the EventLoop wakes up if we register these as Edge-Triggered. This removes the overhead of 2 syscalls and so helps to reduce latency.

Modifications:

- Ensure we register the timerfd and eventfd with EPOLLET flag
- If eventfd_write fails with EAGAIN, call eventfd_read and try eventfd_write again as we only use it as wake-up mechanism.

Result:

Less syscalls and so reducing overhead.

Co-authored-by: Carl Mastrangelo <carl@carlmastrangelo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.