zanmato1984 commented on code in PR #41128:
URL: https://github.com/apache/arrow/pull/41128#discussion_r3401023836
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -459,7 +459,25 @@ if(ARROW_JEMALLOC)
set_source_files_properties(memory_pool_jemalloc.cc
PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
+if(ARROW_MIMALLOC)
+ list(APPEND ARROW_MEMORY_POOL_SRCS memory_pool_mimalloc.cc)
+ set_source_files_properties(memory_pool_mimalloc.cc
+ PROPERTIES SKIP_PRECOMPILE_HEADERS ON
+ SKIP_UNITY_BUILD_INCLUSION ON)
+endif()
+# GH-50083: make sure some allocation-related public symbols are interposable,
+# for the benefit of memory profilers and other runtime analyzers.
+# Ideally we would scope a specific subset of symbols, but it doesn't seem
+# easily doable. See `memory_pool_internal.h`.
+if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU"
+ OR CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+ OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
+ set_source_files_properties(${ARROW_MEMORY_POOL_SRCS}
+ PROPERTIES COMPILE_OPTIONS
"-fsemantic-interposition")
+endif()
Review Comment:
Why is it not within `if(ARROW_MIMALLOC)`? Are we considering allowing other
allocators to be interposed in the future?
##########
cpp/src/arrow/memory_pool_internal.h:
##########
@@ -49,8 +46,21 @@ class JemallocAllocator {
#endif // defined(ARROW_JEMALLOC)
-} // namespace internal
+} // namespace arrow::memory_pool::internal
+
+#ifdef ARROW_MIMALLOC
-} // namespace memory_pool
+extern "C" {
+// GH-50083: expose public symbols with well-known names for memory profilers
+// to be able to intercept our mimalloc calls and better make sense of Arrow's
+// memory profile.
+// These symbols need to be interposable (using e.g. `LD_PRELOAD`), which is
+// ensured using a custom compilation flag in CMakeLists.txt.
Review Comment:
Maybe we can be more explicit on the "custom compilation flag"?
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -459,7 +459,25 @@ if(ARROW_JEMALLOC)
set_source_files_properties(memory_pool_jemalloc.cc
PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
+if(ARROW_MIMALLOC)
+ list(APPEND ARROW_MEMORY_POOL_SRCS memory_pool_mimalloc.cc)
+ set_source_files_properties(memory_pool_mimalloc.cc
Review Comment:
I can understand skipping unity build inclusion. But why is skipping
precompile headers needed?
##########
cpp/src/arrow/memory_pool_test.h:
##########
@@ -106,6 +106,14 @@ class TestMemoryPoolBase : public ::testing::Test {
pool->Free(data512, 10, 512);
}
}
+
+ void TestReleaseUnused() {
Review Comment:
Is this test really helping on the code changed in this PR?
##########
cpp/src/arrow/memory_pool_benchmark.cc:
##########
@@ -17,6 +17,7 @@
#include "arrow/memory_pool.h"
#include "arrow/result.h"
+#include "arrow/util/config.h"
Review Comment:
Why this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
