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.

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

Get rid of malloc symbols in libcommon #7065

Merged
merged 1 commit into from Sep 27, 2019
Merged

Get rid of malloc symbols in libcommon #7065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 24 additions & 3 deletions CMakeLists.txt
View file
Open in desktop
@@ -1,4 +1,3 @@
project(ClickHouse)
cmake_minimum_required(VERSION 3.3)

foreach(policy
Expand All @@ -13,6 +12,7 @@ foreach(policy
endif()
endforeach()

project(ClickHouse)
include (cmake/target.cmake)

# Ignore export() since we don't use it,
Expand Down Expand Up @@ -348,7 +348,7 @@ include (libs/libcommon/cmake/find_jemalloc.cmake)
include (libs/libcommon/cmake/find_cctz.cmake)
include (libs/libmysqlxx/cmake/find_mysqlclient.cmake)

# When testing for memory leaks with Valgrind, dont link tcmalloc or jemalloc.
# When testing for memory leaks with Valgrind, don't link tcmalloc or jemalloc.

if (USE_JEMALLOC)
message (STATUS "Link jemalloc: ${JEMALLOC_LIBRARIES}")
Expand All @@ -367,7 +367,7 @@ elseif (USE_TCMALLOC)
endif ()
elseif (SANITIZE)
message (STATUS "Will use ${SANITIZE} sanitizer.")
else ()
elseif (OS_LINUX)
message (WARNING "Non default allocator is disabled. This is not recommended for production Linux builds.")
endif ()

Expand All @@ -376,8 +376,29 @@ include (cmake/print_flags.cmake)
install (EXPORT global DESTINATION cmake)

add_subdirectory (contrib EXCLUDE_FROM_ALL)

macro (add_executable target)
# invoke built-in add_executable
_add_executable (${ARGV})
get_target_property (type ${target} TYPE)
if (${type} STREQUAL EXECUTABLE)
set_property (GLOBAL APPEND PROPERTY CLICKHOUSE_EXECUTABLES ${target})
endif()
endmacro()

set_property (GLOBAL PROPERTY CLICKHOUSE_EXECUTABLES "")
add_subdirectory (libs)
add_subdirectory (utils)
get_property (executables GLOBAL PROPERTY CLICKHOUSE_EXECUTABLES)
foreach (executable ${executables})
target_link_libraries (${executable} PRIVATE ${MALLOC_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just link clickhouse_new_delete everywhere?

Copy link
Collaborator Author

@amosbird amosbird Sep 25, 2019 •
edited

Choose a reason for hiding this comment

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

because clickhouse_new_delete provide operator::new/delete symbol interposition in libcxx/stdlibc++, which is similar to malloc. I'd like to make this strategy consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not everywhere

because I assume only executables under dbms require memory tracking.

endforeach ()

set_property (GLOBAL PROPERTY CLICKHOUSE_EXECUTABLES "")
add_subdirectory (dbms)
get_property (executables GLOBAL PROPERTY CLICKHOUSE_EXECUTABLES)
foreach (executable ${executables})
target_link_libraries (${executable} PRIVATE clickhouse_new_delete ${MALLOC_LIBRARIES})
endforeach ()

include (cmake/print_include_directories.cmake)
16 changes: 5 additions & 11 deletions dbms/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -100,6 +100,7 @@ set(dbms_sources)
add_headers_and_sources(clickhouse_common_io src/Common)
add_headers_and_sources(clickhouse_common_io src/Common/HashTable)
add_headers_and_sources(clickhouse_common_io src/IO)
list (REMOVE_ITEM clickhouse_common_io_sources src/Common/new_delete.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is special about this file so that we need to move it in a separate lib?


if(USE_RDKAFKA)
add_headers_and_sources(dbms src/Storages/Kafka)
Expand Down Expand Up @@ -139,6 +140,9 @@ endif ()

add_library(clickhouse_common_io ${clickhouse_common_io_headers} ${clickhouse_common_io_sources})

add_library (clickhouse_new_delete STATIC src/Common/new_delete.cpp)
target_link_libraries (clickhouse_new_delete clickhouse_common_io)

if (OS_FREEBSD)
target_compile_definitions (clickhouse_common_io PUBLIC CLOCK_MONOTONIC_COARSE=CLOCK_MONOTONIC_FAST)
endif ()
Expand Down Expand Up @@ -419,17 +423,7 @@ endif()

if (USE_JEMALLOC)
dbms_target_include_directories (SYSTEM BEFORE PRIVATE ${JEMALLOC_INCLUDE_DIR}) # used in Interpreters/AsynchronousMetrics.cpp
target_include_directories (clickhouse_common_io SYSTEM BEFORE PRIVATE ${JEMALLOC_INCLUDE_DIR}) # new_delete.cpp
# common/memory.h
if (MAKE_STATIC_LIBRARIES OR NOT SPLIT_SHARED_LIBRARIES)
# skip if we have bundled build, since jemalloc is static in this case
elseif (${JEMALLOC_LIBRARIES} MATCHES "${CMAKE_STATIC_LIBRARY_SUFFIX}$")
# if the library is static we do not need to link with it,
# since in this case it will be in libs/libcommon,
# and we do not want to link with jemalloc multiple times.
else()
target_link_libraries(clickhouse_common_io PRIVATE ${JEMALLOC_LIBRARIES})
endif()
target_include_directories (clickhouse_new_delete SYSTEM BEFORE PRIVATE ${JEMALLOC_INCLUDE_DIR})
endif ()

dbms_target_include_directories (PUBLIC ${DBMS_INCLUDE_DIR} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/src/Formats/include)
Expand Down
6 changes: 3 additions & 3 deletions dbms/programs/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -24,9 +24,9 @@ configure_file (config_tools.h.in ${CMAKE_CURRENT_BINARY_DIR}/config_tools.h)

macro(clickhouse_target_link_split_lib target name)
if(NOT CLICKHOUSE_ONE_SHARED)
target_link_libraries(${target} PRIVATE clickhouse-${name}-lib ${MALLOC_LIBRARIES})
target_link_libraries(${target} PRIVATE clickhouse-${name}-lib)
else()
target_link_libraries(${target} PRIVATE clickhouse-lib ${MALLOC_LIBRARIES})
target_link_libraries(${target} PRIVATE clickhouse-lib)
endif()
endmacro()

Expand Down Expand Up @@ -111,7 +111,7 @@ if (CLICKHOUSE_SPLIT_BINARY)
install(PROGRAMS clickhouse-split-helper DESTINATION ${CMAKE_INSTALL_BINDIR} RENAME clickhouse COMPONENT clickhouse)
else ()
add_executable (clickhouse main.cpp)
target_link_libraries (clickhouse PRIVATE clickhouse_common_io string_utils ${MALLOC_LIBRARIES})
target_link_libraries (clickhouse PRIVATE clickhouse_common_io string_utils)
target_include_directories (clickhouse BEFORE PRIVATE ${COMMON_INCLUDE_DIR})
target_include_directories (clickhouse PRIVATE ${CMAKE_CURRENT_BINARY_DIR})

Expand Down
2 changes: 1 addition & 1 deletion dbms/src/Client/tests/CMakeLists.txt
View file
Open in desktop
@@ -1,2 +1,2 @@
add_executable(test-connect test_connect.cpp)
target_link_libraries (test-connect dbms)
target_link_libraries (test-connect PRIVATE dbms)
14 changes: 7 additions & 7 deletions dbms/src/Processors/tests/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -6,10 +6,10 @@ add_executable (processors_test_merge_sorting_transform processors_test_merge_so
add_executable (processors_test_expand_pipeline processors_test_expand_pipeline.cpp)
add_executable (processors_test_aggregation processors_test_aggregation.cpp)

target_link_libraries (processors_test dbms)
target_link_libraries (processors_test_chain dbms)
target_link_libraries (processors_test_merge dbms)
target_link_libraries (processors_test_expand_pipeline dbms)
target_link_libraries (processors_test_merging_sorted_transform dbms)
target_link_libraries (processors_test_merge_sorting_transform dbms)
target_link_libraries (processors_test_aggregation dbms clickhouse_aggregate_functions)
target_link_libraries (processors_test PRIVATE dbms)
target_link_libraries (processors_test_chain PRIVATE dbms)
target_link_libraries (processors_test_merge PRIVATE dbms)
target_link_libraries (processors_test_expand_pipeline PRIVATE dbms)
target_link_libraries (processors_test_merging_sorted_transform PRIVATE dbms)
target_link_libraries (processors_test_merge_sorting_transform PRIVATE dbms)
target_link_libraries (processors_test_aggregation PRIVATE dbms clickhouse_aggregate_functions)
24 changes: 0 additions & 24 deletions libs/libcommon/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -65,29 +65,6 @@ add_library (common

${CONFIG_COMMON})

# When testing for memory leaks with Valgrind, dont link tcmalloc or jemalloc.

if (USE_JEMALLOC)
message (STATUS "Link jemalloc: ${JEMALLOC_LIBRARIES}")
set (MALLOC_LIBRARIES ${JEMALLOC_LIBRARIES})
elseif (USE_TCMALLOC)
if (DEBUG_TCMALLOC AND NOT GPERFTOOLS_TCMALLOC_MINIMAL_DEBUG)
message (FATAL_ERROR "Requested DEBUG_TCMALLOC but debug library is not found. You should install Google Perftools. Example: sudo apt-get install libgoogle-perftools-dev")
endif ()

if (DEBUG_TCMALLOC AND GPERFTOOLS_TCMALLOC_MINIMAL_DEBUG)
message (STATUS "Link libtcmalloc_minimal_debug for testing: ${GPERFTOOLS_TCMALLOC_MINIMAL_DEBUG}")
set (MALLOC_LIBRARIES ${GPERFTOOLS_TCMALLOC_MINIMAL_DEBUG})
else ()
message (STATUS "Link libtcmalloc_minimal: ${GPERFTOOLS_TCMALLOC_MINIMAL}")
set (MALLOC_LIBRARIES ${GPERFTOOLS_TCMALLOC_MINIMAL})
endif ()
elseif (SANITIZE)
message (STATUS "Will use ${SANITIZE} sanitizer.")
elseif (OS_LINUX)
message (WARNING "Non default allocator is disabled. This is not recommended for production Linux builds.")
endif ()

if (USE_INTERNAL_MEMCPY)
set (MEMCPY_LIBRARIES memcpy)
endif ()
Expand Down Expand Up @@ -120,7 +97,6 @@ target_link_libraries (common
PUBLIC
${Boost_SYSTEM_LIBRARY}
PRIVATE
${MALLOC_LIBRARIES}
${MEMCPY_LIBRARIES})

if (RT_LIBRARY)
Expand Down
18 changes: 9 additions & 9 deletions libs/libcommon/src/tests/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -10,20 +10,20 @@ add_executable (realloc-perf allocator.cpp)

set(PLATFORM_LIBS ${CMAKE_DL_LIBS})

target_link_libraries (date_lut_init common ${PLATFORM_LIBS})
target_link_libraries (date_lut2 common ${PLATFORM_LIBS})
target_link_libraries (date_lut3 common ${PLATFORM_LIBS})
target_link_libraries (date_lut4 common ${PLATFORM_LIBS})
target_link_libraries (date_lut_default_timezone common ${PLATFORM_LIBS})
target_link_libraries (local_date_time_comparison common)
target_link_libraries (realloc-perf common)
target_link_libraries (date_lut_init PRIVATE common ${PLATFORM_LIBS})
target_link_libraries (date_lut2 PRIVATE common ${PLATFORM_LIBS})
target_link_libraries (date_lut3 PRIVATE common ${PLATFORM_LIBS})
target_link_libraries (date_lut4 PRIVATE common ${PLATFORM_LIBS})
target_link_libraries (date_lut_default_timezone PRIVATE common ${PLATFORM_LIBS})
target_link_libraries (local_date_time_comparison PRIVATE common)
target_link_libraries (realloc-perf PRIVATE common)
add_check(local_date_time_comparison)

if(USE_GTEST)
add_executable(unit_tests_libcommon gtest_json_test.cpp gtest_strong_typedef.cpp gtest_find_symbols.cpp)
target_link_libraries(unit_tests_libcommon common ${GTEST_MAIN_LIBRARIES} ${GTEST_LIBRARIES})
target_link_libraries(unit_tests_libcommon PRIVATE common ${GTEST_MAIN_LIBRARIES} ${GTEST_LIBRARIES})
add_check(unit_tests_libcommon)
endif()

add_executable (dump_variable dump_variable.cpp)
target_link_libraries (dump_variable clickhouse_common_io)
target_link_libraries (dump_variable PRIVATE clickhouse_common_io)
2 changes: 1 addition & 1 deletion libs/libmysqlxx/src/tests/CMakeLists.txt
View file
Open in desktop
@@ -1,2 +1,2 @@
add_executable (mysqlxx_test mysqlxx_test.cpp)
target_link_libraries (mysqlxx_test mysqlxx)
target_link_libraries (mysqlxx_test PRIVATE mysqlxx)