Skip to content

Added optional mimalloc allocator support for Release builds #6

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added optional mimalloc allocator support for Release builds #6

wants to merge 1 commit into from
+122 −1

Conversation

@asn5d
Copy link
Member

@asn5d asn5d commented Dec 3, 2025

This is an optional enhancement, not the main fix for #2 / #3.

@asn5d asn5d requested a review from Copilot December 3, 2025 19:03
Copilot finished reviewing on behalf of asn5d December 3, 2025 19:05
Copilot AI reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request adds optional mimalloc allocator support for Release builds as an enhancement, along with several important bug fixes and code quality improvements across the codebase.

Key Changes:

  • Integration of mimalloc allocator as an optional external dependency for Release builds
  • Modernization of CMake build system using interface libraries for compiler flags
  • Critical bug fixes in loop boundaries and array access patterns
  • Performance improvements through const reference parameter passing

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
CMakeLists.txt Adds mimalloc ExternalProject integration, modernizes compiler flags with interface library, moves compiler detection before project()
cvutil/CMakeLists.txt Adds mimalloc linking for Release builds, updates runtime dependency handling with EXTRA_RUNTIME_DEPS support
RoiManager/CMakeLists.txt Adds mimalloc linking for Release builds, comments out legacy build output configuration
PluginManager/CMakeLists.txt Adds mimalloc linking for Release builds, comments out legacy build output configuration
CMake/find_runtime_deps.cmake Adds support for explicitly specified extra runtime dependencies via EXTRA_RUNTIME_DEPS
cvutilConfig.cmake.in Reverts from install(FILES) to file(INSTALL) for runtime dependency installation
cvutil/cvutil_matlab_interface.h Changes string parameter to const reference for find() function
cvutil/cvutil_matlab_interface.cpp Fixes off-by-one errors in loop bounds (5 instances), updates find() signature, initializes Mat val with braces
cvutil/cvutil_linesim.h Changes vector parameter to const reference for doughlas_peucker()
cvutil/cvutil_linesim.cpp Implements const reference parameter with local copy modification pattern
cvutil/cvutil_core.h Changes vector parameter to const reference for doughlas_peucker()
cvutil/cvutil_core.cpp Updates doughlas_peucker() signature, fixes matrix type references in bwthin() boundary padding
cvutil/cvutil_bwthin.cpp Adds bounds checking before array access (12 instances)
cvutil/cvutil_bwskel.cpp Corrects row offset from 2 to 3 for rectify_components(), splits long conditionals
cvutil/cvutil_bwdist.cpp Fixes off-by-one error in loop bound
.gitignore Adds [Ii]nstall/ directory to ignore list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

