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

zanmato1984 · 2025-01-09T07:47:41Z

Rationale for this change

I gave the reason in #45209

What changes are included in this PR?

Adjust the inheritance list order and make a lot of simplification/regrouping/reordering for existing presets the way I see fit.

Also introduce a soft rule to order inheritance list: sanitizer comes before features comes before base. Though we can't really mandate such a rule (thus it's "soft") or make it clear in comments (our current cmake dosn't support comment in json), we hope people will notice this pattern and follow it when editing this file.

Are these changes tested?

Manually tested.

Are there any user-facing changes?

None.

GitHub Issue: [C++][CMake] Allocators not disabled for sanitizer cmake presets #45209

github-actions · 2025-01-09T07:48:12Z

⚠️ GitHub issue #45209 has been automatically assigned in GitHub to PR creator.

zanmato1984 · 2025-01-09T07:49:20Z

cc @kou @wgtmac . My apology for not thoroughly testing my last PR and having to make this patch. Would you please take a look again? Thanks.

kou · 2025-01-09T09:31:25Z

Hmm. This will be easy to break... Because we want to sort lists in general and we can't add comments in JSON.

How about creating 000-disable-allocators or something preset and use it instead?

zanmato1984 · 2025-01-09T09:38:17Z

Hmm. This will be easy to break... Because we want to sort lists in general and we can't add comments in JSON.

How about creating 000-disable-allocators or something preset and use it instead?

Ah, that sounds nice. I'll try it and update soon. Thank you for the suggestion!

cpp/CMakePresets.json

zanmato1984 · 2025-01-09T19:19:26Z

Hi @kou , I've introduced a group of _allocator "mix-in"s. These mix-ins are not to be used as regular inheritance (e.g., sanitizers still specify allocators off for free usage), but rather to override/arbitrate any conflicting variables among different inherited presets (and the underscore prefix makes it the top of the list alphabetically). We can assume that any potential "arbitrator"s in the future (prefixed with underscore as well) won't overlap with each other so the order between them doesn't matter - as long as they are ahead of any "regular" (non-underscore-prefixed) presets, which is true. I think this way, things are less easy to break.

Once it comes to the day that enforcing override order is mandatory (let's hope that not!), we can revise these names and introduce number-prefix naming then. How do you think?

Plus, I also did some re-ordering/re-grouping/simplification to existing presets the way I see fit. Please also help to check.

cc @pitrou as well.

Thank you!

kou · 2025-01-10T01:04:58Z

cpp/CMakePresets.json

+      }
+    },
+    {
+      "name": "features-emscripten",


Can we inherit _allocator-none here?

kou · 2025-01-10T01:06:27Z

cpp/CMakePresets.json

@@ -6,6 +6,30 @@
    "patch": 0
  },
  "configurePresets": [
+    {
+      "name": "_allocator-none",


Can we use this in features-minimal too?

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.

kou · 2025-01-10T01:07:39Z

cpp/CMakePresets.json

+      }
+    },
+    {
+      "name": "_allocator-mimalloc",


Can we use this in features-basic?

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

kou · 2025-01-10T01:08:06Z

cpp/CMakePresets.json

+      "hidden": true,
+      "cacheVariables": {
+        "ARROW_JEMALLOC": "ON",
+        "ARROW_MIMALLOC": "OFF"


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

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?

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().

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?

OK. Let's use the approach.

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

Can we use it for features-maximal?

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

kou · 2025-01-10T01:08:13Z

cpp/CMakePresets.json

+      "name": "_allocator-mimalloc",
+      "hidden": true,
+      "cacheVariables": {
+        "ARROW_JEMALLOC": "OFF",


Answered in my other comment.

kou · 2025-01-10T01:11:59Z

cpp/CMakePresets.json

@@ -6,6 +6,30 @@
    "patch": 0
  },
  "configurePresets": [
+    {
+      "name": "_allocator-none",


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?

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.

kou

+1

Thanks!

kou · 2025-01-10T03:42:19Z

Could you update the PR description before you merge this?
We need to describe why we have _XXX.

zanmato1984 · 2025-01-10T03:58:12Z

Could you update the PR description before you merge this? We need to describe why we have _XXX.

Updated, thanks.

I'll also do some final verification for this change later before merging.

zanmato1984 · 2025-01-10T05:29:19Z

@github-actions crossbow submit -g cpp

zanmato1984 · 2025-01-10T05:29:35Z

@github-actions crossbow submit -g python

github-actions · 2025-01-10T05:31:49Z

Revision: 20da6bf

Submitted crossbow builds: ursacomputing/crossbow @ actions-a28de98f4d

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

github-actions · 2025-01-10T05:32:11Z

Revision: 20da6bf

Submitted crossbow builds: ursacomputing/crossbow @ actions-489c8994f2

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-python-3

zanmato1984 · 2025-01-10T06:56:58Z

Hi @kou , sorry to bother you again.

The test-ubuntu-22.04-cpp-emscripten failure is related, because the reordering from

      "name": "ninja-debug-emscripten",
      "inherits": [
        "features-emscripten",
        "base-debug"
      ],

to

      "name": "ninja-debug-emscripten",
      "inherits": [
        "base-debug",
        "features-emscripten"
      ],

messed up the ARROW_BUILD_STATIC (set to OFF in base-debug and to ON in features-emscripten, OFF won after the change, then test failed). This is essentially the same issue as the one the PR is trying to fix.

We can again tune the overriding of ARROW_BUILD_STATIC using the same way we did for allocators. However I want to take a step back considering about this question: if the order of elements in a list matters functionally, maybe we shouldn't enforce the alphabetical order at all? What do you think? Thanks.

kou · 2025-01-10T07:23:30Z

Hmm. I'm OK with it if we can use comment in the JSON...

In general, we should use sanitizers-* -> features-* -> base-* order to avoid this problem, right?

I think that 0-sanitizer-*, 1-features-*, base-* is better but it's not a strong opinion. If you like non-alphabetical order, I'm OK with it.

zanmato1984 · 2025-01-10T07:41:10Z

Thank you so much for the confirmation.

In general, we should use sanitizers-* -> features-* -> base-* order to avoid this problem, right?

Yes, that seems to be the general rule for inheritance.

I think that 0-sanitizer-*, 1-features-*, base-* is better but it's not a strong opinion. If you like non-alphabetical order, I'm OK with it.

I personally think introducing number prefix could be a bit overkill. And hope that the repeating occurrences of sanitizers-* -> features-* -> base-* pattern will hint people enough to realize that this is the intended order (though we can't really mandate it). Once we upgrade to cmake that is new enough to support comments, then we can add some description then.

I'll rework the PR, keeping the non-related refinement, and reducing the ordering stuff.

zanmato1984 · 2025-01-10T08:00:57Z

@github-actions crossbow submit -g cpp

zanmato1984 · 2025-01-10T08:01:05Z

@github-actions crossbow submit -g python

github-actions · 2025-01-10T08:03:26Z

Revision: 7150e71

Submitted crossbow builds: ursacomputing/crossbow @ actions-8d07c78518

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

github-actions · 2025-01-10T08:04:37Z

Revision: 7150e71

Submitted crossbow builds: ursacomputing/crossbow @ actions-212fbf47c1

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-python-3

zanmato1984 · 2025-01-10T08:49:04Z

@github-actions crossbow submit -g cpp python

github-actions · 2025-01-10T08:50:10Z

Unable to match any tasks for `python`
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/12706221844

zanmato1984 · 2025-01-10T08:51:21Z

@github-actions crossbow submit -g cpp

zanmato1984 · 2025-01-10T08:51:33Z

@github-actions crossbow submit -g python

github-actions · 2025-01-10T08:53:52Z

Revision: 424a9a2

Submitted crossbow builds: ursacomputing/crossbow @ actions-1e52e8655b

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

github-actions · 2025-01-10T08:54:13Z

Revision: 424a9a2

Submitted crossbow builds: ursacomputing/crossbow @ actions-06bc23cb35

Task	Status
example-python-minimal-build-fedora-conda
example-python-minimal-build-ubuntu-venv
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest-numpy-latest
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-latest-numpy-1.26
test-conda-python-3.11-pandas-latest-numpy-latest
test-conda-python-3.11-pandas-nightly-numpy-nightly
test-conda-python-3.11-pandas-upstream_devel-numpy-nightly
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.12-cpython-debug
test-conda-python-3.13
test-conda-python-3.9
test-conda-python-3.9-pandas-1.1.3-numpy-1.19.5
test-conda-python-emscripten
test-cuda-python-ubuntu-22.04-cuda-11.7.1
test-debian-12-python-3-amd64
test-debian-12-python-3-i386
test-fedora-39-python-3
test-ubuntu-22.04-python-3
test-ubuntu-22.04-python-313-freethreading
test-ubuntu-24.04-python-3

zanmato1984 · 2025-01-10T09:04:20Z

I've refined the code and validated that all the variables set in each preset are as expected.

Other than the allocators being disabled for sanitizer presets (as intended), there is another diff for ninja-debug-valgrind-minimal with ARROW_BUILD_INTEGRATION and ARROW_BUILD_TESTS changed from ON to OFF, which IMO is correct. I've also updated the PR description.

@kou would you mind to look again? Thanks.

kou

+1

We don't need to keep old descriptions with ~~XXX~~. Because we can see old descriptions by GitHub's UI. We can use only the latest description for commit message.

cpp/CMakePresets.json

Co-authored-by: Sutou Kouhei <[email protected]>

zanmato1984 · 2025-01-10T09:43:20Z

PR description updated.

zanmato1984 · 2025-01-10T15:13:57Z

The CI failures are not related. Merging.

I appreciate the comments and help from @kou , thank you!

Fix the issue that allocator not disabled for sanitizer cmake presets

732198c

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 9, 2025

A lot of reorganization of cmake presets

e55bdb8

zanmato1984 commented Jan 9, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 9, 2025

kou reviewed Jan 10, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jan 10, 2025

kou reviewed Jan 10, 2025

View reviewed changes

Reuse _allocator presets

452370c

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jan 10, 2025

Add preset for all allocators

20da6bf

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 10, 2025

kou approved these changes Jan 10, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 10, 2025

Revert allocator presets and adjust inheritance order

7150e71

Fix for maximal builds

424a9a2

kou approved these changes Jan 10, 2025

View reviewed changes

cpp/CMakePresets.json Outdated Show resolved Hide resolved

Update cpp/CMakePresets.json

1b67c7c

Co-authored-by: Sutou Kouhei <[email protected]>

zanmato1984 merged commit d0b11bb into apache:main Jan 10, 2025
32 of 33 checks passed

zanmato1984 removed the awaiting merge Awaiting merge label Jan 10, 2025

zanmato1984 mentioned this pull request Jan 10, 2025

[C++][CMake] Allocators not disabled for sanitizer cmake presets #45209

Closed

	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;
	}

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

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

Conversation

zanmato1984 commented Jan 9, 2025 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 9, 2025

zanmato1984 commented Jan 9, 2025

kou commented Jan 9, 2025

zanmato1984 commented Jan 9, 2025

zanmato1984 commented Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

kou commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025 • edited Loading

kou commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

github-actions bot commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

kou left a comment

Choose a reason for hiding this comment

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 10, 2025

zanmato1984 commented Jan 9, 2025 •

edited

Loading

zanmato1984 commented Jan 9, 2025 •

edited

Loading

zanmato1984 Jan 10, 2025 •

edited

Loading

zanmato1984 commented Jan 10, 2025 •

edited

Loading