Copilot commented on code in PR #13368:
URL: https://github.com/apache/gluten/pull/13368#discussion_r4348075677
##########
cpp/velox/compute/VeloxBackend.cc:
##########
@@ -259,9 +259,17 @@ 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);
+ const int32_t broadcastHashTableBuildThreads = std::max<int32_t>(1, [&]() {
+ if (!backendConf_->valueExists(kBroadcastHashTableBuildThreads)) {
+ LOG(WARNING) << kBroadcastHashTableBuildThreads << " is not set. Falling
back to " << kNumTaskSlotsPerExecutor
+ << " (" << numTaskSlotsPerExecutor << ").";
+ return numTaskSlotsPerExecutor;
+ }
+ return backendConf_->get<int32_t>(kBroadcastHashTableBuildThreads).value();
+ }());
+ if (broadcastHashTableBuildThreads > 1) {
+ broadcastHashTableBuildExecutor_ =
std::make_unique<folly::CPUThreadPoolExecutor>(broadcastHashTableBuildThreads);
+ }
Review Comment:
The warning message can be misleading when `numTaskSlotsPerExecutor` is `0`
(log says fallback to 0, but `std::max(1, ...)` resolves it to 1 and results in
the serial path). Consider logging the *resolved* thread count (after applying
the minimum) and/or adjusting the message to mention the minimum-of-1 behavior.
Also consider reducing verbosity (e.g., INFO/VLOG or logging once) since this
can be emitted on every backend initialization when the setting is
intentionally omitted and resolved elsewhere.
##########
cpp/velox/tests/VeloxBackendTest.cc:
##########
@@ -18,31 +18,60 @@
#include <folly/executors/CPUThreadPoolExecutor.h>
#include <folly/futures/Future.h>
#include <gtest/gtest.h>
+#include <optional>
+#include <string>
#include <thread>
+#include <unordered_map>
#include "compute/VeloxBackend.h"
#include "config/VeloxConfig.h"
namespace gluten {
-class VeloxBackendTest : public ::testing::Test {
+struct BroadcastBuildThreadsTestParam {
+ int taskSlots;
+ std::optional<int> buildThreads;
+ std::optional<int> expectedThreads;
+};
+
+class VeloxBackendTest : public
::testing::TestWithParam<BroadcastBuildThreadsTestParam> {
protected:
- static void SetUpTestSuite() {
- VeloxBackend::create(AllocationListener::noop(),
{{kNumTaskSlotsPerExecutor, "0"}});
+ void SetUp() override {
+ const auto& param = GetParam();
+ std::unordered_map<std::string, std::string>
conf{{kNumTaskSlotsPerExecutor, std::to_string(param.taskSlots)}};
+ if (param.buildThreads.has_value()) {
+ conf.emplace(kBroadcastHashTableBuildThreads,
std::to_string(param.buildThreads.value()));
+ }
+ VeloxBackend::create(AllocationListener::noop(), conf);
}
- static void TearDownTestSuite() {
+ void TearDown() override {
VeloxBackend::get()->tearDown();
}
Review Comment:
This test now calls `VeloxBackend::create(...)` in `SetUp()` for every
parameter, but `TearDown()` only calls `VeloxBackend::get()->tearDown()` and
does not obviously release/reset the backend singleton. If `create()` is a
one-time initializer (common for singletons), later parameter sets may not
actually reinitialize the backend with new conf values, making the
parameterized cases unreliable/flaky. A more robust approach is to either (a)
add a test-only reset/destroy API and use it here, or (b) keep a single
`create()` in `SetUpTestSuite()` and structure tests so they don’t require
recreating the backend per case.
--
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]
