Topic subscription scalability fix(#1078) by guillaumeautran · Pull Request #12 · clearpathrobotics/ros_comm

guillaumeautran · 2017-07-25T16:49:17Z

Improve performance of topic socket polling.

mikepurvis · 2017-07-25T17:46:45Z

clients/roscpp/src/libros/io.cpp

 #else
  #include <cstring> // strerror
  #include <fcntl.h> // for non-blocking configuration
+  #include <sys/epoll.h>


This is going to cause trouble on POSIX platforms where epoll isn't a thing, especially OS X and BSD. I think you'll need to test for the header in CMake and then have an #ifdef that only optionally includes it.

You can do this easily with CMake's check_include_file macro, see: https://cmake.org/cmake/help/v3.5/module/CheckIncludeFile.html

Does that also mean we need to support all three implementation? Windows / poll / epoll ?
If that's the case, I'd much rather have an implementation file per OS.
io_win.cpp / io_posix.cpp / io_epoll.cpp.
That way, implementation file can be neat and clean (could be all in one file as well though.)

Summoning @dirk-thomas for an opinion on this.

I am not opposed to any of the options. But I would think that maintaining the current pattern of keeping the implementations in the same file will likely be easier to review. If you decide to split the implementation please make sure to use separate commits for the following steps:

copy existing file to new files without any changes

remove redundant parts

make the code work again from the new files

This will ensure that the git history is easy to follow.

mikepurvis · 2017-07-25T17:50:15Z

clients/roscpp/src/libros/io.cpp

+    ROS_ERROR("Unable to add FD to epoll: %s", strerror(errno));
+  }
+#endif
+}


I haven't fully digested all of what's going on here, but this if/else stuff going on everywhere feels like it's going to be a big pain to maintain. Should we be contemplating supplying multiple distinct implementations (epoll, poll, windows), and using a single branch somewhere to select which one is used?

I followed the existing model doing my best not to disrupt too much. But I agree, a single branch is likely best.

mikepurvis · 2017-07-25T17:50:44Z

clients/roscpp/src/libros/io.cpp

 */
-int poll_sockets(socket_pollfd *fds, nfds_t nfds, int timeout) {
+boost::shared_ptr<std::vector<socket_pollfd> > poll_sockets(int epfd, socket_pollfd *fds, nfds_t nfds, int timeout) {
 #if defined(WIN32)


This return type is gross enough that I'd consider supplying a typedef for it.

Yeap, can do...

mikepurvis · 2017-07-25T17:51:15Z

clients/roscpp/src/libros/io.cpp

+        if (fd_cnt < 0)
+        {
+          ROS_ERROR("Error in epoll_wait! %s", strerror(errno));
+        }


Do we know what conditions would cause this, and if there's something else we need to be doing about them?

The answer is "no". don't know when this would happen. And the best I can do now is retry next time hoping the failure will go away.

I am suspecting that may be the computer going into sleep mode might cause the syscall to fail on return. But have not tested it thoroughly yet. There are some flags to control that part which I want to play with once the overall structure is solidified.

Actually, the one case I know (taken care of in latest push), is that the syscall can be interrupted. In this case, we just return and try again later.

guillaumeautran · 2017-07-26T17:16:20Z

Some performance numbers using a played bag with 317 topics (bags plays in a loop)

$ rostopic list | wc -l
317

Legacy rosbag recorder (Linux version using poll)

$ time rosbag record  --duration 2m --all
..snip..
real	2m0.648s
user	0m10.856s
sys	0m22.224s

This version of ros_comm using poll (#undef HAVE_EPOLL)

$ time rosbag record  --duration 2m --all
..snip..
real	2m0.622s
user	0m10.488s
sys	0m20.912s

This version of ros_comm using epoll (#define HAVE_EPOLL)

$ time rosbag record  --duration 2m --all
..snip..
real	2m0.943s
user	0m14.744s
sys	0m7.224s

I did not try the windows version so don't know the numbers there...

dirk-thomas · 2017-07-26T17:29:33Z

Please try to avoid any unrelated white space changes. I know that it is tempting to "clean things up" on the go but it makes reviewing the patch more effort than necessary and make potential backports more problematic.

guillaumeautran · 2017-07-26T17:52:16Z

I'll scrub again for unrelated white-space changes. Sorry about that.

Switching to using epoll system calls to improve performance of the topic polling code by a factor of 2.

mikepurvis · 2017-08-17T00:57:03Z

This looks sane to me. Let's give it a try.

mikepurvis · 2017-08-17T01:00:23Z

Pushed to upstream's lunar-edge branch: https://github.com/ros/ros_comm/tree/lunar-edge

Switching to using epoll system calls to improve performance of the topic polling code by a factor of 2.

guillaumeautran force-pushed the CORE-7220-epoll_raw branch 4 times, most recently from 1ee218b to 91541a4 Compare July 25, 2017 17:41

mikepurvis reviewed Jul 25, 2017

View reviewed changes

guillaumeautran force-pushed the CORE-7220-epoll_raw branch from 91541a4 to e3a33f9 Compare July 26, 2017 17:17

guillaumeautran force-pushed the CORE-7220-epoll_raw branch 2 times, most recently from 32d303d to 2856125 Compare July 27, 2017 20:16

Topic subscription scalability fix (ros#1078)

4ca9af0

Switching to using epoll system calls to improve performance of the topic polling code by a factor of 2.

guillaumeautran force-pushed the CORE-7220-epoll_raw branch from 2856125 to 4ca9af0 Compare July 27, 2017 20:25

mikepurvis merged commit dc1a122 into lunar-devel Aug 17, 2017

mikepurvis deleted the CORE-7220-epoll_raw branch August 17, 2017 00:59

guillaumeautran mentioned this pull request Sep 12, 2017

Topic subscription scalability fix ros/ros_comm#1162

Closed

mikepurvis pushed a commit that referenced this pull request Nov 3, 2017

Topic subscription scalability fix (ros#1078) (#12)

955c15b

Switching to using epoll system calls to improve performance of the topic polling code by a factor of 2.

mikepurvis mentioned this pull request Nov 3, 2017

Topic subscription scalability fix ros/ros_comm#1217

Merged

mikepurvis pushed a commit that referenced this pull request Nov 15, 2017

Topic subscription scalability fix (ros#1078) (#12)

1973dc5

Switching to using epoll system calls to improve performance of the topic polling code by a factor of 2.

Conversation

guillaumeautran commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guillaumeautran commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dirk-thomas commented Jul 26, 2017

Uh oh!

guillaumeautran commented Jul 26, 2017

Uh oh!

mikepurvis commented Aug 17, 2017

Uh oh!

mikepurvis commented Aug 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guillaumeautran commented Jul 26, 2017 •

edited

Loading