search

ros / catkin

A CMake-based build system that is used to build all packages in ROS.
http://wiki.ros.org/catkin
BSD 3-Clause "New" or "Revised" License
321 stars 280 forks source link

deduplicate dependency lists of exported code generation targets #1123

Closed pseyfert closed 3 years ago

pseyfert commented 4 years ago

TL;DR

deduplicate the list of _EXPORTED_TARGETS (from the comments I read this is about codegen targets).

background

In one of our projects the cmake configure time recently increased significantly (10s → 2min) and memory consumption of cmake increased noticably. With cmake's trace capabilities we found that in the <ourproject>_EXPORTED_TARGETS list took a lot of time to grow and by adding printout to the generated <workspace>/devel/share/<ourproject>/cmake/<ourproject>Config.cmake file, noticed that there were many many duplicate additions of

std_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
geometry_msgs_generate_messages_cpp
geometry_msgs_generate_messages_eus
geometry_msgs_generate_messages_lisp
geometry_msgs_generate_messages_nodejs
geometry_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
geometry_msgs_generate_messages_cpp
geometry_msgs_generate_messages_eus
geometry_msgs_generate_messages_lisp
geometry_msgs_generate_messages_nodejs
geometry_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
roscpp_generate_messages_cpp

It seemed to us that deduplication of the list of exported targets is desirable and changed the template, just like the deduplication is already done with the list of library dirs in the line before.

Configure time went down to O(10s) again and build and tests seem fine.

second thoughts

I'm a bit suspicious though why deduplication isn't done already as the means to do so are there and even applied in the line before (and quickly glimpsing at the git history, _list_append_unique was already in that line when _EXPORTED_TARGETS got added).

For completeness sake, we're still have on our todo list to double check if we're seeing just a symptom of an underlying issue (potentially on our side) that causes these long duplications (I don't see a circular dependency in our stack). Also,we're using catkin_simple as catkin frontend which handles message generation targets for us.

dirk-thomas commented 4 years ago

@pseyfert Please provide a minimal reproducible example which demonstrates this. I would like to first understand where it is coming from / why the entries are duplicated.

pseyfert commented 4 years ago

thanks for the quick response. i'll let you know when i have something.

pseyfert commented 4 years ago

https://github.com/pseyfert/catkin_generated_target_reproducer in this setup, adding some printout instead of the duplicate removal, I see:

-- now have in base_msgs_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_gener           ate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py
-- running list APPEND base_msgs_EXPORTED_TARGETS std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_           generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in base_msgs_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_gener           ate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_a_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_g           enerate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_a_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_           messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;ba           se_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_           generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_b_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_g           enerate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_b_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_           messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;ba           se_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_           generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py;base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_c_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_g           enerate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_c_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_           messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- running list APPEND pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;ba           se_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py
-- now have in pkg_downstream_EXPORTED_TARGETS base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_           generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py;base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py;base_msgs_generate_messages_cpp;base_msgs_generate_messages_eus;base_msgs_generate_messages_lisp;base_msgs_generate_messages_nodejs;base_msgs_generate_messages_py;std_msgs_generate_messages_cpp;std_msgs_generate_messages_eus;std_msgs_generate_messages_lisp;std_msgs_generate_messages_nodejs;std_msgs_generate_messages_py

as you'll see I use catkin_simple therein, which is because a colleague spotted in the docs of its known limitations

All targets have a target dependency on any downstream message generation, which results in sub-optimal parallelization of targets, as there are unnecessary dependencies created.

We didn't (yet?) check if we're seeing a side effect of that (mostly because we first tried to get a reproducer close to our setup w/o rewriting from catkin_simple to catkin w/o catkin_simple)

SebasRatz commented 4 years ago

Hello, we were finally able to reproduce the duplication with catkin only, thus without catkin simple. It seems the issue happens with diamond structured dependencies involving more than 2 packages. We have the following structure in the reproducer:

          base_msgs
       /         |        \
 pkg_a     pkg_b      pkg_c
      \         |          /
          pkg_downstream
                |
           pkg_app

The following print just a line above the change in this MR shows that exported targets are duplicated in the list. The duplication starts only at pkg_app.

 message(STATUS "now in @PROJECT_NAME@_EXPORTED_TARGETS:  ${@PROJECT_NAME@_EXPORTED_TARGETS}")

The reproducer without catkin simple can be found here: https://github.com/SebasRatz/catkin_generated_target_reproducer

dirk-thomas commented 4 years ago

@tfoote @mjcarroll FYI.

mjcarroll commented 4 years ago

I have reproduced this behavior using the supplied repository:

In the example, it manifests on in pkg_app as described, I get the following:

base_msgs_generate_messages_cpp
base_msgs_generate_messages_eus
base_msgs_generate_messages_lisp
base_msgs_generate_messages_nodejs
base_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py
base_msgs_generate_messages_cpp
base_msgs_generate_messages_eus
base_msgs_generate_messages_lisp
base_msgs_generate_messages_nodejs
base_msgs_generate_messages_py
std_msgs_generate_messages_cpp
std_msgs_generate_messages_eus
std_msgs_generate_messages_lisp
std_msgs_generate_messages_nodejs
std_msgs_generate_messages_py

When applying this fix, it does remove duplicates.

I'm still a bit unsure, though, if this is a regression in behavior, as that particular piece of code has not been changed in quite some time.

@dirk-thomas was there a particular reason that the EXPORTED_TARGETS variable was not using _list_append_unique previously? Had it just never been a problem?

pseyfert commented 4 years ago

I'm still a bit unsure, though, if this is a regression in behavior

In case there is a misunderstanding. When I wrote

In one of our projects the cmake configure time recently increased significantly

we have on our side reorganised our packages and dependencies and ran into the issue in the process. So it's not necessarily a catkin regression, might also be we never had a problematic dependency graph.

dirk-thomas commented 4 years ago

was there a particular reason that the EXPORTED_TARGETS variable was not using _list_append_unique previously? Had it just never been a problem?

I guess it just never was an issue before. Since the order of targets is important I doubt list_append_unique() is the right approach. Using list_append_deduplicate() is probably the better approach.

pseyfert commented 4 years ago

Using list_append_deduplicate() is probably the better approach.

works in our project. updated.

SebasRatz commented 3 years ago

Just as a follow up info; the present issue got painful for us in a workspace with 100+ packages and many inter-dependencies. Towards the end the cmake configuration step alone was consuming around 20 GB of RAM (!) and took several minutes for certain top-level packages. With the proposed fix the time and memory consumption of the cmake step went back to being insignificant.