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.

Sign up for GitHub

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

Jump to bottom

Replace stdlib write/read with send/recv (Fixes #12673) #12679

Merged
merged 1 commit into from Aug 22, 2022
Merged

Replace stdlib write/read with send/recv (Fixes #12673) #12679

merged 1 commit into from Aug 22, 2022

Conversation

franz1981
Copy link
Contributor

Motivation:

The performance Unix write/read paths is more involved (and slower) then the specialized socket send/rcv ones.

Modification:

Replace Unix write/read paths with send/recv

Result:

Better performance for single buffer send/recv

@franz1981 franz1981 self-assigned this Aug 8, 2022
@franz1981
Copy link
Contributor Author

@normanmaurer waiting the CI :)

@franz1981 franz1981 marked this pull request as draft August 8, 2022 11:02
@franz1981
Copy link
Contributor Author

@normanmaurer comments are welcome, undrafting

@franz1981 franz1981 marked this pull request as ready for review August 8, 2022 12:35
@franz1981 franz1981 force-pushed the epoll_snd_rcv branch 3 times, most recently from 2e647e1 to b46aa8c Compare August 8, 2022 12:54
@franz1981 franz1981 force-pushed the epoll_snd_rcv branch 2 times, most recently from 0370cae to 302a144 Compare August 8, 2022 13:08
franz1981
franz1981 commented Aug 8, 2022
View reviewed changes
transport-classes-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java

@Override
protected int write(final ByteBuffer buf, final int pos, final int limit) throws IOException {
return socket.send(buf, pos, limit);
Copy link
Contributor Author

@franz1981 franz1981 Aug 8, 2022 •
edited

Choose a reason for hiding this comment

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

That's an hack because SocketWritableByteChannel is a public API and I cannot introduce a generic param T extends FileDescriptor on it @normanmaurer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't like this trick @normanmaurer :( I feel dirty

@franz1981 franz1981 force-pushed the epoll_snd_rcv branch 2 times, most recently from ca160bd to 702e3c5 Compare August 8, 2022 14:45
@franz1981
Copy link
Contributor Author

I've added some protected final method to Socket and the checker complain I've modified a public API :(
I'm forced to move send/recv in the epoll socket c file @normanmaurer ?
I would like to keep it on unix socket, in case we want to port this to kqueue (after being tested)

@normanmaurer
Copy link
Member

I've added some protected final method to Socket and the checker complain I've modified a public API :(
I'm forced to move send/recv in the epoll socket c file @normanmaurer ?
I would like to keep it on unix socket, in case we want to port this to kqueue (after being tested)

just remove final

@normanmaurer
Copy link
Member

@franz1981 let me know once I should check again

@franz1981
Copy link
Contributor Author

@normanmaurer fingers crossed!
I'll run some bench on our lab in the afternoon (although using the full quarkus stack ^^)

@normanmaurer normanmaurer added this to the 4.1.80.Final milestone Aug 10, 2022
@normanmaurer
Copy link
Member

@franz1981 please add the numbers here

@franz1981
Copy link
Contributor Author

franz1981 commented Aug 11, 2022 •
edited

The firsts load tests using just the receive side (ie read vs recv) shows this improvement:
~4_200_000 requests/sec vs ~4_700_000 requests/sec

The test used has been Techempower plaintext with quarkus (meaning a full-stack, not just netty) using

wrk -H 'Host: 192.168.0.106' -H 'Accept: text/plain,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7' -H 'Connection: keep-alive' --latency -d 15 -c 256 --timeout 8 -t 64 http://192.168.0.106:8080/plaintext -s pipeline.lua -- 16

vs the full reactive stack.
Due to pipelining, the write side is likely using writev hence the improvement is just due to the recv side change

@normanmaurer
Copy link
Member

Also /cc @Lukasa for visibility

@normanmaurer
Copy link
Member

/cc @trustin

@Lukasa
Copy link
Contributor

Lukasa commented Aug 13, 2022

Interesting, where's the data coming from to suggest that send outperforms write? Relatedly, does the same thing apply to sendmsg outperforming writev?

@normanmaurer
Copy link
Member

@Lukasa the issue has some more details as well #12673

@franz1981
Copy link
Contributor Author

Consider @Lukasa that the most benefit come by the different specialization of the syscalls: send/recv are indeed for socket fds, while write/read can be used for other types of fds
The mentioned libreactor issue better explain the involved kernel code paths

@franz1981
Replace stdlib write/read with send/recv (Fixes netty#12673)
d490a93 
Motivation:

The performance Unix write/read paths is more involved (and slower) then the specialized socket send/rcv ones.

Modification:

Replace Unix write/read paths with send/recv

Result:

Better performance for single buffer send/recv
@franz1981
Copy link
Contributor Author

@normanmaurer given I've modified a socket public API I'm prepared to some failure :(
but the comments are addressed :)

@franz1981
Copy link
Contributor Author

@normanmaurer re #12679 (comment) I was wrong, so, ready to go!

@normanmaurer normanmaurer merged commit 4292669 into netty:4.1 Aug 22, 2022
@normanmaurer
Copy link
Member

Thanks a lot!

normanmaurer pushed a commit that referenced this pull request Aug 22, 2022
@franz1981 @normanmaurer
Replace stdlib write/read with send/recv (Fixes #12673) (#12679)
8e72919 
Motivation:

The performance Unix write/read paths is more involved (and slower) then the specialized socket send/rcv ones.

Modification:

Replace Unix write/read paths with send/recv

Result:

Better performance for single buffer send/recv
normanmaurer added a commit that referenced this pull request Aug 22, 2022
@normanmaurer @franz1981
Replace stdlib write/read with send/recv (Fixes #12673) (#12679) (#12724
73073a9 
)

Motivation:

The performance Unix write/read paths is more involved (and slower) then the specialized socket send/rcv ones.

Modification:

Replace Unix write/read paths with send/recv

Result:

Better performance for single buffer send/recv

Co-authored-by: Francesco Nigro <nigro.fra@gmail.com>
@lepistone lepistone mentioned this pull request Dec 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanmaurer normanmaurer normanmaurer approved these changes

@chrisvest chrisvest Awaiting requested review from chrisvest

Assignees

@franz1981 franz1981

Labels
improvement
Projects
None yet
Milestone
4.1.80.Final
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@franz1981 @normanmaurer @Lukasa