From 03b96477499b43a87ac4c90835bf2aea2e90cae6 Mon Sep 17 00:00:00 2001 From: Krzysztof Laskowski Date: Fri, 12 Mar 2021 17:20:34 +0000 Subject: [PATCH] catkin pkg config template: performance fix for pack/unpack/append 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 --- cmake/catkin_libraries.cmake | 26 ++------------------- cmake/list_append_deduplicate.cmake | 5 ----- cmake/list_append_unique.cmake | 15 ++++--------- cmake/templates/pkgConfig.cmake.in | 35 +++++------------------------ 4 files changed, 12 insertions(+), 69 deletions(-) diff --git a/cmake/catkin_libraries.cmake b/cmake/catkin_libraries.cmake index b19f4c8aa..7f3514146 100644 --- a/cmake/catkin_libraries.cmake +++ b/cmake/catkin_libraries.cmake @@ -71,24 +71,7 @@ endfunction() # function(catkin_pack_libraries_with_build_configuration VAR) set(result "") - set(_argn ${ARGN}) - list(LENGTH _argn _count) - set(_index 0) - while(${_index} LESS ${_count}) - list(GET _argn ${_index} lib) - if("${lib}" MATCHES "^(debug|optimized|general)$") - math(EXPR _index "${_index} + 1") - if(${_index} EQUAL ${_count}) - message(FATAL_ERROR "catkin_pack_libraries_with_build_configuration() the list of libraries '${_argn}' ends with '${lib}' which is a build configuration keyword and must be followed by a library") - endif() - list(GET _argn ${_index} library) - list(APPEND result "${lib}${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}${library}") - else() - list(APPEND result "${lib}") - endif() - math(EXPR _index "${_index} + 1") - endwhile() - #debug_message(10 "catkin_pack_libraries_with_build_configuration(${VAR} ${_argn}) ${result}") + string(REGEX REPLACE "(^|;)(debug|optimized|general);([^;]+)(;|$)" "\\1\\2${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}\\3\\4" result "${ARGN}") set(${VAR} "${result}" PARENT_SCOPE) endfunction() @@ -105,12 +88,7 @@ endfunction() # function(catkin_unpack_libraries_with_build_configuration VAR) set(result "") - foreach(lib ${ARGN}) - string(REGEX REPLACE "^(debug|optimized|general)${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}(.+)$" "\\1;\\2" lib "${lib}") - list(APPEND result "${lib}") - endforeach() - #set(_argn ${ARGN}) - #debug_message(10 "catkin_unpack_libraries_with_build_configuration(${VAR} ${_argn}) ${result}") + string(REGEX REPLACE "(debug|optimized|general)${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}([^;]+)" "\\1;\\2" result "${ARGN}") set(${VAR} "${result}" PARENT_SCOPE) endfunction() diff --git a/cmake/list_append_deduplicate.cmake b/cmake/list_append_deduplicate.cmake index e59a61bb1..5c56099c3 100644 --- a/cmake/list_append_deduplicate.cmake +++ b/cmake/list_append_deduplicate.cmake @@ -1,11 +1,6 @@ # # Append elements to a list and remove existing duplicates from the list. # -# .. note:: Using CMake's ``list(APPEND ..)`` and -# ``list(REMOVE_DUPLICATES ..)`` is not sufficient since its -# implementation uses a set internally which makes the operation -# unstable. -# macro(list_append_deduplicate listname) if(NOT "${ARGN}" STREQUAL "") if(${listname}) diff --git a/cmake/list_append_unique.cmake b/cmake/list_append_unique.cmake index 4c9bb1df8..a6163cd62 100644 --- a/cmake/list_append_unique.cmake +++ b/cmake/list_append_unique.cmake @@ -1,16 +1,9 @@ # # Append elements to a list if they are not already in the list. # -# .. note:: Using CMake's ``list(APPEND ..)`` and -# ``list(REMOVE_DUPLICATES ..)`` is not sufficient since its -# implementation uses a set internally which makes the operation -# unstable. -# macro(list_append_unique listname) - foreach(_item ${ARGN}) - list(FIND ${listname} ${_item} _index) - if(_index EQUAL -1) - list(APPEND ${listname} ${_item}) - endif() - endforeach() + if(NOT "${ARGN}" STREQUAL "") + list(APPEND ${listname} ${ARGN}) + list(REMOVE_DUPLICATES ${listname}) + endif() endmacro() diff --git a/cmake/templates/pkgConfig.cmake.in b/cmake/templates/pkgConfig.cmake.in index 8740d9944..e2f7d0ed7 100644 --- a/cmake/templates/pkgConfig.cmake.in +++ b/cmake/templates/pkgConfig.cmake.in @@ -16,47 +16,24 @@ endmacro() # copied from catkin/cmake/list_append_unique.cmake to keep pkgConfig # self contained macro(_list_append_unique listname) - foreach(_item ${ARGN}) - list(FIND ${listname} ${_item} _index) - if(_index EQUAL -1) - list(APPEND ${listname} ${_item}) - endif() - endforeach() + if(NOT "${ARGN}" STREQUAL "") + list(APPEND ${listname} ${ARGN}) + list(REMOVE_DUPLICATES ${listname}) + endif() endmacro() # pack a list of libraries with optional build configuration keywords # copied from catkin/cmake/catkin_libraries.cmake to keep pkgConfig # self contained macro(_pack_libraries_with_build_configuration VAR) - set(${VAR} "") - set(_argn ${ARGN}) - list(LENGTH _argn _count) - set(_index 0) - while(${_index} LESS ${_count}) - list(GET _argn ${_index} lib) - if("${lib}" MATCHES "^(debug|optimized|general)$") - math(EXPR _index "${_index} + 1") - if(${_index} EQUAL ${_count}) - message(FATAL_ERROR "_pack_libraries_with_build_configuration() the list of libraries '${ARGN}' ends with '${lib}' which is a build configuration keyword and must be followed by a library") - endif() - list(GET _argn ${_index} library) - list(APPEND ${VAR} "${lib}${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}${library}") - else() - list(APPEND ${VAR} "${lib}") - endif() - math(EXPR _index "${_index} + 1") - endwhile() + string(REGEX REPLACE "(^|;)(debug|optimized|general);([^;]+)(;|$)" "\\1\\2${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}\\3\\4" ${VAR} "${ARGN}") endmacro() # unpack a list of libraries with optional build configuration keyword prefixes # copied from catkin/cmake/catkin_libraries.cmake to keep pkgConfig # self contained macro(_unpack_libraries_with_build_configuration VAR) - set(${VAR} "") - foreach(lib ${ARGN}) - string(REGEX REPLACE "^(debug|optimized|general)${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}(.+)$" "\\1;\\2" lib "${lib}") - list(APPEND ${VAR} "${lib}") - endforeach() + string(REGEX REPLACE "(debug|optimized|general)${CATKIN_BUILD_CONFIGURATION_KEYWORD_SEPARATOR}([^;]+)" "\\1;\\2" ${VAR} "${ARGN}") endmacro()