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

catkin pkg config template: performance fix for pack/unpack/append #1137

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

aurzenligl
Copy link

@aurzenligl aurzenligl commented Mar 12, 2021

rationale
These algorithms may be used O(M^2) times where M is the level of rospackage nesting, and each pack/unpack/append algorithm complexity are O(N^2)/O(N)/O(N^2) where N is libraries count. The fix allows to reduce them to O(1) complexity if we only consider amount of cmake statements executed. In a ros distro where level of nesting may exceed 10 and library count a couple of hundreds, cmake execution time can easily take more than 30 seconds, which becomes prohibitive.

reproduction
https://github.com/aurzenligl/study/tree/master/ros-catkin-pkgconfig

# noetic-devel
aurzenligl@study:/code/ros-catkin-pkgconfig$ ./compile.sh 
real	0m18.339s
user	0m16.333s
sys	0m1.994s
# aurzenligl/pkg-config-speedup
aurzenligl@study:/code/ros-catkin-pkgconfig$ ./compile.sh 
real	0m5.504s
user	0m4.378s
sys	0m1.146s

notes
Notes regarding REMOVE_DUPLICATES in list_append_unique and list_append_deduplicate are incorrect, in case of:

  • list_append_unique: REMOVE_DUPLICATES uses set/unordered_set to remember encountered items, all output items are written in order, so the algorithm is "stable", it doesn't shuffle elements
  • list_append_deduplicate: REMOVE_DUPLICATES is improper here, as it removes duplicates from the added element list, not the added-to element list, not due to it's "unstability" as note suggests

3.0.2: https://github.com/Kitware/CMake/blob/v3.0.2/Source/cmListCommand.cxx#L480-L493
3.19.6: https://github.com/Kitware/CMake/blob/v3.19.6/Source/cmAlgorithms.h#L120-L130

These algorithms may be used O(M^2) times where M is the level of
rospackage nesting, and each pack/unpack/append algorithm complexity
are O(N^2)/O(N)/O(N^2) where N is libraries count. The fix allows
to reduce them to O(1) complexity if we only consider amount of cmake
statements executed.

Notes regarding REMOVE_DUPLICATES in list_append_unique and
list_append_deduplicate are incorrect, in case of:

- list_append_unique: REMOVE_DUPLICATES uses set/unordered_set to
  remember encountered items, all output items are written in order,
  so the algorithm is "stable", it doesn't shuffle elements

- list_append_deduplicate: REMOVE_DUPLICATES is improper here, as it
  removes duplicates from the added element list, not the added-to element
  list, not due to it's "unstability" as note suggests

3.0.2:  https://github.com/Kitware/CMake/blob/v3.0.2/Source/cmListCommand.cxx#L480-L493
3.19.6: https://github.com/Kitware/CMake/blob/v3.19.6/Source/cmAlgorithms.h#L120-L130
@aurzenligl aurzenligl force-pushed the aurzenligl/pkg-config-speedup branch from 234f866 to 03b9647 Compare March 12, 2021 19:20
@aurzenligl
Copy link
Author

@tfoote, @mjcarroll, should I open an issue associated with the PR?

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.

1 participant