Adding TCP-NO-DELAY transport hint #295

StefanGlaser · 2019-04-03T15:41:28Z

This pull request introduces transport hints to specify the TCP-NO-DELAY option when creating a new subscriber. Furthermore, it also processes the TCP-NO-DELAY header field for incoming subscriber connections and sets this option on the corresponding channel.

For testing purposes I've modified the pubsub tutorial to exchange messages in 1000Hz. I've tested rosjava (subscriber) to rosjava (publisher) as well as rosjava (subscriber) to rospy (publisher). The roscore and subscriber were running on my PC and the publisher on a RasPi 3b, connected via a dedicated router. The transmitted messages were monitored in Wireshark. Both tests worked as expected.

Wireshark results:

TCP-NO-DELAY on
TCP-NO-DELAY off

The packets in the second screenshot that are not present in the first screenshot are packets that contain more than one ros message.

As there exists a connection buffer / pool in rosjava, I'm not 100% sure what happens if there are two subscribers to the same topic within a rosjava process, one with TCP-NO-DELAY disabled and one with the option enabled. My current expectation is that the first connection that is established decides if TCP-NO-DELAY is enabled or disabled for the corresponding channel. I didn't include the transport hints within the equals method of the TopicDeclaration class, as I wasn't sure what effects this will imply.

This pull request solves issue #292.

* Added TransportHints class, currently only containing a TCP-NO-DELAY option. * Added option to specify transport hints when subscribing to a topic. * Added transport hints to TopicDeclaration class.

* Check incoming subscriber connection requests for the TCP-NO-DELAY header field and set the TCP-NO-DELAY option on the corresponding channel accordingly.

googlebot · 2019-04-03T15:41:32Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

StefanGlaser · 2019-04-03T18:41:37Z

I signed it!

googlebot · 2019-04-03T18:41:40Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

StefanGlaser · 2019-04-03T19:02:10Z

...even though I don't like it very much. Or maybe I don't understand it...

jubeira

Really cool @StefanGlaser!

I'd like to give it a shot before approving and merging, but overall it looks good!
By any chance have you tested this with cross-language publishers / subscribers? I'd like to verify that this feature works e.g. with rospy or roscpp as well to double check.

StefanGlaser · 2019-04-03T22:14:28Z

Thanks @jubeira! I would also like to take the opportunity to thank you all for this project!

No hurry, take your time for testing.

Regarding cross-language support:
I've successfully tested rosjava (listener) in combination with rospy (publisher) as mentioned above. The messages transmitted during a subscription contain the tcp_nodelay key as can be seen in the Wireshark screenshots. So I expect this patch to be compatible to roscpp and rospy. Still, I'm looking forward to run some additional tests, as I'm also interested to see what happens if I have two subscribers to the same topic within one rosjava process.

jubeira · 2019-04-04T13:35:41Z

I've successfully tested rosjava (listener) in combination with rospy (publisher) as mentioned above.

Great! Sorry, I think I missed that part the first time I read the post.
I'll try to get this tested before EOW. Thanks again!

jubeira

First pass done!
I've left a few comments: some minor nitpicks, and some details on TransportHints; other than that it looks good.

I ran the test with both publisher and subscriber in the same system and I noticed no difference when using TCP_NODELAY. I guess that's expected anyways; I'll try with two different systems later.

jubeira · 2019-04-05T14:14:43Z

rosjava/src/main/java/org/ros/node/topic/TransportHints.java

