Copilot commented on code in PR #13115:
URL: https://github.com/apache/gluten/pull/13115#discussion_r4280570828
##########
cpp/velox/compute/VeloxBackend.cc:
##########
@@ -254,6 +255,10 @@ void VeloxBackend::init(
std::make_unique<folly::CPUThreadPoolExecutor>(ioThreads,
folly::CPUThreadPoolExecutor::makeLifoSemQueue());
}
+ const auto broadcastHashTableBuildThreads =
+ std::max<int32_t>(1,
backendConf_->get<int32_t>(kBroadcastHashTableBuildThreads,
numTaskSlotsPerExecutor));
+ broadcastHashTableBuildExecutor_ =
std::make_unique<folly::CPUThreadPoolExecutor>(broadcastHashTableBuildThreads);
Review Comment:
This creates an additional CPU thread pool unconditionally during backend
init. Even if parallel broadcast build is rarely used, this increases the
process thread count and can lead to CPU oversubscription (especially since the
previous behavior reused an existing executor). Consider lazy-initializing the
pool on first use (e.g., via `std::call_once` inside
`broadcastHashTableBuildExecutor()`), or constructing it only when parallel
build is enabled/needed, to keep thread footprint proportional to actual usage.
##########
cpp/velox/tests/VeloxBackendTest.cc:
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <folly/executors/CPUThreadPoolExecutor.h>
+#include <folly/futures/Future.h>
+#include <gtest/gtest.h>
+#include <thread>
+
+#include "compute/VeloxBackend.h"
+#include "config/VeloxConfig.h"
+
+namespace gluten {
+
+class VeloxBackendTest : public ::testing::Test {
+ protected:
+ static void SetUpTestSuite() {
+ VeloxBackend::create(AllocationListener::noop(),
{{kNumTaskSlotsPerExecutor, "0"}});
+ }
+
+ static void TearDownTestSuite() {
+ VeloxBackend::get()->tearDown();
+ }
+};
+
+TEST_F(VeloxBackendTest, broadcastHashTableBuildExecutorWithZeroTaskSlots) {
+ auto* backend = VeloxBackend::get();
+ auto* executor =
dynamic_cast<folly::CPUThreadPoolExecutor*>(backend->broadcastHashTableBuildExecutor());
+ ASSERT_NE(executor, nullptr);
+ ASSERT_EQ(executor->numThreads(), 1);
+ const auto callerThread = std::this_thread::get_id();
+ EXPECT_NE(folly::via(executor, [] { return std::this_thread::get_id();
}).get(), callerThread);
+}
Review Comment:
This test relies on `dynamic_cast` to `folly::CPUThreadPoolExecutor`, which
depends on RTTI being enabled and ties the test to a specific concrete executor
type. To make the test more robust, consider exposing a more specific return
type from `VeloxBackend::broadcastHashTableBuildExecutor()` (e.g.,
`folly::ThreadPoolExecutor*`) or adding a small test-only/inspectable API (like
a thread-count accessor) so the test can avoid RTTI while still validating
sizing and execution off the caller thread.
##########
docs/velox-configuration.md:
##########
@@ -16,6 +16,7 @@ nav_order: 16
| spark.gluten.sql.columnar.backend.velox.abandonPartialAggregationMinPct
| 🔄 Dynamic | 90 | If partial aggregation aggregationPct
greater than this value, partial aggregation may be early abandoned. Note: this
option only works when flushable partial aggregation is enabled. Ignored when
spark.gluten.sql.columnar.backend.velox.flushablePartialAggregation=false.
|
| spark.gluten.sql.columnar.backend.velox.abandonPartialAggregationMinRows
| 🔄 Dynamic | 100000 | If partial aggregation input rows
number greater than this value, partial aggregation may be early abandoned.
Note: this option only works when flushable partial aggregation is enabled.
Ignored when
spark.gluten.sql.columnar.backend.velox.flushablePartialAggregation=false.
|
| spark.gluten.sql.columnar.backend.velox.asyncTimeoutOnTaskStopping
| âš“ Static | 30000ms | Timeout in milliseconds when waiting
for runtime-scoped async work to finish during teardown.
|
+| spark.gluten.sql.columnar.backend.velox.broadcastHashTableBuildThreads
| âš“ Static | <undefined> | Size of the dedicated CPU thread pool
for the parallel broadcast hash-table build. By default, the thread pool is
sized to the maximum task slots per Spark executor on executors, or to
spark.driver.cores on the driver (used when driver-side broadcast hash-table
build is enabled), with a minimum of one thread.
|
Review Comment:
The 'Default' column says `<undefined>`, but the description states a
concrete default behavior (derived from task slots / `spark.driver.cores`, min
1). To avoid confusion for users, consider updating the default column to
reflect the computed default (e.g., `numTaskSlotsPerExecutor (min 1)` / driver
override) or rewording the description to clearly distinguish between 'no
explicit config set' vs 'effective default used'.
##########
cpp/velox/compute/VeloxBackend.cc:
##########
@@ -254,6 +255,10 @@ void VeloxBackend::init(
std::make_unique<folly::CPUThreadPoolExecutor>(ioThreads,
folly::CPUThreadPoolExecutor::makeLifoSemQueue());
}
+ const auto broadcastHashTableBuildThreads =
+ std::max<int32_t>(1,
backendConf_->get<int32_t>(kBroadcastHashTableBuildThreads,
numTaskSlotsPerExecutor));
+ broadcastHashTableBuildExecutor_ =
std::make_unique<folly::CPUThreadPoolExecutor>(broadcastHashTableBuildThreads);
Review Comment:
The PR description says this follows the existing `std::call_once` pattern
(like `getReaderThreadPool()`), but the executor is currently constructed
eagerly in `init()` and `broadcastHashTableBuildExecutor()` is a simple
accessor. Either update the implementation to match the described
`call_once`/lazy-init approach, or adjust the PR description to reflect the
eager-init behavior.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
