Improve polling for available file descriptors #2365

Hemofektik · 2023-12-14T18:02:40Z

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 · 2024-02-13T15:30:11Z

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 · 2024-02-13T15:34:49Z

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

Hemofektik · 2024-02-13T15:40:39Z

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 · 2024-02-13T16:52:51Z

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 · 2024-02-13T17:28:14Z

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 · 2024-02-13T17:38:54Z

utilities/xmlrpcpp/include/xmlrpcpp/XmlRpcServer.h

@@ -125,10 +125,6 @@ namespace XmlRpc {

    // Minimum number of free file descriptors before rejecting clients.
    static const int FREE_FD_BUFFER;
-#ifndef _WINDOWS


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

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?

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

now it should be correct

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

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.

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

- 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 · 2024-02-14T13:48:19Z

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 · 2024-02-14T14:22:07Z

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 · 2024-02-14T14:28:24Z

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 · 2024-02-14T14:38:17Z

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!

peci1 suggested changes Feb 13, 2024

View reviewed changes

peci1 reviewed Feb 13, 2024

View reviewed changes

Hemofektik force-pushed the noetic-devel branch 3 times, most recently from f09e5d0 to b439358 Compare February 13, 2024 18:24

Improve polling for available file descriptors

2582923

- 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

Hemofektik force-pushed the noetic-devel branch from b439358 to 2582923 Compare February 13, 2024 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve polling for available file descriptors #2365

Improve polling for available file descriptors #2365

Hemofektik commented Dec 14, 2023

peci1 left a comment

Hemofektik commented Feb 13, 2024 •

edited

Loading

peci1 commented Feb 13, 2024

Hemofektik commented Feb 13, 2024

peci1 commented Feb 13, 2024

Hemofektik commented Feb 13, 2024

peci1 Feb 13, 2024

Hemofektik Feb 13, 2024

peci1 Feb 13, 2024 •

edited

Loading

Hemofektik Feb 13, 2024

Hemofektik Feb 13, 2024

Hemofektik Feb 13, 2024

Hemofektik Feb 13, 2024

Hemofektik Feb 13, 2024

Hemofektik Feb 14, 2024

peci1 Feb 14, 2024

peci1 commented Feb 14, 2024

Hemofektik commented Feb 14, 2024

peci1 commented Feb 14, 2024 •

edited

Loading

peci1 commented Feb 14, 2024

Improve polling for available file descriptors #2365

Are you sure you want to change the base?

Improve polling for available file descriptors #2365

Conversation

Hemofektik commented Dec 14, 2023

peci1 left a comment

Choose a reason for hiding this comment

Hemofektik commented Feb 13, 2024 • edited Loading

peci1 commented Feb 13, 2024

Hemofektik commented Feb 13, 2024

peci1 commented Feb 13, 2024

Hemofektik commented Feb 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peci1 Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peci1 commented Feb 14, 2024

Hemofektik commented Feb 14, 2024

peci1 commented Feb 14, 2024 • edited Loading

peci1 commented Feb 14, 2024

Hemofektik commented Feb 13, 2024 •

edited

Loading

peci1 Feb 13, 2024 •

edited

Loading

peci1 commented Feb 14, 2024 •

edited

Loading