+
+  Map<String, String> options;
+
+ public TransportHints() {


nitpick: please indent functions and code with two spaces (same applies for next line).

jubeira · 2019-04-05T14:15:48Z

rosjava/src/main/java/org/ros/node/topic/TransportHints.java

+
+  public TransportHints tcpNoDelay(boolean nodelay) {
+    options.put(ConnectionHeaderFields.TCP_NODELAY, nodelay ? "1" : "0");
+


nitpick: remove newline.

jubeira · 2019-04-05T14:22:47Z

rosjava/src/main/java/org/ros/node/topic/TransportHints.java

+  }
+
+  public TransportHints(boolean tcpNoDelay) {
+    tcpNoDelay(tcpNoDelay);


This constructor yields a NullPointerException when called, as the default constructor is not called and therefore the map is not created.

Please consider either:

Calling the default constructor first.

Moving the initialization of the map to an instance initializer.

Initialize options inline (i.e. Map<String, String> options = Maps.newConcurrentMap<String, String>();).

I know in Java 7 it's not necessary to specify <String, String> twice, but please do so so as to make it easier to compile rosjava for older Java versions.

jubeira · 2019-04-05T14:24:41Z

rosjava/src/main/java/org/ros/node/topic/TransportHints.java

+ */
+public class TransportHints {
+
+  Map<String, String> options;


nit: add private access specifier.

* Replaced internal map with simple boolean. * Fixed initialization bug. * Fixed code style.

StefanGlaser · 2019-04-07T12:23:49Z

Thanks a lot for your feedback!
Sorry for all the inconveniences. I'm a bit confused by the number of mistakes I committed...

I've addressed your points in a new commit, in which I also replaced the internal map of the TransportHints class with a simple boolean attribute. This reduces the class dependencies and is a much more elegant solution to the given task in my opinion.

Great to hear that first tests didn't show any problems. The effect of TCP_NODELAY is very different depending on your setup. Even in my distributed setup I only get a couple of combined messages every now and then. But it's not only about the fact that multiple messages may be transmitted in one tcp packet. It's also about internal transmission delays. TCP_NODELAY should reduce the overall delay between the write call and the actual transmission of the corresponding tcp packet. However, checking for multiple messages transmitted in one tcp packet helps verifying that TCP_NODELAY works.

jubeira · 2019-04-09T19:12:14Z

Sorry for all the inconveniences. I'm a bit confused by the number of mistakes I committed...

No worries! That's what the reviews are for ;) thanks for addressing everything!
I'll give it a shot tomorrow using two different systems once more; I don't have the hardware with me right now. Codewise it looks good, so I'll likely get this approved and merged this week.

Thanks again @StefanGlaser!

jubeira

LGTM!
I've just tested the patch in two different systems and indeed, the new option generates the effect described above.

Just for the record, I noticed that if the hints between publisher and subscriber don't match, the Java subscriber is the option that prevails (tested with rospy publisher). In other words:

Java subscriber `tcpnodelay`	Python publisher `tcpnodelay`	Nodelay effect verified in Wireshark
No	No	No
No	Yes	No
Yes	No	Yes
Yes	Yes	Yes

jubeira · 2019-04-10T16:58:34Z

@StefanGlaser do you need a binary / Maven release with this change, or can you wait a little longer building from sources?

StefanGlaser · 2019-04-12T07:15:52Z

Yeah, reviews are essential and we just approved it again - unfortunately ;)

Great work on checking the effect of TCP_NODELAY. The results look fine and as expected. However, you only checked one part of it. You didn't check if rosjava handles the flag for incoming subscriber connections, in case the publisher is written in rosjava. This was the mistake I did when falsely claiming rosjava already respects the TCP_NODELAY flag.

I'm looking forward to switch our project to maven, soon. So in theory yes, a new release would be nice. However, I would prefer to help getting a rosjava release for melodic done, as it may also help others and shows that this project is still active.

jubeira · 2019-04-12T11:49:50Z

However, you only checked one part of it. You didn't check if rosjava handles the flag for incoming subscriber connections, in case the publisher is written in rosjava. This was the mistake I did when falsely claiming rosjava already respects the TCP_NODELAY flag.

Yes, that's right, but in that sense I trust the tests you did before.

I'm looking forward to switch our project to maven, soon. So in theory yes, a new release would be nice. However, I would prefer to help getting a rosjava release for melodic done, as it may also help others and shows that this project is still active.

I'm on it! I need some more time to bump all the necessary versions and dependencies, which is a bit tedious and maybe a bit error-prone. If you are willing to volunteer, I might ping you to give it a shot once everything is on its place. Does that sound good?

StefanGlaser · 2019-04-25T17:36:45Z

Hey, sry, totally forgot to answer...

That sounds great! I'm happy to volunteer. Feel free to ping me whenever you need.

StefanGlaser added 2 commits April 3, 2019 14:50

Added option to specify transport hints.

ba3dd3e

* Added TransportHints class, currently only containing a TCP-NO-DELAY option. * Added option to specify transport hints when subscribing to a topic. * Added transport hints to TopicDeclaration class.

Process TCP-NO-DELAY option for incoming subscriber connections.

7ae94f6

* Check incoming subscriber connection requests for the TCP-NO-DELAY header field and set the TCP-NO-DELAY option on the corresponding channel accordingly.

googlebot added the cla: no label Apr 3, 2019

googlebot added cla: yes and removed cla: no labels Apr 3, 2019

jubeira reviewed Apr 3, 2019

View reviewed changes

jubeira suggested changes Apr 5, 2019

View reviewed changes

Simplified TransportHints class.

391a0a9

* Replaced internal map with simple boolean. * Fixed initialization bug. * Fixed code style.

jubeira approved these changes Apr 10, 2019

View reviewed changes

jubeira merged commit 111c1d1 into rosjava:kinetic Apr 10, 2019

jacknlliu mentioned this pull request Aug 24, 2019

using TCP_NODELAY transport hint IFL-CAMP/iiwa_stack#218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding TCP-NO-DELAY transport hint #295

Adding TCP-NO-DELAY transport hint #295

StefanGlaser commented Apr 3, 2019

googlebot commented Apr 3, 2019

StefanGlaser commented Apr 3, 2019

googlebot commented Apr 3, 2019

StefanGlaser commented Apr 3, 2019

jubeira left a comment

StefanGlaser commented Apr 3, 2019

jubeira commented Apr 4, 2019

jubeira left a comment

jubeira Apr 5, 2019

jubeira Apr 5, 2019

jubeira Apr 5, 2019

jubeira Apr 5, 2019

StefanGlaser commented Apr 7, 2019

jubeira commented Apr 9, 2019

jubeira left a comment

jubeira commented Apr 10, 2019

StefanGlaser commented Apr 12, 2019

jubeira commented Apr 12, 2019

StefanGlaser commented Apr 25, 2019


		public TransportHints tcpNoDelay(boolean nodelay) {
		options.put(ConnectionHeaderFields.TCP_NODELAY, nodelay ? "1" : "0");

Adding TCP-NO-DELAY transport hint #295

Adding TCP-NO-DELAY transport hint #295

Conversation

StefanGlaser commented Apr 3, 2019

googlebot commented Apr 3, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

StefanGlaser commented Apr 3, 2019

googlebot commented Apr 3, 2019

StefanGlaser commented Apr 3, 2019

jubeira left a comment

Choose a reason for hiding this comment

StefanGlaser commented Apr 3, 2019

jubeira commented Apr 4, 2019

jubeira left a comment

Choose a reason for hiding this comment

jubeira Apr 5, 2019

Choose a reason for hiding this comment

jubeira Apr 5, 2019

Choose a reason for hiding this comment

jubeira Apr 5, 2019

Choose a reason for hiding this comment

jubeira Apr 5, 2019

Choose a reason for hiding this comment

StefanGlaser commented Apr 7, 2019

jubeira commented Apr 9, 2019

jubeira left a comment

Choose a reason for hiding this comment

jubeira commented Apr 10, 2019

StefanGlaser commented Apr 12, 2019

jubeira commented Apr 12, 2019

StefanGlaser commented Apr 25, 2019