Conversation
|
CI (before / after):
|
|
I would expose a flag in the API to turn it on or off. The default should be using some heuristic (as you described: bounded size below threshold). |
|
tl;dr I'd like to merge as-is and ticket parameterization and optimization. What do others think? My issue with exposing the option right now is that it isn't clear to me how it should work for middlewares which don't have this option. I feel that it's exposing some implementation specific details up to the user level. Ideally we wouldn't do that because it either makes the option complicated (what does it do on systems which do not have "async publishing") or it couples our interface to particular kinds of implementations (we require the notion of asynchronous publishing to implement rmw). If we add an parameter to control this, then I think we need to make it a ROS level general concept that every implementation needs to support. I think it is potentially related to the need for us to clarify our blocking behavior for publish, see: https://github.com/ros2/rcl/blob/master/rcl/include/rcl/publisher.h#L173-L175 My preference would be to turn it on always for now, as that will "just work", in order to fix some tests and demos that currently won't work with Connext. This would be at the expense of some performance, but just until we figure out how to parameterize it or optimize it correctly. Admittedly, that might take a while to address. Btw, this applies to FastRTPS too, because it also has this setting (which is an extension of the DDS spec). |
|
Bump, @ros2/ros2_team can you comment on this when you all get time? |
|
+1 👍 turning it on by default for now and creating a ticket for future-work to expose the option to turn it off (or automatically set it) through the rmw and rcl layers sounds good to me. We need to be able to successfully transmit large messages reliably, and this change allows that to happen. Optimizing the code path for compact bounded messages can come in later alpha releases, perhaps after we find a way for a large fraction of our messages to have bounded length (for starters, by setting a sane upper bound for the |
|
Ok, I'll ticket the issues of having a better heuristic and having the options exposed through the rcl api. |
|
I ticketed both issues:
With clean CI (as far as I can tell) and a +1 from @codebot, I'll merge this so that we have large message publishing for the image demos. |
Fixes #181
I'd like feedback on whether or not we should always turn this on. I'm guessing there is a performance impact for turning it on, and so it may not be wise to do so. As I see it, we have a few options:
The last option is clearly attractive since it "just works" and would save performance when it could, but I see two problems with the approach: figuring out a heuristic that always works for Connext and differences in behavior when this is enabled vs disabled. It would be surprising for the publish function to change behavior depending on the message you're using.
Also, I'm pretty sure you cannot change this setting after creating the publisher, so we'll have to have a heuristic that can determine whether or not to turn it on before starting to publish data.
My first guess would be something like, turn on async publishing:
The problem being that this would mean we have to turn on async publishing for pretty much all of our messages, since many have unbounded strings.
For now, we can just turn it on always and I can ticket the need to figure out when to turn it off for performance gains.