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

Regarding ament_libraries_deduplicate implementation #447

Open
VRichardJP opened this issue May 11, 2023 · 1 comment
Open

Regarding ament_libraries_deduplicate implementation #447

VRichardJP opened this issue May 11, 2023 · 1 comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)

Comments

@VRichardJP
Copy link
Contributor

VRichardJP commented May 11, 2023

Hi!

I have a few questions as I inspect a few cmake traces, and see that a big share of find_package() time is spent in ament_libraries_deduplicate:

image

if("${_lib}" MATCHES "^(debug|optimized|general)$")

  1. Where do the "debug", "optimized" and "general" come from? There are several places where ament macros check whether these keywords exist, but they don't seem to be introduced by ament. Is this some obscure CMake feature? As I check into a few inputs, I can't find any of these keyword.

  2. If I understand ament_libraries_pack_build_configuration code correctly, the function parses inputs like libA;libB;debug;libC;libD into libA;libB;debug:libC;libD. Is that correct? If yes, isn't the code overly complex? Wouldn't a simple regex do the job (and way faster?). The unpack function code seems to exactly do that, but does the REGEX REPLACE in a loop (which is not necessary?)

  3. In the main loop of ament_libraries_deduplicate here:

foreach(_lib ${_packed})
# remove existing value if it exists
list(REMOVE_ITEM _unique ${_lib})
# append value to the end
list(APPEND _unique ${_lib})
endforeach()

It seems to me the code could be faster written like this (without loop):

# keep only the last occurence while removing duplicates
list(REVERSE _packed)
list(REMOVE_DUPLICATES _packed)
list(REVERSE _packed)
@VRichardJP VRichardJP changed the title Regarding ament_libraries_deduplicate doing? Regarding ament_libraries_deduplicate implementation? May 11, 2023
@VRichardJP VRichardJP changed the title Regarding ament_libraries_deduplicate implementation? Regarding ament_libraries_deduplicate implementation May 11, 2023
@VRichardJP
Copy link
Contributor Author

VRichardJP commented May 11, 2023

I managed to successfully build the whole autoware project (~250 packages) with the following implementation (which should do exactly the same):

macro(ament_libraries_deduplicate VAR)
  string(REGEX REPLACE "(^|;)(debug|optimized|general);(.+)" "\\2${AMENT_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}\\3" _packed "${ARGN}")
  list(REVERSE _packed)
  list(REMOVE_DUPLICATES _packed)
  list(REVERSE _packed)
  string(REGEX REPLACE "(^|;)(debug|optimized|general)${AMENT_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}(.+)" "\\2;\\3" ${VAR} "${_packed}")
endmacro()

The macro is way faster with this change. Here is an example on autoware behavior_path_planner package (left before, right after):
image

Before ament_auto_find_build_dependencies() function took almost 17s, after it only took 4s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in review Waiting for review (Kanban column)
Projects
None yet
Development

No branches or pull requests

2 participants