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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
