Skip to content

GH-45209: [C++][CMake] Fix the issue that allocator not disabled for sanitizer cmake presets #45210

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

Merged
merged 7 commits into from
Jan 10, 2025
Merged

GH-45209: [C++][CMake] Fix the issue that allocator not disabled for sanitizer cmake presets #45210

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
732198c
Fix the issue that allocator not disabled for sanitizer cmake presets
zanmato1984 Jan 9, 2025
e55bdb8
A lot of reorganization of cmake presets
zanmato1984 Jan 9, 2025
452370c
Reuse _allocator presets
zanmato1984 Jan 10, 2025
20da6bf
Add preset for all allocators
zanmato1984 Jan 10, 2025
7150e71
Revert allocator presets and adjust inheritance order
zanmato1984 Jan 10, 2025
424a9a2
Fix for maximal builds
zanmato1984 Jan 10, 2025
1b67c7c
Update cpp/CMakePresets.json
zanmato1984 Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
261 changes: 135 additions & 126 deletions cpp/CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,30 @@
"patch": 0
},
"configurePresets": [
{
"name": "_allocator-none",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use this in features-minimal too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes we can. I was trying to add some specialty to _ presets, that is, not using it in regular inheritance where there is no conflicting variables. However I think nothing functional really prevents it from being used in regular inheritance. I'll try that and update soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we can add comments by "$comment": https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html#format

Could you add a comment why we use _XXX name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the idea. But unfortunately "$comment" is only supported in version 10 or above, and version 10 is only supported since cmake 3.31.

From the link you provided.

Preset files specifying version 10 or above may include comments using the key $comment at any level within the JSON object to provide documentation.

10 Added in version 3.31.

Copy link
Member

Choose a reason for hiding this comment

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

Oh...

"hidden": true,
"cacheVariables": {
"ARROW_JEMALLOC": "OFF",
"ARROW_MIMALLOC": "OFF"
}
},
{
"name": "_allocator-jemalloc",
"hidden": true,
"cacheVariables": {
"ARROW_JEMALLOC": "ON",
"ARROW_MIMALLOC": "OFF"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?
How about specifying only ARROW_JEMALLOC here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like this?

Suggested change
"ARROW_MIMALLOC": "OFF"

I think we should always specify one ON one OFF together (unless we are 100% sure about one value of them being specified earlier) - otherwise there could be potentially two allocators which is not desired?

Copy link
Member

Choose a reason for hiding this comment

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

We can enable multiple allocators at the same time. :-)

The default allocator is chosen based on our recommendation:

arrow/cpp/src/arrow/memory_pool.cc

Line 143 in 5181c24

struct SupportedBackend default_backend = SupportedBackends().front();

arrow/cpp/src/arrow/memory_pool.cc

Lines 86 to 100 in 5181c24

const std::vector<SupportedBackend>& SupportedBackends() {
static std::vector<SupportedBackend> backends = {
// mimalloc is our preferred allocator for several reasons:
// 1) it has good performance
// 2) it is well-supported on all our main platforms (Linux, macOS, Windows)
// 3) it is easy to configure and has a consistent API.
#ifdef ARROW_MIMALLOC
{"mimalloc", MemoryPoolBackend::Mimalloc},
#endif
#ifdef ARROW_JEMALLOC
{"jemalloc", MemoryPoolBackend::Jemalloc},
#endif
{"system", MemoryPoolBackend::System}};
return backends;
}

But users can select any allocator from enabled allocators by creating arrow::MemoryPool instead of using arrow::default_memory_pool().

Copy link
Contributor Author

@zanmato1984 zanmato1984 Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, I see.

However I think we are expecting these allocator presets to enforce the allocator selection. That is, _allocator_X should mean "only enable allocator X, nothing else".

So for intentionally enabling both allocators, we can have another _allocator_all maybe?

Copy link
Member

Choose a reason for hiding this comment

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

OK. Let's use the approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But is _allocator_all useful enough to worth an individual cmake preset? (It makes the _allocator group intact though).

Copy link
Member

Choose a reason for hiding this comment

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

Can we use it for features-maximal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that makes much more sense. Updated. Also changed to use plural _allocators.

}
},
{
"name": "_allocator-mimalloc",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use this in features-basic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As replied in my other comment, yes we can. Will try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"hidden": true,
"cacheVariables": {
"ARROW_JEMALLOC": "OFF",
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Answered in my other comment.

"ARROW_MIMALLOC": "ON"
}
},
{
"name": "base",
"hidden": true,
Expand Down Expand Up @@ -46,34 +70,6 @@
"CMAKE_BUILD_TYPE": "RelWithDebInfo"
}
},
{
"name": "features-emscripten",
"hidden": true,
"cacheVariables": {
"ARROW_ACERO": "ON",
"ARROW_BUILD_SHARED": "OFF",
"ARROW_BUILD_STATIC": "ON",
"ARROW_CSV": "ON",
"ARROW_CUDA": "OFF",
"ARROW_DEPENDENCY_SOURCE": "BUNDLED",
"ARROW_DEPENDENCY_USE_SHARED": "OFF",
"ARROW_ENABLE_THREADING": "OFF",
"ARROW_FLIGHT": "OFF",
"ARROW_IPC": "ON",
"ARROW_JEMALLOC": "OFF",
"ARROW_JSON": "ON",
"ARROW_MIMALLOC": "OFF",
"ARROW_ORC": "ON",
"ARROW_RUNTIME_SIMD_LEVEL": "NONE",
"ARROW_S3": "OFF",
"ARROW_SIMD_LEVEL": "NONE",
"ARROW_SUBSTRAIT": "ON",
"ARROW_WITH_BROTLI": "ON",
"ARROW_WITH_OPENTELEMETRY": "OFF",
"ARROW_WITH_SNAPPY": "ON",
"CMAKE_C_BYTE_ORDER": "LITTLE_ENDIAN"
}
},
{
"name": "features-minimal",
"hidden": true,
Expand Down Expand Up @@ -103,7 +99,6 @@
"cacheVariables": {
"ARROW_SUBSTRAIT": "ON",
"ARROW_ACERO": "ON",
"ARROW_MIMALLOC": "ON",
"ARROW_PARQUET": "ON",
"ARROW_WITH_BROTLI": "ON",
"ARROW_WITH_BZ2": "ON",
Expand Down Expand Up @@ -174,15 +169,11 @@
{
"name": "features-python",
"inherits": [
"features-main"
"features-main",
"features-python-minimal"
],
"hidden": true,
"cacheVariables": {
"ARROW_COMPUTE": "ON",
"ARROW_CSV": "ON",
"ARROW_DATASET": "ON",
"ARROW_FILESYSTEM": "ON",
"ARROW_JSON": "ON",
"ARROW_ORC": "ON"
}
},
Expand All @@ -193,35 +184,54 @@
"features-filesystems",
"features-flight-sql",
"features-gandiva",
"features-main",
"features-python-minimal"
"features-python"
],
"hidden": true,
"cacheVariables": {
"ARROW_ORC": "ON",
"PARQUET_REQUIRE_ENCRYPTION": "ON"
}
},
{
"name": "features-maximal",
"inherits": [
"features-main",
"features-cuda",
"features-filesystems",
"features-flight-sql",
"features-gandiva",
"features-python-maximal"
],
"hidden": true,
"cacheVariables": {
"ARROW_BUILD_EXAMPLES": "ON",
"ARROW_BUILD_UTILITIES": "ON",
"ARROW_ORC": "ON",
"ARROW_SKYHOOK": "ON",
"ARROW_TENSORFLOW": "ON",
"PARQUET_BUILD_EXAMPLES": "ON",
"PARQUET_BUILD_EXECUTABLES": "ON",
"PARQUET_REQUIRE_ENCRYPTION": "ON"
"PARQUET_BUILD_EXECUTABLES": "ON"
}
},
{
"name": "features-emscripten",
Copy link
Member

Choose a reason for hiding this comment

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

Can we inherit _allocator-none here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

"hidden": true,
"cacheVariables": {
"ARROW_ACERO": "ON",
"ARROW_BUILD_SHARED": "OFF",
"ARROW_BUILD_STATIC": "ON",
"ARROW_CSV": "ON",
"ARROW_CUDA": "OFF",
"ARROW_DEPENDENCY_SOURCE": "BUNDLED",
"ARROW_DEPENDENCY_USE_SHARED": "OFF",
"ARROW_ENABLE_THREADING": "OFF",
"ARROW_FLIGHT": "OFF",
"ARROW_IPC": "ON",
"ARROW_JEMALLOC": "OFF",
"ARROW_JSON": "ON",
"ARROW_MIMALLOC": "OFF",
"ARROW_ORC": "ON",
"ARROW_RUNTIME_SIMD_LEVEL": "NONE",
"ARROW_S3": "OFF",
"ARROW_SIMD_LEVEL": "NONE",
"ARROW_SUBSTRAIT": "ON",
"ARROW_WITH_BROTLI": "ON",
"ARROW_WITH_OPENTELEMETRY": "OFF",
"ARROW_WITH_SNAPPY": "ON",
"CMAKE_C_BYTE_ORDER": "LITTLE_ENDIAN"
}
},
{
Expand Down Expand Up @@ -371,45 +381,97 @@
"cacheVariables": {}
},
{
"name": "ninja-debug-valgrind-basic",
"name": "ninja-debug-emscripten",
"inherits": [
"base-debug",
"features-basic",
"features-valgrind"
"features-emscripten"
],
"displayName": "Debug build for Valgrind with reduced dependencies",
"displayName": "Debug build which builds an Emscripten library",
"cacheVariables": {}
},
{
"name": "ninja-debug-valgrind",
"name": "ninja-debug-valgrind-minimal",
"inherits": [
"base-debug",
"features-main",
"features-valgrind"
"features-valgrind",
"ninja-debug-minimal"
],
"displayName": "Debug build for Valgrind with more optional components",
"displayName": "Debug build for Valgrind without anything enabled",
"cacheVariables": {}
},
{
"name": "ninja-debug-valgrind-minimal",
"name": "ninja-debug-valgrind-basic",
"inherits": [
"base-debug",
"features-minimal",
"features-valgrind"
"features-valgrind",
"ninja-debug-basic"
],
"displayName": "Debug build for Valgrind without anything enabled",
"displayName": "Debug build for Valgrind with tests and reduced dependencies",
"cacheVariables": {}
},
{
"name": "ninja-debug-valgrind",
"inherits": [
"features-valgrind",
"ninja-debug"
],
"displayName": "Debug build for Valgrind with tests and more optional components",
"cacheVariables": {}
},
{
"name": "ninja-debug-valgrind-maximal",
"inherits": [
"base-debug",
"features-maximal",
"features-valgrind"
"features-valgrind",
"ninja-debug-maximal"
],
"displayName": "Debug build for Valgrind with tests and everything enabled",
"cacheVariables": {}
},
{
"name": "ninja-debug-asan",
"inherits": [
"_allocator-none",
"ninja-debug",
"sanitizer-asan"
],
"displayName": "Debug build for ASAN with tests and more optional components",
"cacheVariables": {}
},
{
"name": "ninja-debug-tsan",
"inherits": [
"_allocator-none",
"ninja-debug",
"sanitizer-tsan"
],
"displayName": "Debug build for TSAN with tests and more optional components",
"cacheVariables": {}
},
{
"name": "ninja-debug-ubsan",
"inherits": [
"_allocator-none",
"ninja-debug",
"sanitizer-ubsan"
],
"displayName": "Debug build for Valgrind with everything enabled",
"displayName": "Debug build for UBSAN with tests and more optional components",
"cacheVariables": {}
},
{
"name": "fuzzing",
"inherits": [
"base",
"sanitizer-asan",
"sanitizer-ubsan"
],
"displayName": "Debug build with IPC and Parquet fuzzing targets",
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"CMAKE_C_COMPILER": "clang",
"CMAKE_CXX_COMPILER": "clang++",
"ARROW_IPC": "ON",
"ARROW_PARQUET": "ON",
"ARROW_FUZZING": "ON"
}
},
{
"name": "ninja-release-minimal",
"inherits": [
Expand Down Expand Up @@ -446,24 +508,6 @@
"displayName": "Release build with CUDA integration",
"cacheVariables": {}
},
{
"name": "ninja-debug-emscripten",
"inherits": [
"features-emscripten",
"base-debug"
],
"displayName": "Debug build which builds an Emscripten library",
"cacheVariables": {}
},
{
"name": "ninja-release-emscripten",
"inherits": [
"features-emscripten",
"base-release"
],
"displayName": "Release build which builds an Emscripten library",
"cacheVariables": {}
},
{
"name": "ninja-release-flight",
"inherits": [
Expand Down Expand Up @@ -527,6 +571,15 @@
"displayName": "Release build with everything enabled (except benchmarks)",
"cacheVariables": {}
},
{
"name": "ninja-release-emscripten",
"inherits": [
"features-emscripten",
"base-release"
],
"displayName": "Release build which builds an Emscripten library",
"cacheVariables": {}
},
{
"name": "ninja-benchmarks-basic",
"inherits": [
Expand All @@ -553,50 +606,6 @@
],
"displayName": "Benchmarking build with everything enabled",
"cacheVariables": {}
},
{
"name": "ninja-debug-asan",
"inherits": [
"ninja-debug",
"sanitizer-asan"
],
"displayName": "Debug ASAN build with tests and more optional components",
"cacheVariables": {}
},
{
"name": "ninja-debug-tsan",
"inherits": [
"ninja-debug",
"sanitizer-tsan"
],
"displayName": "Debug TSAN build with tests and more optional components",
"cacheVariables": {}
},
{
"name": "ninja-debug-ubsan",
"inherits": [
"ninja-debug",
"sanitizer-ubsan"
],
"displayName": "Debug UBSAN build with tests and more optional components",
"cacheVariables": {}
},
{
"name": "fuzzing",
"inherits": [
"base",
"sanitizer-asan",
"sanitizer-ubsan"
],
"displayName": "Debug build with IPC and Parquet fuzzing targets",
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"CMAKE_C_COMPILER": "clang",
"CMAKE_CXX_COMPILER": "clang++",
"ARROW_IPC": "ON",
"ARROW_PARQUET": "ON",
"ARROW_FUZZING": "ON"
}
}
]
}
Loading