Improve polling for available file descriptors

[Hemofektik]

The polling of all file descriptors can take a lot of time especially on modern OSs where each process can open 1<<20 FDs.
This PR addresses this issue and stops polling for FDs once the desired number of available FDs has been found.
The heap memory needed for holding the FDs is also much smaller and no longer persistent throughout the lifetime of the XmlRpcServer.

TL;DR

• Remove 8MB of memory for each socket created
• Reduce available file descriptor polling to minimum necessary
  • roughly a 1000 times faster
  • old duration: 34'571'539 ns
  • new duration: 34'464 ns

[peci1]

This looks interesting.

For the PR to proceed, you should revert the whitespace-only changes and force-push a new commit without them.

How did you measure the time? What is the influence on a real running system?

[Hemofektik]

For the PR to proceed, you should revert the whitespace-only changes and force-push a new commit without them.

I will do that thanks.

How did you measure the time? What is the influence on a real running system?

I saw this issue actually using nsight profiler when one of our nodes tried to create around 50 subscribers/publishers which takes forever. I then checked this code piece individually and measured its duration. 35 ms is not much for a single pub/sub but it can add up. So this change could save us 1-2 seconds in some nodes of startup time. This is probably also visible when using nodelets heavily.

[peci1]

Interesting. Would it be possible to write a simple script (pair of scripts?) that would demonstrate this?

[Hemofektik]

Interesting. Would it be possible to write a simple script (pair of scripts?) that would demonstrate this?

You mean as part of the tests? I could try to add a test that is calling this code and measures it.
In my particular case I copied the enoughFreeFDs() method to my test program to measure it individually before and after.

[peci1]

Probably not as part of unit tests. The buildfarm performance varies too wildly to perform any kind of performance testing that would be able to measure such differences.

I meant something we could use manually when reviewing the PR.

[Hemofektik]

I meant something we could use manually when reviewing the PR.

Here is a hybrid of the original code and the final code in the PR I used for validating this.
fd_test.zip
You can replace the content of enoughFreeFDs() with the original parts from the ros_comm to see its original performance. You need to change the initFDs() function then also to resize the vector to the original 8 MB size to capture the 1<<20 FDs.

[peci1]

Oh, and you definitely can't remove anything from header files. That would change API and ABI which is forbidden for released ROS versions.

[Hemofektik]

Fixed

[peci1]

Hmm, I think you should drop the local poll_fds variable and use the one from the header instead. It is a protected variable, so there might be downstream code expecting to find some FDs in it. Would that make sense?

[Hemofektik]

Oh you are right, it is a protected member. Will fix it.

[Hemofektik]

fixed

[Hemofektik]

no not yet

[Hemofektik]

now it should be correct

[Hemofektik]

But actually right now it is not used to hold available file descriptors as this can also change over time

[Hemofektik]

Although this member is protected and could in theory be used in successor classes it would not make any sense. These FDs would not be usable in any way. There is no guarantee that they are still available later. Their only purpose was to count the number of available FDs at the time of creation.
This code piece was very questionable from the beginning. The last change to it was supposed to do exactly what it does now with this PR but actually missed the point.

[peci1]

You're right, the possibility of using this member is so low that I don't think it is worth it doing anything else...

[peci1]

I've tested the file you sent and indeed the proposed change makes it run much faster with higher ulimit -n values.

Relevant issues and PRs mentioned that most systems have this limit set to 1024, so the speedup would not be noticeable. But in docker, the limit is 1048576 by default and there it starts to be visible.

I'm still thinking about how to create a real-world (but self-contained and minimal) ROS testcase that would showcase the problem using roscpp. Would you be able to adjust the old example from https://github.com/peci1/ros_infinite_loop/tree/docker to demonstrate the problem?

[Hemofektik]

Would you be able to adjust the old example from https://github.com/peci1/ros_infinite_loop/tree/docker to demonstrate the problem?

Sure, but it would take some days until I have some spare time available.

[peci1]

I'm nevertheless not a maintainer of this repo, so don't take my comments as something required. I just try to help getting this PR in a form that would be easy to grasp for maintainers so that it could eventually get merged. There's not much maintainership effort left for ROS 1, so the official maintainers need this kind of community help.

[peci1]

I've tried running our teaching autonomous exploration pipeline with Trutlebot in Gazebo with this PR. Everything works as I'm used to. So at least practically, it seems be okay!