Only keep the last sample in the image tools by default. #238

clalancette · 2018-06-01T18:26:46Z

Particularly for cam2image, running it without a subscriber
and KEEP_ALL will eventually cause it to exhaust virtual
memory space. Running it with KEEP_LAST lets it run indefinitely
without growing memory usage.

We saw this problem while just running cam2image for this tutorial https://github.com/ros2/ros2/wiki/Rosbag-with-ROS1-Bridge . We had left it running for a little while before running the bridge/rosbag, and it would crash after a few minutes. With this change in place, we can leave it running for as long as we like.

clalancette · 2018-06-07T17:06:10Z

Linux
Linux-aarch64
macOS
Windows

dhood

You're using fastrtps right? I wouldn't have expected the publisher to keep the samples if there are no subscribers (unless we use transient local durability, which we aren't). But if this is the behaviour that we observe, then I agree it makes sense to change the default of the demo.

The help text of the demo/title of the PR are misleading because keep last will keep up to the queue size, not just 1 sample. I added e8e401a to improve the docs to more closely match https://github.com/ros2/ros2/wiki/About-Quality-of-Service-Settings

We should delay/keep this change in mind wrt ros2/rmw_fastrtps#202 which is referencing the existing defaults

clalancette · 2018-06-07T18:09:56Z

You're using fastrtps right? I wouldn't have expected the publisher to keep the samples if there are no subscribers (unless we use transient local durability, which we aren't). But if this is the behaviour that we observe, then I agree it makes sense to change the default of the demo.

Fast-RTPS, correct. Do you think the publisher keeping the samples even if there are no subscribers is a bug we should report to upstream Fast-RTPS?

The help text of the demo/title of the PR are misleading because keep last will keep up to the queue size, not just 1 sample. I added e8e401a to improve the docs to more closely match https://github.com/ros2/ros2/wiki/About-Quality-of-Service-Settings

👍

We should delay/keep this change in mind wrt ros2/rmw_fastrtps#202 which is referencing the existing defaults

Good point. I'll keep this in progress until that is merged, then I'll retest.

dhood · 2018-06-07T18:11:46Z

Do you think the publisher keeping the samples even if there are no subscribers is a bug we should report to upstream Fast-RTPS?

I'm not sure if the behaviour is within the spec or not, so don't know that it's a bug. We could ask eProsima.

dhood · 2018-06-07T18:29:22Z

Do you think the publisher keeping the samples even if there are no subscribers is a bug we should report to upstream Fast-RTPS?

@richiprosima did you have any input on if this is the expected behaviour?

dirk-thomas · 2018-06-07T18:33:23Z

Do you think the publisher keeping the samples even if there are no subscribers is a bug

This is not a bug but expected behavior in DDS. The purpose is to provide all "historical" data to late joining subscribers.

dhood · 2018-06-07T18:35:32Z

even if using durability of volatile as mentioned?

dirk-thomas · 2018-06-07T21:55:57Z

I wouldn't expect it to happen with volatile. Seems like the same thing we discussed earlier: ros2/rmw_fastrtps#68 (comment)

richiware · 2018-06-08T06:02:43Z

While publisher was running, was a subscriber running too? In this case, was the subscriber's application killed or stopped neatly?

My thought is that publisher doesn't know the subscriber is not anymore there and is storing the samples until the subscriber acknowledge them.

mikaelarguedas · 2018-06-08T07:06:02Z

I can confirm that this is reproducible without any subscriber running.

Using master, without any other node running, I ran:
time ros2 run image_tools cam2image -b -x 640 -y 480 -f 400

After 14 sec, the process is using 4.5GB of RAM and crashes when trying to publish the 5001st sample

[INFO] [cam2image]: Publishing image #5000
[INFO] [cam2image]: Publishing image #5001
terminate called after throwing an instance of 'std::runtime_error'
  what():  failed to publish message: cannot publish data, at /home/mikael/work/ros2/bouncy_ws/src/ros2/rmw_fastrtps/rmw_fastrtps_cpp/src/rmw_publish.cpp:53, at /home/mikael/work/ros2/bouncy_ws/src/ros2/rcl/rcl/src/rcl/publisher.c:241

real	0m14.235s
user	0m4.079s
sys	0m2.804s

That is expected when we use KEEP_ALL from what I understand ? ros2/rmw_fastrtps#68
Or does @richiware mean that we should not face the 5000 message issue if we use volatile no subscriber was ever matched on that topic?

@clalancette The patch should be modified to use the defaults profile from rmw. This problem was faced and worked around in ros2/rmw#82. So I think that this demo should stop hard-coding the history and reliability policy (but keep defining the queue size) and use the default profiles instead

richiware · 2018-06-08T11:01:56Z

Although you use KEEP_ALL, in VOLATILE samples are stored until subscribers acknowledge them. As I see this mechanism is implemented when an ACKNACK message is received.

In your scenario where there aren't subscribers this mechanism is not called and the samples are not removed from the history. This is a bug. I will work on it. Thanks for the reporting.

clalancette · 2018-06-08T12:08:53Z

Using master, without any other node running, I ran:
time ros2 run image_tools cam2image -b -x 640 -y 480 -f 400

After 14 sec, the process is using 4.5GB of RAM and crashes when trying to publish the 5001st sample

@mikaelarguedas Thanks for the additional testing; that is exactly what we saw in testing.

@clalancette The patch should be modified to use the defaults profile from rmw. This problem was faced and worked around in ros2/rmw#82. So I think that this demo should stop hard-coding the history and reliability policy (but keep defining the queue size) and use the default profiles instead

I think there is some value in having a demo where users can easily play with the "low-level" settings of history and reliability to see what the effects on the system are. That being said, I think that we should probably use the default profiles as a default, and only modify it if the user passes a flag. What do you think about that?

In your scenario where there aren't subscribers this mechanism is not called and the samples are not removed from the history. This is a bug. I will work on it. Thanks for the reporting.

@richiware Thanks for the confirmation.

mikaelarguedas · 2018-06-08T12:12:33Z

I think that we should probably use the default profiles as a default, and only modify it if the user passes a flag. What do you think about that?

Yes, that is what I'm suggesting 👍

I think there is some value in having a demo where users can easily play with the "low-level" settings of history and reliability to see what the effects on the system are.

Removing the hard-coded defaults will not modify user's ability to tweak the QoS by passing command line arguments

clalancette · 2018-06-14T18:14:03Z

I re-tested this with the latest Fast-RTPS changes, and this still "leaks" memory if there are no subscribers. Therefore, I've gone ahead nad rebased this, and changed it to stick with rmw_qos_profile_default unless the user explicitly requests something else via a command-line option.

I tested by first running ros2 run image_tools cam2image -- -b with no subscribers, and saw that it did not start using more and more memory (because of the KEEP_LAST default). Then I ran ros2 run image_tools cam2image -- -b -k 1, and saw that it used more and more memory (because of the KEEP_ALL requested plus the Fast-RTPS bug). I also ran it with ros2 run image_tools cam2image -- -b -d 400, and saw that it used up memory until it did the 400th publish, at which point it stop using more memory. So at this point, I think this is doing what we want (and working around the Fast-RTPS bug for now). I'm going to place it back in review, and new CI:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2018-06-14T18:26:58Z

@mikaelarguedas Pointed out that I should test against the release/1.6.0 branch of upstream Fast-RTPS. I rebuilt with that and ran the test again with KEEP_ALL, and no subscribers, and now there is no leaking. I think this PR is still valuable, so I'll leave it in review, but it is also less important now that the upstream Fast-RTPS bug has been fixed.

mikaelarguedas · 2018-06-14T18:30:32Z

image_tools/src/cam2image.cpp

    return 0;
  }

  // Initialize a ROS 2 node to publish images read from the OpenCV interface to the camera.
  auto node = rclcpp::Node::make_shared("cam2image");
  rclcpp::Logger node_logger = node->get_logger();

-  // Set the parameters of the quality of service profile. Initialize as the default profile
-  // and set the QoS parameters specified on the command line.


I found these comments valuable even if they come at the cost of a few local variables.
Do you mind keeping reliability_policy and friends variables but initialize them with the default profile values instead of the previously hard-coded ones?

I did basically this, but just re-used the fields of the structure. The code looks slightly weird this way, but at least has the documentation that you liked. I could re-introduce the local variables, but it seems like more of a way to make a mistake than anything else. Let me know what you think.

mikaelarguedas · 2018-07-05T21:43:34Z

It's not a very big deal but while this allows to keep the comments, all the statements have not effect so I don't know if it's clearer for users.

I'd prefer keeping the temporary variables (initialized with the default values of the qos profile), modify them based on user provided input, and then assign them to the custom qos profile with the appropriate comment.

Then I don't see it as a blocker for getting this merged.

Particularly for cam2image, running it without a subscriber and KEEP_ALL will eventually cause it to exhaust virtual memory space. Running it with KEEP_LAST lets it run indefinitely without growing memory usage. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-07-18T15:55:20Z

OK, I've rebased now and switched to only using the default arguments for depth, reliability, and history. I think this should be ready for another review; one more CI:

Linux
Linux-aarch64
macOS
Windows

mikaelarguedas

lgtm, thanks for iterating

mikaelarguedas · 2018-07-18T18:10:33Z

Can you look into the failures on windows ? it looks to be due to this change as it doesnt appear on master

clalancette · 2018-07-18T18:11:23Z

Can you look into the failures on windows ? it looks to be due to this change as it doesnt appear on master

Yeah, just looking into it now. Odd failure, I'm not sure why it is happening.

Windows was getting upset at the Unicode ones. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2018-07-18T19:38:59Z

Got it, Windows didn't like the Unicode quotes. One more CI on Windows just to confirm that it is fixed:

Windows

mikaelarguedas · 2018-07-18T20:14:18Z

So this was machine independant after all ? or is there something different on portable than the other machines?

clalancette · 2018-07-18T20:15:22Z

So this was machine independant after all ? or is there something different on portable than the other machines?

It was not machine dependent. I made a mistake while trying to reproduce it, and finally found the real problem (the quotes).

mikaelarguedas · 2018-07-18T20:15:50Z

👍

clalancette · 2018-07-19T13:51:00Z

CI is passing, and I have two approvals, so I'm going to merge this in. Thanks!

clalancette added the in progress Actively being worked on (Kanban column) label Jun 7, 2018

dhood approved these changes Jun 7, 2018

View reviewed changes

clalancette force-pushed the image-tools-keep-last branch from e8e401a to 9ea0a33 Compare June 14, 2018 18:03

clalancette added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 14, 2018

mikaelarguedas reviewed Jun 14, 2018

View reviewed changes

clalancette force-pushed the image-tools-keep-last branch from 9ea0a33 to 9bea79d Compare June 20, 2018 20:42

clalancette force-pushed the image-tools-keep-last branch from 9bea79d to 464d7ce Compare July 18, 2018 15:51

clalancette force-pushed the image-tools-keep-last branch from 464d7ce to 79ff18b Compare July 18, 2018 15:54

mikaelarguedas approved these changes Jul 18, 2018

View reviewed changes

Change Unicode quotes to single quotes.

7f529fc

Windows was getting upset at the Unicode ones. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette merged commit 71ab326 into master Jul 19, 2018

clalancette deleted the image-tools-keep-last branch July 19, 2018 13:51

clalancette removed the in review Waiting for review (Kanban column) label Jul 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only keep the last sample in the image tools by default. #238

Only keep the last sample in the image tools by default. #238

clalancette commented Jun 1, 2018

clalancette commented Jun 7, 2018

dhood left a comment

clalancette commented Jun 7, 2018

dhood commented Jun 7, 2018

dhood commented Jun 7, 2018

dirk-thomas commented Jun 7, 2018

dhood commented Jun 7, 2018

dirk-thomas commented Jun 7, 2018

richiware commented Jun 8, 2018

mikaelarguedas commented Jun 8, 2018

richiware commented Jun 8, 2018

clalancette commented Jun 8, 2018

mikaelarguedas commented Jun 8, 2018 •

edited

Loading

clalancette commented Jun 14, 2018

clalancette commented Jun 14, 2018

mikaelarguedas Jun 14, 2018

clalancette Jun 20, 2018

mikaelarguedas commented Jul 5, 2018

clalancette commented Jul 18, 2018

mikaelarguedas left a comment

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 18, 2018

clalancette commented Jul 18, 2018

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 18, 2018

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 19, 2018

Only keep the last sample in the image tools by default. #238

Only keep the last sample in the image tools by default. #238

Conversation

clalancette commented Jun 1, 2018

clalancette commented Jun 7, 2018

dhood left a comment

Choose a reason for hiding this comment

clalancette commented Jun 7, 2018

dhood commented Jun 7, 2018

dhood commented Jun 7, 2018

dirk-thomas commented Jun 7, 2018

dhood commented Jun 7, 2018

dirk-thomas commented Jun 7, 2018

richiware commented Jun 8, 2018

mikaelarguedas commented Jun 8, 2018

richiware commented Jun 8, 2018

clalancette commented Jun 8, 2018

mikaelarguedas commented Jun 8, 2018 • edited Loading

clalancette commented Jun 14, 2018

clalancette commented Jun 14, 2018

mikaelarguedas Jun 14, 2018

Choose a reason for hiding this comment

clalancette Jun 20, 2018

Choose a reason for hiding this comment

mikaelarguedas commented Jul 5, 2018

clalancette commented Jul 18, 2018

mikaelarguedas left a comment

Choose a reason for hiding this comment

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 18, 2018

clalancette commented Jul 18, 2018

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 18, 2018

mikaelarguedas commented Jul 18, 2018

clalancette commented Jul 19, 2018

mikaelarguedas commented Jun 8, 2018 •

edited

Loading