Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

Also support sendmmsg(...) on connected UDP channels when using nati… #9536

Merged
merged 1 commit into from Sep 6, 2019

Conversation

@normanmaurer
Copy link
Member

normanmaurer commented Sep 4, 2019

…ve epoll transport

Motivation:

We should also use sendmmsg on connected channels whenever possible to reduce the overhead of syscalls.

Modifications:

No matter if the channel is connected or not try to use sendmmsg when supported to reduce the overhead of syscalls

Result:

Better performance on connected UDP channels due less syscalls

@normanmaurer normanmaurer requested review from franz1981 and njhill Sep 4, 2019
@normanmaurer normanmaurer added this to the 4.1.40.Final milestone Sep 4, 2019

// Use UIO_MAX_IOV as this is the maximum number we can write with one sendmmsg(...) call.
private final NativeDatagramPacket[] packets = new NativeDatagramPacket[UIO_MAX_IOV];

// We share one IovArray for all NativeDatagramPackets to reduce memory overhead. This will allow us to write
// up to IOV_MAX iovec across all messages in one sendmmsg(...) call.
private final IovArray iovArray = new IovArray();

private final MyMessageProcessor processor = new MyMessageProcessor();

This comment has been minimized.

Copy link
@franz1981

franz1981 Sep 5, 2019

Contributor

why introducing this? The original solution was avoiding this pointer chasing

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 5, 2019

Author Member

@franz1981 because I did not want to create a new object all the time... or you say I should just still implement it directly and update the fields accordantly ?

This comment has been minimized.

Copy link
@franz1981

franz1981 Sep 5, 2019

Contributor

or you say I should just still implement it directly and update the fields accordantly

Excatly 👍

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 5, 2019

Author Member

I can do this but I think what we have is more elegant and clean... do you really think it matters ?

This comment has been minimized.

Copy link
@franz1981

franz1981 Sep 5, 2019

Contributor

Good question...I haven't measured so I cannot tell if is that relevant. but you risk to have this -> processor -> remote pointer chasing that would be saved by not allowing a mutable state (ie remote), although encapsulated in a different class.
NativeDatagramPacketArray is final and package private so I won't care that much to be that clean, if the intent is clear enough :P

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 5, 2019

Author Member

I think I would keep it like it is for now.. it just feels "cleaner" .

Copy link
Member

njhill left a comment

