Copilot commented on code in PR #13115:
URL: https://github.com/apache/gluten/pull/13115#discussion_r4283534422
##########
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 states 'default behavior is unchanged — this is a
pool-isolation change, not a concurrency change.' While task *parallelism* may
be unchanged, this change *does* increase default resource usage by adding a
new pool of threads in addition to the existing ioExecutor. Please either (a)
adjust the description to reflect the increased default thread footprint, or
(b) change the implementation to avoid creating extra threads unless the
feature is actually 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:
This eagerly creates an additional thread pool sized (by default) similarly
to the existing executors, increasing the total thread count per process even
if parallel broadcast build is never used. Consider lazy-initializing this pool
on first use (e.g., call_once or equivalent) and/or only creating it when
parallel join build is enabled/configured, to avoid unnecessary thread overhead.
##########
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 accessor returns a generic folly::Executor*, but the implementation
always constructs a folly::CPUThreadPoolExecutor. This forces tests (and any
future callers needing thread-pool behavior) to use RTTI casts. Consider
tightening the stored type and accessor to folly::CPUThreadPoolExecutor (or
folly::ThreadPoolExecutor) to make the API explicit and avoid runtime casting.
##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/joins/GlutenParallelBroadcastJoinSuite.scala:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.
+ */
+package org.apache.spark.sql.execution.joins
+
+import org.apache.gluten.execution.{BroadcastHashJoinExecTransformer,
VeloxWholeStageTransformerSuite}
+
+import org.apache.spark.SparkConf
+import org.apache.spark.sql.execution.ColumnarBroadcastExchangeExec
+
+class GlutenParallelBroadcastJoinSuite extends VeloxWholeStageTransformerSuite
{
+
+ override protected val resourcePath: String = "placeholder"
Review Comment:
Using a literal value like \"placeholder\" for resourcePath makes the intent
unclear and can be brittle if the base suite expects a real resource directory
for resolution or logging. Prefer either (a) not overriding resourcePath if
it’s unused for this suite, or (b) setting it to a meaningful existing test
resource path consistent with other suites.
##########
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);
Review Comment:
This test relies on dynamic_cast (RTTI) to validate the executor type and
inspect numThreads(). If the project/toolchain is built with RTTI disabled,
this will fail to compile. A more robust approach is to expose a typed accessor
(e.g., returning folly::CPUThreadPoolExecutor*) or provide a small VeloxBackend
API to query the configured build thread count, avoiding RTTI in tests.
--
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]
