Skip to content

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

Merged
normanmaurer merged 1 commit intonetty:4.1from
franz1981:epoll_snd_rcv
Aug 22, 2022
Merged

Replace stdlib write/read with send/recv (Fixes #12673)#12679
normanmaurer merged 1 commit intonetty:4.1from
franz1981:epoll_snd_rcv

Conversation

@franz1981
Copy link
Copy Markdown
Contributor

@franz1981 franz1981 commented Aug 8, 2022

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
Copy Markdown
Contributor Author

franz1981 commented Aug 8, 2022

@normanmaurer waiting the CI :)

normanmaurer
normanmaurer reviewed Aug 8, 2022
View reviewed changes
Comment thread transport-native-unix-common/src/main/c/netty_unix_filedescriptor.c Outdated
@franz1981 franz1981 marked this pull request as draft August 8, 2022 11:02
@franz1981
Copy link
Copy Markdown
Contributor Author

franz1981 commented Aug 8, 2022

@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
normanmaurer
normanmaurer requested changes Aug 8, 2022
View reviewed changes
Comment thread transport-native-unix-common/src/main/c/netty_unix_filedescriptor.c Outdated
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/LinuxSocket.java Outdated
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/LinuxSocket.java Outdated
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/LinuxSocket.java Outdated
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/LinuxSocket.java Outdated
@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
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/AbstractEpollStreamChannel.java Outdated
Copy link
Copy Markdown
Contributor Author

@franz1981 franz1981 Aug 8, 2022 •
edited
Loading

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
Copy Markdown
Contributor Author

@franz1981 franz1981 Aug 10, 2022

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
Copy Markdown
Contributor Author

franz1981 commented Aug 8, 2022

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
Copy Markdown
Member

normanmaurer commented Aug 8, 2022

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
Copy Markdown
Member

normanmaurer commented Aug 10, 2022

@franz1981 let me know once I should check again

@franz1981
Copy link
Copy Markdown
Contributor Author

franz1981 commented Aug 10, 2022

@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
Copy Markdown
Member

normanmaurer commented Aug 10, 2022

@franz1981 please add the numbers here

normanmaurer
normanmaurer requested changes Aug 10, 2022
View reviewed changes
Comment thread transport-classes-epoll/src/main/java/io/netty/channel/epoll/LinuxSocket.java Outdated
@franz1981
Copy link
Copy Markdown
Contributor Author

franz1981 commented Aug 11, 2022 •
edited
Loading

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
Copy Markdown
Member

normanmaurer commented Aug 12, 2022

Also /cc @Lukasa for visibility

@normanmaurer
Copy link
Copy Markdown
Member

normanmaurer commented Aug 12, 2022

/cc @trustin

@Lukasa
Copy link
Copy Markdown
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
Copy Markdown
Member

normanmaurer commented Aug 13, 2022

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

@franz1981
Copy link
Copy Markdown
Contributor Author

franz1981 commented Aug 15, 2022

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
Copy Markdown
Contributor Author

franz1981 commented Aug 20, 2022

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

@franz1981
Copy link
Copy Markdown
Contributor Author

franz1981 commented Aug 22, 2022

@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
Copy Markdown
Member

normanmaurer commented Aug 22, 2022

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.

3 participants

@franz1981 @normanmaurer @Lukasa