Copilot commented on code in PR #13115:
URL: https://github.com/apache/gluten/pull/13115#discussion_r4270211579
##########
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:
New behavior is introduced for overriding the pool size via
`kBroadcastHashTableBuildThreads`, but the added C++ test only covers the
fallback when task slots are configured as 0. Please add a unit test case that
sets `kBroadcastHashTableBuildThreads` explicitly (e.g., to 2) and asserts
`numThreads()` matches the configured value.
##########
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 unconditionally creates (and typically starts) an additional CPU thread
pool for every VeloxBackend instance, increasing total thread count even when
parallel broadcast build is never used. Consider lazy-creating the pool on
first use (e.g., `std::call_once` similar to `getReaderThreadPool()`), or
deferring thread creation until parallel broadcast build is actually
enabled/triggered, to reduce baseline resource overhead.
##########
cpp/velox/compute/VeloxBackend.h:
##########
@@ -135,6 +142,7 @@ class VeloxBackend {
std::shared_ptr<facebook::velox::config::ConfigBase> backendConf_;
std::unique_ptr<ReaderThreadPool> readerThreadPool_;
+ std::unique_ptr<folly::Executor> broadcastHashTableBuildExecutor_;
Review Comment:
The executor is always constructed as a `folly::CPUThreadPoolExecutor`, but
is stored as `std::unique_ptr<folly::Executor>`, which forces tests (and any
future code) to rely on `dynamic_cast` to access pool-specific properties like
thread count. Consider storing it as
`std::unique_ptr<folly::CPUThreadPoolExecutor>` (while still returning
`folly::Executor*` from the accessor), or providing a small typed accessor for
introspection to avoid RTTI-dependent downcasts.
##########
cpp/velox/compute/VeloxBackend.h:
##########
@@ -53,6 +54,12 @@ class VeloxBackend {
ReaderThreadPool* getReaderThreadPool();
+ /// A dedicated CPU thread pool for the parallel broadcast hash-table build.
The build is
+ /// CPU-bound, so it must not share ioExecutor() which is reserved for async
IO tasks.
+ folly::Executor* broadcastHashTableBuildExecutor() const {
+ return broadcastHashTableBuildExecutor_.get();
+ }
Review Comment:
The PR description refers to a `VeloxBackend::hashTableBuildExecutor()` API,
but the actual API introduced is `broadcastHashTableBuildExecutor()`. To avoid
confusion for reviewers and future maintainers, please align the naming (either
update the PR description/docs to match, or rename the API if
`hashTableBuildExecutor` is the intended name).
--
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]
