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

Interface specific compilation units #183

Merged
merged 3 commits into from
Apr 16, 2019

Conversation

dirk-thomas
Copy link
Member

In short: the PR trades a longer build time for a lower memory consumption during the build ⚖️

Resolves ros2/build_farmer#106.

Before the patch the generated code produced one .cpp file per package. The memory consumption of the compiler therefore scales with the number of messages per package and the template specialization stuff is not light on memory. For packages like geometry_msgs, sensor_msgs or std_msgs which each contain ~30 messages the memory consumption of a single compilation unit could easily reach 2.5 GB. Building the bridge with N threads where N is by default the number of CPU cores could therefore easily exhaust the available memory.

With the patch the generated code produces one .cpp file per interface file (message, service). Therefore the memory consumption for a single compilation unit drops to less than 0.5 GB. The trade off is that instead of generating X .cpp files (where X is the number of interface packages it now generates Y .cpp files (where Y is the number of interface files which is easily an order of magnitude higher). So while the peak memory consumption drops significantly the compilation time will increase. On my system with 8 cores it took ~3min 20s before and ~7min 45s afterwards.

Build Status

The two commits should not be squashed to keep the first commit which just duplicates one of the templates separate to ease tracking the history.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Apr 12, 2019
@dirk-thomas dirk-thomas self-assigned this Apr 12, 2019
@dirk-thomas dirk-thomas force-pushed the interface_specific_compilation_units branch from d511f92 to 28c137a Compare April 12, 2019 20:58
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like the change and I think it is better to trade off slower compilation for less memory, especially for more memory constrained systems. Some notes:

  1. The inline comments are all nits.
  2. With this change in place, we should also update the README, as it says it takes a "tremendous amount of memory", which won't be true anymore (I noticed that the most memory is now used during link time, ~1GB resident).
  3. I'm not a huge fan of having the declaration for the get_factory_<pkg>__msg__<msgname> methods inside of the pkg_factories.hpp.em file. It seems more natural for e.g. pendulum_msgs__msg__JointCommand__factories.cpp to have the definition, pendulum_msgs__msg__JointCommand__factories.hpp to have the declaration, and then pendulum_msg_factories.hpp #include pendulum_msgs__msg__JointCommand__factories.hpp. What do you think?

resource/pkg_factories.hpp.em Show resolved Hide resolved
resource/pkg_factories.cpp.em Show resolved Hide resolved
resource/interface_factories.cpp.em Outdated Show resolved Hide resolved
@dirk-thomas
Copy link
Member Author

Squashing the last three commits.

@dirk-thomas dirk-thomas force-pushed the interface_specific_compilation_units branch from a1f6a18 to 79518bf Compare April 16, 2019 15:18
@dirk-thomas
Copy link
Member Author

we should also update the README, as it says it takes a "tremendous amount of memory", which won't be true anymore

Done in cd0ef16.

@dirk-thomas
Copy link
Member Author

I'm not a huge fan of having the declaration for the get_factory_<pkg>__msg__<msgname> methods inside of the pkg_factories.hpp.em file.

This has been that way before this patch, it is unrelated to this change and also not really necessary. Therefore I don't plan of changing it as part of this PR.

@dirk-thomas dirk-thomas merged commit c9911be into master Apr 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the interface_specific_compilation_units branch April 16, 2019 18:56
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

building the bridge requires a lot of memory
2 participants