See comment inline (and sorry if I'm mistaken).

A separate but related thing I noticed, again I am not that familiar with the code so I may be missing something - the connected field of EpollDatagramChannel doesn't appear to be kept properly in sync with the local and remote address fields of the superclass.

In particular if the channel is created from an fd in active state, I think connected should be set to true in the constructor. More importantly, when disconnected the local and remote addresses should be set to null. Otherwise with the change in current form, the channel could continue to send packets to a remote address even after being disconnected from it.

@Override
public boolean processMessage(Object msg) {
return msg instanceof DatagramPacket && addReadable((DatagramPacket) msg);
void add(ChannelOutboundBuffer buffer, final InetSocketAddress remote) throws Exception {

This comment has been minimized.

Copy link
@njhill

njhill Sep 6, 2019

Member

@normanmaurer if I understand correctly, it would be better here to not pass a separate address but just a boolean to indicate whether connected, and if so override the remote address to be null in processMessage(...).

This would save overhead of copying the address for every packet in NativeDatagramPacket#init(...). You can send NULL address when connected, I think that also applies to sendmmsg

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 6, 2019

Author Member

let me try if this works... good point.

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Sep 6, 2019

See comment inline (and sorry if I'm mistaken).

A separate but related thing I noticed, again I am not that familiar with the code so I may be missing something - the connected field of EpollDatagramChannel doesn't appear to be kept properly in sync with the local and remote address fields of the superclass.

In particular if the channel is created from an fd in active state, I think connected should be set to true in the constructor.

I am not sure this is really true... I need to think about this and I think we can fix this as a followup.

More importantly, when disconnected the local and remote addresses should be set to null. Otherwise with the change in current form, the channel could continue to send packets to a remote address even after being disconnected from it.

Yes this sounds like a bug. I will open another PR for this with a unit test as it is not really related to this change.

@normanmaurer normanmaurer force-pushed the sendmmsg_connected branch from ab9359f to 05017c1 Sep 6, 2019
@@ -119,9 +119,9 @@ public boolean processMessage(Object msg) {
ByteBuf buf = packet.content();
return add0(buf, buf.readerIndex(), buf.readableBytes(), packet.recipient());

This comment has been minimized.

Copy link
@njhill

njhill Sep 6, 2019

Member

I think even here it makes sense to pass null if connected... since the address will be ignored IIUC

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 6, 2019

Author Member

I don't think this is true... even on connected channels you can send to other addresses IIUC.

This comment has been minimized.

Copy link
@njhill

njhill Sep 6, 2019

Member

I'm not sure how the syscalls behave but java DatagramChannel does not allow it, and the NIO impl will ignore other addresses if connected: https://github.com/netty/netty/blob/4.1/transport/src/main/java/io/netty/channel/socket/nio/NioDatagramChannel.java#L291

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Sep 6, 2019

Author Member

I am a bit confused... in the code you linked we will always pass the remoteAddress of the DatagramPacket if there is one even if we are connected. So it is inline with what we have here now... Also I tested what happens if I set something invalid in native code for the addr and it failed the syscall. So it seems like it is not ignored (like I suspected).

This comment has been minimized.

Copy link
@njhill

njhill Sep 6, 2019

Member

Oops :) you are right sorry I misread the remoteAddress there as being the channel's address rather than the packet's address ... but this won't actually work if you attempt it. So the NIO impl is kind of broken in this regard

Caused by: java.lang.IllegalArgumentException: Connected address not equal to target address
	at sun.nio.ch.DatagramChannelImpl.send(DatagramChannelImpl.java:448)
	at io.netty.channel.socket.nio.NioDatagramChannel.doWriteMessage(NioDatagramChannel.java:292)

but I think you're right that the kernel allows it so I guess fine to leave as-is

@normanmaurer normanmaurer requested a review from njhill Sep 6, 2019
@njhill
njhill approved these changes Sep 6, 2019
Copy link
Member

njhill left a comment

LGTM!

buffer.forEachFlushedMessage(processor);
} finally {
processor.connected = false;
}

This comment has been minimized.

Copy link
@njhill

njhill Sep 6, 2019

Member

not sure resetting this is really needed now since it's always set before use?

@normanmaurer

This comment has been minimized.

Copy link
Member Author

normanmaurer commented Sep 6, 2019

@njhill #9545 should address your other comment

@njhill
njhill approved these changes Sep 6, 2019
@normanmaurer normanmaurer force-pushed the sendmmsg_connected branch from 0444d15 to 69e5bf4 Sep 6, 2019
…e epoll transport

Motivation:

We should also use sendmmsg on connected channels whenever possible to reduce the overhead of syscalls.

Modifications:

No matter if the channel is connected or not try to use sendmmsg when supported to reduce the overhead of syscalls

Result:

Better performance on connected UDP channels due less syscalls
@normanmaurer normanmaurer force-pushed the sendmmsg_connected branch from 69e5bf4 to f8a4ccd Sep 6, 2019
@normanmaurer normanmaurer merged commit 7b7f319 into 4.1 Sep 6, 2019
3 checks passed
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 sendmmsg_connected branch Sep 6, 2019
normanmaurer added a commit that referenced this pull request Sep 6, 2019
…e epoll transport (#9536)

Motivation:

We should also use sendmmsg on connected channels whenever possible to reduce the overhead of syscalls.

Modifications:

No matter if the channel is connected or not try to use sendmmsg when supported to reduce the overhead of syscalls

Result:

Better performance on connected UDP channels due less syscalls
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.