Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make middleware selection more independent of build-time package availability #67

Merged
merged 2 commits into from
Sep 26, 2019

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 5, 2019

Defer list of available RMWs to build time of downstream packages so you can build a dependent package even if RMW_IMPLEMENTATION is set to a value not available when the rmw_implementation package was built.

Remove CMake Config flag "RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING".
Add CMake Config flag "RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION" so you can statically link a middleware, even if multiple present at build time. This value can be explicitly set, but by default preserves the behavior of "statically link if one middleware found".

Add a scary message if the user tries to change the implementation and built with RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION=ON

@rotu
Copy link
Contributor Author

rotu commented Sep 5, 2019

Alternative to #65

@rotu rotu changed the title Rework dynamic RMW Make middleware selection more independent of built-time package availability Sep 5, 2019
@rotu rotu changed the title Make middleware selection more independent of built-time package availability Make middleware selection more independent of build-time package availability Sep 5, 2019
@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2019

I’m not 100% sure, with the changes in this PR, if it’s still necessary to list middlewares in package.xml.

https://answers.ros.org/question/332359/why-does-rmw_implementation-have-a-build-dependency-on-certain-middleware-packages/

@dirk-thomas
Copy link
Member

As mentioned on the other ticket (#65 (comment)) imo it should be preserved that when only a single RMW implementation is present that by default rmw_implementation statically links against it. Since this patch changes that behavior I don't think that is a desired.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2019

I believe that change is desirable. Behavior-changing optimizations such as direct vs indirect loading of RMW should be explicit rather than implicit.

The previous behavior is unintuitive (#58) and undocumented.

If I’m tuning performance in real life, I’m going to want to compare multiple RMWs, and that means probably (1) having them checked out in my workspace at compile time or installed via apt, even if I’m only using one (2) toggling poco with the same RMW.

@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 6, 2019

I believe that change is desirable.

I disagree on this. If a user builds from source with a single RMW impl the most common case is that they want to use only that RMW impl. and then it is imo preferred that it performs the static linking since the user shouldn't "pay for" what they don't use / need.

(1) having them checked out in my workspace at compile time or installed via apt

For this the case of a single RMW impl at build time doesn't apply.

So in which scenario are you using a binary which was built with a single RMW implementation and then want to add other RMW implementations later? All binaries by Open Robotics use more than one RMW impl.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2019

since the user shouldn't "pay for" what they don't use / need

That’s the thing: you can’t tell when rmw_implementation is built whether the user will need dynamic linking when rmw_implementation is actually used in an overlay.

The previous behavior is an example of premature optimization.

In any case, if you install via apt, all users are forced to “pay for” this feature since the OSRF binaries are built with multiple typesupports.

So in which scenario are you using a binary which was built with a single RMW implementation and then want to add other RMW implementations later?

A hypothetical: I’ve built my underlay to zoom under FastRTPS, and I’d like to try out CycloneDDS. Now I install FastRTPS alongside CycloneDDS, and inadvertently either change the FastRTPS performance by linking differently or accidentally compare statically linked FastRTPS to dynamically linked CycloneDDS.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2019

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

@dirk-thomas
Copy link
Member

That’s the thing: you can’t tell when rmw_implementation is built whether the user will need dynamic linking when rmw_implementation is actually used in an overlay.

Correct. That is why we have to make a decision at built time which might not be the right one in some cases. My point is that I think the current strategy aim to do the right thing for the common use cases.

In any case, if you install via apt, all users are forced to “pay for” this feature since the OSRF binaries are built with multiple typesupports.

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

We simply decided that providing support for multiple RMW impl. in the provided binary packages is beneficial to the majority of the community.

I’ve built my underlay to zoom under FastRTPS, and I’d like to try out CycloneDDS. Now I install FastRTPS alongside CycloneDDS, and inadvertently either change the FastRTPS performance by linking differently or accidentally compare statically linked FastRTPS to dynamically linked CycloneDDS.

I you want to measure performance (which is not a common use case compared to the overall community which just wants to use the binaries) you need to know what you are doing anyway. So if you don't want to compare apples with oranges you need to compile both RMW impl either each on their own or both together to have an exact comparison.

Let's just agree that we disagree on what each of us thinks the right choice is here. Maybe someone else wants to comment with their opinion to get a broader set of perspectives.

@rotu
Copy link
Contributor Author

rotu commented Sep 6, 2019

Let's just agree that we disagree on what each of us thinks the right choice is here. Maybe someone else wants to comment with their opinion to get a broader set of perspectives.

👍 @nuclearsandwich, could you weigh in?

@clalancette
Copy link
Contributor

My opinion: we should default to always building dynamically, with an option to build one statically if the user wants. That is, I think we should take this PR. Most of the time, users are not concerned with absolute performance, and thus having dynamic loading be the default no matter how many DDS vendors you have installed seems to be the most consistent thing to do.

@nuclearsandwich
Copy link
Member

Not to mention if you rosdep install, you get the other RMWs! So the previous behavior seems to discourage meaningful use of rosdep.

I don't think that this has any bearing on the default linking behavior for rmw_implementations. Providing more complete group semantics and implementing them across the tooling is a separate (but valid) issue.

I agree with @dirk-thomas that anyone profiling will need to be paying attention to the rmw binding style and that which one is the default is less important. Our documentation for building from source focuses on the use of the default rmw. It might be worth adding something here: https://index.ros.org/doc/ros2/Installation/Dashing/Linux-Development-Setup/#install-more-dds-implementations-optional which mentions that if you first built a workspace with only one rmw implementation that you may need to reconfigure rmw_implementation to pick up the second one.

I also think defaulting to skipping poco makes sense as a default of "don't pay for what you aren't using" and yes someone may want to use it down the road and may not be fully aware of the change in behavior but I think that's a better situation than requiring people to opt-in to the optimization.

@rotu
Copy link
Contributor Author

rotu commented Sep 11, 2019

Even (especially) if the performance difference is extreme, we should provide a way to affirmatively opt in to static linking. There isn't one currently. ("Oops! I switched to another DDS but accidentally left the first RMW package installed with apt so it's building with dynamic linking and there's no way to tell").

Either it's important enough to make link type explicit or it's not. I don't see an argument for "this is important but not important enough to allow the user to turn ON static linking, only off".

@rotu rotu force-pushed the poco_opt_out branch 2 times, most recently from da7b714 to af5891c Compare September 11, 2019 19:30
@nuclearsandwich
Copy link
Member

I don't see an argument for "this is important but not important enough to allow the user to turn ON static linking, only off".

I got caught up in the discussion of changing the default that I wasn't considering this. I would be in favor of a build option that disables poco usage in favor of a specific rmw implementation as long as it doesn't disrupt the current default behavior.

@rotu
Copy link
Contributor Author

rotu commented Sep 19, 2019

Reverted to the static build if one RMW is found at build time. I think this is the wrong call, but at the end of the day, I don't think it will make much difference.

Please rereview

@rotu
Copy link
Contributor Author

rotu commented Sep 20, 2019

Rebased and revised commit message:

Defer list of available RMWs to build time of downstream packages so you can build a dependent package even if RMW_IMPLEMENTATION is set to a value not available when the rmw_implementation package was built.

Remove CMake Config flag "RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING".
Add CMake Config flag "RMW_IMPLEMENTATION_BAKED" so you can statically link a middleware, even if multiple present at build time. This value can be explicitly set, but by default preserves the behavior of "statically link if one middleware found".

Downgrade missing rmw library to a warning, since some RMW's like rmw_cyclonedds_cpp and rmw_fastrtps_dynamic_cpp (?) do not need to inject build dependencies.
Add a scary message if the user tries to change the implementation and built with RMW_IMPLEMENTATION_BAKED=ON

Signed-off-by: Dan Rose dan@digilabs.io

rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Show resolved Hide resolved
rmw_implementation/CMakeLists.txt Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
rmw_implementation/rmw_implementation-extras.cmake.in Outdated Show resolved Hide resolved
@rotu
Copy link
Contributor Author

rotu commented Sep 25, 2019

Rebased, reworded, and cleaned up unnecessary whitespace changes, as well as functional issues you pointed out. Thanks for your patience, @dirk-thomas!

Defer list of available RMWs to build time of downstream packages so you can build a dependent package even if RMW_IMPLEMENTATION is set to a value not available when the rmw_implementation package was built.

Remove CMake Config flag "RMW_IMPLEMENTATION_FORCE_DYNAMIC_LOADING".
Add CMake Config flag "RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION" so you can statically link a middleware, even if multiple present at build time. This value can be explicitly set, but by default preserves the behavior of "statically link if one middleware found".

Add a scary message if the user tries to change the implementation and built with RMW_IMPLEMENTATION_DISABLE_RUNTIME_SELECTION=ON

Signed-off-by: Dan Rose <dan@digilabs.io>
re-add logging call of rmw default

Signed-off-by: Dan Rose <dan@digilabs.io>
@dirk-thomas
Copy link
Member

dirk-thomas commented Sep 26, 2019

CI builds with only FastRTPS:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

CI builds with FastRTPS and Connext:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status (unrelated)

@dirk-thomas dirk-thomas added the enhancement New feature or request label Sep 26, 2019
@dirk-thomas
Copy link
Member

Thanks for this improvement and for iterating on the patch!

@dirk-thomas dirk-thomas merged commit b521289 into ros2:master Sep 26, 2019
@rotu rotu deleted the poco_opt_out branch September 27, 2019 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants