You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AntoinePrv (via GitHub)" <gi...@apache.org> on 2025/07/21 13:37:44 UTC
[PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
AntoinePrv opened a new pull request, #47152:
URL: https://github.com/apache/arrow/pull/47152
Thanks for opening a pull request!
### Rationale for this change
Reviving #46034 and addressing reviwers comments.
@pitrou @kou @cyb70289
### What changes are included in this PR?
- Affinity detecttion is put as a ``CpuInfo`` member function, with fallback to -1
- `std::thread::hardware_concurency` failure fallback is moved to ``CpuInfo``
### Are these changes tested?
On CI
### Are there any user-facing changes?
No
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3096862139
Just a quick comment here (I'm about to leave on holiday): given that we would like to get rid of `CpuInfo`, we should put this functionality (affinity detection and concurrency detection) in `util/io_util.h` instead.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #47152:
URL: https://github.com/apache/arrow/pull/47152
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47152:
URL: https://github.com/apache/arrow/pull/47152#discussion_r2285709851
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -2219,6 +2220,21 @@ int64_t GetTotalMemoryBytes() {
#endif
}
+Result<uint32_t> GetNumAffinityCores() {
+#if defined(__linux__)
+ cpu_set_t mask;
+ if (sched_getaffinity(0, sizeof(mask), &mask) == 0) {
+ auto count = CPU_COUNT(&mask);
+ if (count > 0 &&
+ static_cast<uint64_t>(count) < std::numeric_limits<uint32_t>::max()) {
+ return {static_cast<uint32_t>(count)};
Review Comment:
I don't think you need the braces here?
##########
cpp/src/arrow/util/io_util.h:
##########
@@ -419,6 +419,12 @@ int64_t GetCurrentRSS();
ARROW_EXPORT
int64_t GetTotalMemoryBytes();
+/// \brief Get the number of affinity core on the system.
+///
+/// This is only implemented on Linux.
+/// If a value is returned, it is guaranteed to be greater or equal to one.
+ARROW_EXPORT Result<uint32_t> GetNumAffinityCores();
Review Comment:
How about returning `int32_t`? We usually avoid unsigned integer types in APIs.
##########
cpp/src/arrow/result.h:
##########
@@ -377,6 +377,13 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
return MoveValueUnsafe();
}
+ T ValueOr(T alternative) const& {
Review Comment:
Let's add a short docstring here?
```suggestion
/// Return the internally stored value or alternative if an error is stored.
T ValueOr(T alternative) const& {
```
##########
cpp/src/arrow/util/io_util_test.cc:
##########
@@ -1123,5 +1123,16 @@ TEST(Memory, TotalMemory) {
#endif
}
+TEST(CpuInfoTest, CpuAffinity) {
+ auto affinity_cores = GetNumAffinityCores();
+
+#ifdef __linux__
+ ASSERT_TRUE(affinity_cores.ok());
+ ASSERT_LE(affinity_cores.ValueOr(1u), std::thread::hardware_concurrency());
+#else
+ ASSERT_FALSE(affinity_cores.ok());
+#endif
Review Comment:
You could make this more Arrow-idiomatic:
```suggestion
auto maybe_affinity_cores = GetNumAffinityCores();
#ifdef __linux__
ASSERT_OK_AND_ASSIGN(affinity_cores, maybe_affinity_cores);
ASSERT_GE(affinity_cores, 1);
ASSERT_LE(affinity_cores, std::thread::hardware_concurrency());
#else
ASSERT_RAISES(NotImplemented, maybe_affinity_cores);
#endif
```
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -2219,6 +2220,21 @@ int64_t GetTotalMemoryBytes() {
#endif
}
+Result<uint32_t> GetNumAffinityCores() {
+#if defined(__linux__)
+ cpu_set_t mask;
+ if (sched_getaffinity(0, sizeof(mask), &mask) == 0) {
+ auto count = CPU_COUNT(&mask);
+ if (count > 0 &&
+ static_cast<uint64_t>(count) < std::numeric_limits<uint32_t>::max()) {
+ return {static_cast<uint32_t>(count)};
+ }
+ }
+ return {Status::IOError("Could not read the CPU affinity.")};
+#endif
Review Comment:
Using a `#else` clause here would probably be better to avoid warnings about dead code?
##########
cpp/src/arrow/util/io_util.cc:
##########
@@ -2219,6 +2220,21 @@ int64_t GetTotalMemoryBytes() {
#endif
}
+Result<uint32_t> GetNumAffinityCores() {
+#if defined(__linux__)
+ cpu_set_t mask;
+ if (sched_getaffinity(0, sizeof(mask), &mask) == 0) {
+ auto count = CPU_COUNT(&mask);
+ if (count > 0 &&
+ static_cast<uint64_t>(count) < std::numeric_limits<uint32_t>::max()) {
+ return {static_cast<uint32_t>(count)};
+ }
+ }
+ return {Status::IOError("Could not read the CPU affinity.")};
Review Comment:
Same here (no braces), but we should also give better error reporting using `errno`, see `IOErrorFromErrno`.
##########
cpp/src/arrow/util/io_util_test.cc:
##########
@@ -1123,5 +1123,16 @@ TEST(Memory, TotalMemory) {
#endif
}
+TEST(CpuInfoTest, CpuAffinity) {
Review Comment:
Let's remove the reference to CpuInfo?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3207588993
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 8509ca4d912f3f69e996df0f278d549c1d0c330b.
There weren't enough matching historic benchmark results to make a call on whether there were regressions.
The [full Conbench report](https://github.com/apache/arrow/runs/48518754734) has more details.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47152:
URL: https://github.com/apache/arrow/pull/47152#discussion_r2219263116
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -513,7 +526,18 @@ struct CpuInfo::Impl {
OsRetrieveCacheSize(&cache_sizes);
OsRetrieveCpuInfo(&hardware_flags, &vendor, &model_name);
original_hardware_flags = hardware_flags;
- num_cores = std::max(static_cast<int>(std::thread::hardware_concurrency()), 1);
+
+ num_cores = static_cast<int>(std::thread::hardware_concurrency());
+ if (num_cores <= 0) {
+ num_cores = 4;
+ ARROW_LOG(WARNING) << "Failed to determine the number of available threads, "
+ "using a hardcoded arbitrary value "
+ << num_cores;
+ }
Review Comment:
Moved from thread_pool.cc to standardize the detection of number of CPUs.
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -513,7 +526,18 @@ struct CpuInfo::Impl {
OsRetrieveCacheSize(&cache_sizes);
OsRetrieveCpuInfo(&hardware_flags, &vendor, &model_name);
original_hardware_flags = hardware_flags;
- num_cores = std::max(static_cast<int>(std::thread::hardware_concurrency()), 1);
+
+ num_cores = static_cast<int>(std::thread::hardware_concurrency());
+ if (num_cores <= 0) {
+ num_cores = 4;
+ ARROW_LOG(WARNING) << "Failed to determine the number of available threads, "
+ "using a hardcoded arbitrary value "
+ << num_cores;
+ }
Review Comment:
Moved from ``thread_pool.cc`` to standardize the detection of number of CPUs.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3205869203
CI failure is unrelated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3201412934
@AntoinePrv Please don't mention usernames in the PR description, as they will end up in the final commit message and trigger annoying notifications later on.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3096898707
Ha I understood from the previous discussion that this would remain the interface wrapping whichever underlying library.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47152:
URL: https://github.com/apache/arrow/pull/47152#discussion_r2219265204
##########
cpp/src/arrow/util/cpu_info.cc:
##########
@@ -550,7 +574,9 @@ const CpuInfo* CpuInfo::GetInstance() {
int64_t CpuInfo::hardware_flags() const { return impl_->hardware_flags; }
-int CpuInfo::num_cores() const { return impl_->num_cores <= 0 ? 1 : impl_->num_cores; }
Review Comment:
The check was and is unused.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3201409782
I have checked locally that it works (Ubuntu 24.04):
```console
$ python -c "import pyarrow as pa; print(pa.cpu_count())"
24
$ taskset -c 5,6,7 python -c "import pyarrow as pa; print(pa.cpu_count())"
3
$ OMP_NUM_THREADS=9 taskset -c 5,6,7 python -c "import pyarrow as pa; print(pa.cpu_count())"
9
$ OMP_THREAD_LIMIT=9 taskset -c 5,6,7 python -c "import pyarrow as pa; print(pa.cpu_count())"
3
$ OMP_THREAD_LIMIT=2 taskset -c 5,6,7 python -c "import pyarrow as pa; print(pa.cpu_count())"
2
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45860: [C++] Reboot: Respect CPU affinity in cpu_count and ThreadPool default capacity [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47152:
URL: https://github.com/apache/arrow/pull/47152#issuecomment-3097012557
It might indeed, but I'm not sure anything is settled in that regard. Affinity is also not really a CPU property.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org