RoiManager/CMakeLists.txt
Comment on lines +57 to +59
target_link_libraries(RoiManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(RoiManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "RoiManager will use mimalloc shared library for Release configuration")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Missing check for mimalloc target availability. While cvutil/CMakeLists.txt (lines 125-132) checks if the mimalloc target exists before linking, RoiManager doesn't perform this check. This could cause build failures if USE_MIMALLOC is ON but the mimalloc target hasn't been created yet.

Consider adding a similar check:

if(USE_MIMALLOC)
    if(TARGET mimalloc)
        target_link_libraries(RoiManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
        target_link_options(RoiManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
        message(STATUS "RoiManager will use mimalloc shared library for Release configuration")
    else()
        message(WARNING "USE_MIMALLOC is ON but mimalloc target not found!")
    endif()
endif()
Suggested change
target_link_libraries(RoiManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(RoiManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "RoiManager will use mimalloc shared library for Release configuration")
if(TARGET mimalloc)
target_link_libraries(RoiManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(RoiManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "RoiManager will use mimalloc shared library for Release configuration")
else()
message(WARNING "USE_MIMALLOC is ON but mimalloc target not found!")
endif()

Copilot uses AI. Check for mistakes.
PluginManager/CMakeLists.txt
Comment on lines +64 to +66
target_link_libraries(PluginManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(PluginManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "PluginManager will use mimalloc shared library for Release configuration")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Missing check for mimalloc target availability. While cvutil/CMakeLists.txt (lines 125-132) checks if the mimalloc target exists before linking, PluginManager doesn't perform this check. This could cause build failures if USE_MIMALLOC is ON but the mimalloc target hasn't been created yet.

Consider adding a similar check:

if(USE_MIMALLOC)
    if(TARGET mimalloc)
        target_link_libraries(PluginManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
        target_link_options(PluginManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
        message(STATUS "PluginManager will use mimalloc shared library for Release configuration")
    else()
        message(WARNING "USE_MIMALLOC is ON but mimalloc target not found!")
    endif()
endif()
Suggested change
target_link_libraries(PluginManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(PluginManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "PluginManager will use mimalloc shared library for Release configuration")
if(TARGET mimalloc)
target_link_libraries(PluginManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(PluginManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "PluginManager will use mimalloc shared library for Release configuration")
else()
message(WARNING "USE_MIMALLOC is ON but mimalloc target not found!")
endif()

Copilot uses AI. Check for mistakes.
cvutil/CMakeLists.txt
-D INSTALL_BIN=${CMAKE_INSTALL_PREFIX}/$<IF:$<BOOL:${WIN32}>,bin,lib/cvutil>
-D SEARCH_DIRS=${DEP_PATH}
-D SEARCH_DIRS="${RUNTIME_SEARCH_DIRS}"
-D EXTRA_RUNTIME_DEPS="$<$<AND:$<BOOL:${USE_MIMALLOC}>,$<CONFIG:Release>>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll;${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll>$<$<CONFIG:Debug>:;${ASAN_RUNTIME_DEP}>"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Complex generator expression with potential syntax issue. The expression concatenates mimalloc DLLs for Release and ASAN for Debug without proper delimiters. The semicolons in the file paths will be interpreted as CMake list separators, but the structure is unclear:

$<$<AND:...>:dll1;dll2>$<$<CONFIG:Debug>:;${ASAN_RUNTIME_DEP}>

This creates ambiguous list parsing. Consider simplifying to separate the Release and Debug cases more clearly, or ensure proper list formatting.

Suggested change
-D EXTRA_RUNTIME_DEPS="$<$<AND:$<BOOL:${USE_MIMALLOC}>,$<CONFIG:Release>>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll;${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll>$<$<CONFIG:Debug>:;${ASAN_RUNTIME_DEP}>"
-D EXTRA_RUNTIME_DEPS="$<IF:$<AND:$<BOOL:${USE_MIMALLOC}>,$<CONFIG:Release>>,${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll;${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll,$<IF:$<CONFIG:Debug>,${ASAN_RUNTIME_DEP},>>"

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +127 to +128
# $<$<CONFIG:Debug>:-DMI_TRACK_ASAN=ON>
# $<$<CONFIG:Debug>:-DMI_SHOW_ERRORS=ON>
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

[nitpick] The commented-out configuration options -DMI_TRACK_ASAN=ON and -DMI_SHOW_ERRORS=ON for Debug builds should either be enabled or removed. If these options are important for ASAN compatibility in Debug mode (as suggested by the presence of ASAN flags in the compiler settings), they should be uncommented. If they're not needed, the comments should be removed to avoid confusion.

Suggested change
# $<$<CONFIG:Debug>:-DMI_TRACK_ASAN=ON>
# $<$<CONFIG:Debug>:-DMI_SHOW_ERRORS=ON>
$<$<CONFIG:Debug>:-DMI_TRACK_ASAN=ON>
$<$<CONFIG:Debug>:-DMI_SHOW_ERRORS=ON>

Copilot uses AI. Check for mistakes.
@asn5d asn5d requested a review from Copilot December 3, 2025 22:53
Copilot finished reviewing on behalf of asn5d December 3, 2025 22:56
Copilot AI reviewed Dec 3, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CMakeLists.txt
if(USE_MIMALLOC)
include(ExternalProject)

message(STATUS "Building mimalloc as external project (will be linked for Debug and Release configurations)...")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The message indicates mimalloc will be linked for "Debug and Release configurations," but the actual linking (in cvutil, RoiManager, and PluginManager) only links it for Release builds via $<$<CONFIG:Release>:mimalloc>. This message is misleading and should state "will be built for both Debug and Release configurations, but only linked for Release" or simply "will be linked for Release configuration."

Suggested change
message(STATUS "Building mimalloc as external project (will be linked for Debug and Release configurations)...")
message(STATUS "Building mimalloc as external project (will be built for both Debug and Release configurations, but only linked for Release configuration)...")

Copilot uses AI. Check for mistakes.
cvutil/CMakeLists.txt
if(TARGET mimalloc)
target_link_libraries(cvutil PRIVATE $<$<CONFIG:Release>:mimalloc>)
# Force the linker to include mimalloc.dll as a dependency
target_link_options(cvutil PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The target link option /INCLUDE:mi_version is MSVC-specific (Windows only), but there's no platform check before applying it. If this code runs on Linux or macOS, it will fail. This option should be guarded with a platform check like $<$<PLATFORM_ID:Windows>:...> or $<$<CXX_COMPILER_ID:MSVC>:...>. The same issue exists in RoiManager and PluginManager.

Suggested change
target_link_options(cvutil PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
target_link_options(cvutil PRIVATE $<$<AND:$<CONFIG:Release>,$<CXX_COMPILER_ID:MSVC>>:/INCLUDE:mi_version>)

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +234 to +237
install(FILES
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
CONFIGURATIONS Release
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The mimalloc DLLs installation is only configured for Release builds (CONFIGURATIONS Release), but if the ExternalProject builds mimalloc for both Debug and Release as suggested by line 107's message, there should be consideration for Debug builds as well. Additionally, this install command doesn't check if the files actually exist before installation. Consider adding existence checks or using generator expressions to handle this more robustly.

Suggested change
install(FILES
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
CONFIGURATIONS Release
install(FILES
$<IF:$<EXISTS:${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll>,${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll,>
$<IF:$<EXISTS:${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll>,${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll,>
CONFIGURATIONS Debug Release

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +131 to +148
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)

# Create imported target for linking
add_library(mimalloc SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc mimalloc_external)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)

# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The BUILD_BYPRODUCTS specifies paths like ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll which are Windows-specific (.dll files). If USE_MIMALLOC is enabled on Linux or macOS, this will be incorrect as those platforms use .so or .dylib files respectively. The mimalloc configuration should either be Windows-only or use platform-appropriate file extensions.

Suggested change
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
# Create imported target for linking
add_library(mimalloc SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc mimalloc_external)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll>
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll>
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib>
$<$<PLATFORM_ID:Linux>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.so>
$<$<PLATFORM_ID:Darwin>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dylib>
)
# Create imported target for linking
add_library(mimalloc SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc mimalloc_external)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll>
$<$<PLATFORM_ID:Linux>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.so>
$<$<PLATFORM_ID:Darwin>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dylib>
IMPORTED_IMPLIB
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib>
)
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION
$<$<PLATFORM_ID:Windows>:${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll>
$<$<PLATFORM_ID:Linux>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc-redirect.so>
$<$<PLATFORM_ID:Darwin>:${MIMALLOC_INSTALL_DIR}/lib/mimalloc-redirect.dylib>

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +144 to +149
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The mimalloc-redirect target specifies Windows-specific .dll path. The mimalloc-redirect library is primarily needed on Windows for DLL override behavior. On Linux, this is typically not needed or uses a different mechanism. This configuration should be Windows-specific.

Suggested change
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
# Create imported target for mimalloc-redirect (Windows only)
if(WIN32)
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
endif()

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +109 to +151
# Determine build directory for mimalloc
set(MIMALLOC_PREFIX ${CMAKE_BINARY_DIR}/_external/mimalloc)
set(MIMALLOC_INSTALL_DIR ${MIMALLOC_PREFIX}/install)

# Use ExternalProject_Add for complete control - this excludes mimalloc from install
# Build for both Debug (with ASAN tracking) and Release (without ASAN)
ExternalProject_Add(
mimalloc_external
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_TAG v2.2.4
PREFIX ${MIMALLOC_PREFIX}
CMAKE_ARGS
-DCMAKE_BUILD_TYPE=$<CONFIG>
-DMI_BUILD_SHARED=ON
-DMI_BUILD_STATIC=OFF
-DMI_BUILD_TESTS=OFF
-DMI_BUILD_OBJECT=OFF
-DMI_OVERRIDE=ON
# $<$<CONFIG:Debug>:-DMI_TRACK_ASAN=ON>
# $<$<CONFIG:Debug>:-DMI_SHOW_ERRORS=ON>
-DCMAKE_INSTALL_PREFIX=${MIMALLOC_INSTALL_DIR}
BUILD_BYPRODUCTS
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)

# Create imported target for linking
add_library(mimalloc SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc mimalloc_external)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)

# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)

message(STATUS "mimalloc external project configured")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The CMAKE_ARGS parameter -DCMAKE_BUILD_TYPE=$<CONFIG> uses a generator expression which is not evaluated at configuration time when ExternalProject_Add runs. This will likely not work as intended - the external project will receive the literal string "$<CONFIG>" instead of "Debug" or "Release". Consider using CMAKE_CONFIGURATION_TYPES or building separate debug/release versions of mimalloc, or use a specific build type like Release since the linking is only done for Release builds anyway.

Suggested change
# Determine build directory for mimalloc
set(MIMALLOC_PREFIX ${CMAKE_BINARY_DIR}/_external/mimalloc)
set(MIMALLOC_INSTALL_DIR ${MIMALLOC_PREFIX}/install)
# Use ExternalProject_Add for complete control - this excludes mimalloc from install
# Build for both Debug (with ASAN tracking) and Release (without ASAN)
ExternalProject_Add(
mimalloc_external
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_TAG v2.2.4
PREFIX ${MIMALLOC_PREFIX}
CMAKE_ARGS
-DCMAKE_BUILD_TYPE=$<CONFIG>
-DMI_BUILD_SHARED=ON
-DMI_BUILD_STATIC=OFF
-DMI_BUILD_TESTS=OFF
-DMI_BUILD_OBJECT=OFF
-DMI_OVERRIDE=ON
# $<$<CONFIG:Debug>:-DMI_TRACK_ASAN=ON>
# $<$<CONFIG:Debug>:-DMI_SHOW_ERRORS=ON>
-DCMAKE_INSTALL_PREFIX=${MIMALLOC_INSTALL_DIR}
BUILD_BYPRODUCTS
${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
# Create imported target for linking
add_library(mimalloc SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc mimalloc_external)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
message(STATUS "mimalloc external project configured")
# Determine build directories for mimalloc
set(MIMALLOC_PREFIX ${CMAKE_BINARY_DIR}/_external/mimalloc)
set(MIMALLOC_INSTALL_DIR_DEBUG ${MIMALLOC_PREFIX}/install_debug)
set(MIMALLOC_INSTALL_DIR_RELEASE ${MIMALLOC_PREFIX}/install_release)
# Build Debug version (with ASAN tracking)
ExternalProject_Add(
mimalloc_external_debug
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_TAG v2.2.4
PREFIX ${MIMALLOC_PREFIX}/debug
CMAKE_ARGS
-DCMAKE_BUILD_TYPE=Debug
-DMI_BUILD_SHARED=ON
-DMI_BUILD_STATIC=OFF
-DMI_BUILD_TESTS=OFF
-DMI_BUILD_OBJECT=OFF
-DMI_OVERRIDE=ON
-DMI_TRACK_ASAN=ON
-DMI_SHOW_ERRORS=ON
-DCMAKE_INSTALL_PREFIX=${MIMALLOC_INSTALL_DIR_DEBUG}
BUILD_BYPRODUCTS
${MIMALLOC_INSTALL_DIR_DEBUG}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR_DEBUG}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR_DEBUG}/lib/mimalloc.dll.lib
)
# Build Release version (without ASAN)
ExternalProject_Add(
mimalloc_external_release
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_TAG v2.2.4
PREFIX ${MIMALLOC_PREFIX}/release
CMAKE_ARGS
-DCMAKE_BUILD_TYPE=Release
-DMI_BUILD_SHARED=ON
-DMI_BUILD_STATIC=OFF
-DMI_BUILD_TESTS=OFF
-DMI_BUILD_OBJECT=OFF
-DMI_OVERRIDE=ON
-DCMAKE_INSTALL_PREFIX=${MIMALLOC_INSTALL_DIR_RELEASE}
BUILD_BYPRODUCTS
${MIMALLOC_INSTALL_DIR_RELEASE}/bin/mimalloc.dll
${MIMALLOC_INSTALL_DIR_RELEASE}/bin/mimalloc-redirect.dll
${MIMALLOC_INSTALL_DIR_RELEASE}/lib/mimalloc.dll.lib
)
# Create imported target for linking (Debug)
add_library(mimalloc_debug SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc_debug mimalloc_external_debug)
set_target_properties(mimalloc_debug PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR_DEBUG}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR_DEBUG}/lib/mimalloc.dll.lib
)
# Create imported target for linking (Release)
add_library(mimalloc_release SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc_release mimalloc_external_release)
set_target_properties(mimalloc_release PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR_RELEASE}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR_RELEASE}/lib/mimalloc.dll.lib
)
# Create imported target for mimalloc-redirect (Debug)
add_library(mimalloc-redirect_debug SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect_debug mimalloc_external_debug)
set_target_properties(mimalloc-redirect_debug PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR_DEBUG}/bin/mimalloc-redirect.dll
)
# Create imported target for mimalloc-redirect (Release)
add_library(mimalloc-redirect_release SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect_release mimalloc_external_release)
set_target_properties(mimalloc-redirect_release PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR_RELEASE}/bin/mimalloc-redirect.dll
)
message(STATUS "mimalloc external projects for Debug and Release configured")

Copilot uses AI. Check for mistakes.
PluginManager/CMakeLists.txt
Comment on lines +63 to +67
if(USE_MIMALLOC)
target_link_libraries(PluginManager PRIVATE $<$<CONFIG:Release>:mimalloc>)
target_link_options(PluginManager PRIVATE $<$<CONFIG:Release>:/INCLUDE:mi_version>)
message(STATUS "PluginManager will use mimalloc shared library for Release configuration")
endif()
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Unlike cvutil/CMakeLists.txt which checks if(TARGET mimalloc) before linking, this code only checks USE_MIMALLOC but doesn't verify that the mimalloc target actually exists. If the mimalloc external project fails to build or if there's a configuration issue, this will cause a build error. Add a check for TARGET mimalloc similar to cvutil for consistency and better error handling.

Copilot uses AI. Check for mistakes.
CMakeLists.txt
# Build for both Debug (with ASAN tracking) and Release (without ASAN)
ExternalProject_Add(
mimalloc_external
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

[nitpick] The version v2.2.4 is being used. While this is a valid version tag, consider verifying that this specific version is compatible with the project's requirements and has been tested. It may be beneficial to add a comment explaining why this specific version was chosen (e.g., latest stable release, tested compatibility, specific bug fixes).

Suggested change
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
GIT_REPOSITORY https://github.com/microsoft/mimalloc.git
# Using v2.2.4: latest stable release as of 2024-06, tested for compatibility with this project.

Copilot uses AI. Check for mistakes.
CMakeLists.txt
Comment on lines +139 to +149
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)

# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The IMPORTED_LOCATION and IMPORTED_IMPLIB properties specify Windows-specific paths with .dll and .dll.lib extensions. If USE_MIMALLOC is enabled on non-Windows platforms, these paths will be incorrect. The imported target configuration should either be Windows-only or use platform-appropriate properties and file extensions (e.g., .so files on Linux, no IMPORTED_IMPLIB needed for non-Windows).

Suggested change
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
if(WIN32)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc.dll
IMPORTED_IMPLIB ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dll.lib
)
elseif(APPLE)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.dylib
)
elseif(UNIX)
set_target_properties(mimalloc PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/lib/mimalloc.so
)
endif()
# Create imported target for mimalloc-redirect
add_library(mimalloc-redirect SHARED IMPORTED GLOBAL)
add_dependencies(mimalloc-redirect mimalloc_external)
if(WIN32)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/bin/mimalloc-redirect.dll
)
elseif(APPLE)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/lib/mimalloc-redirect.dylib
)
elseif(UNIX)
set_target_properties(mimalloc-redirect PROPERTIES
IMPORTED_LOCATION ${MIMALLOC_INSTALL_DIR}/lib/mimalloc-redirect.so
)
endif()

Copilot uses AI. Check for mistakes.
cvutil/CMakeLists.txt
Comment on lines +248 to +250
if(USE_MIMALLOC AND TARGET mimalloc)
# Add mimalloc install directory to search paths
list(APPEND RUNTIME_SEARCH_DIRS ${MIMALLOC_INSTALL_DIR}/bin)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

[nitpick] The code references ${MIMALLOC_INSTALL_DIR} which is defined inside the if(USE_MIMALLOC) block in the root CMakeLists.txt. However, if cvutil/CMakeLists.txt is processed and USE_MIMALLOC is ON but the root CMakeLists hasn't been processed yet (or in unusual build configurations), this variable might be undefined. While CMake typically processes the root first, it's better practice to verify the variable is set before using it in the generator expression, or rely solely on the TARGET mimalloc check.

Copilot uses AI. Check for mistakes.
Copilot AI mentioned this pull request Dec 3, 2025
Draft
Copy link
Contributor

Copilot AI commented Dec 3, 2025

@asn5d I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Contributor

Copilot AI commented Dec 3, 2025

@asn5d I've opened a new pull request, #9, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI mentioned this pull request Dec 3, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@asn5d