GH-45591: [C++][Acero] Refine hash join benchmark and remove openmp from the project #45593

zanmato1984 · 2025-02-20T15:19:28Z

Rationale for this change

See #45591 .

What changes are included in this PR?

Replace the usage of openmp with arrow-native multi-threading primitives;
Remove all the occurrences of openmp from the project;
Support stats for build side rows in hash join benchmark, and update certain benchmark.

Are these changes tested?

Manually tested.

Are there any user-facing changes?

Removed a public CMake option but I think it shouldn't affect the user.

GitHub Issue: [C++][Acero] Refine hash join benchmark #45591

github-actions · 2025-02-20T15:19:57Z

⚠️ GitHub issue #45591 has been automatically assigned in GitHub to PR creator.

zanmato1984 · 2025-02-20T15:21:08Z

ci/scripts/cpp_build.sh

@@ -141,7 +141,6 @@ else
    -DARROW_BUILD_BENCHMARKS=${ARROW_BUILD_BENCHMARKS:-OFF} \
    -DARROW_BUILD_EXAMPLES=${ARROW_BUILD_EXAMPLES:-OFF} \
    -DARROW_BUILD_INTEGRATION=${ARROW_BUILD_INTEGRATION:-OFF} \
-    -DARROW_BUILD_OPENMP_BENCHMARKS=${ARROW_BUILD_OPENMP_BENCHMARKS:-OFF} \


IIRC, the hash join benchmark is never ran in our CI. And this is probably why.

zanmato1984 · 2025-02-20T15:21:15Z

cpp/src/arrow/acero/CMakeLists.txt

@@ -221,36 +221,21 @@ if(ARROW_BUILD_BENCHMARKS)

  add_arrow_acero_benchmark(aggregate_benchmark SOURCES aggregate_benchmark.cc)

-  if(ARROW_BUILD_OPENMP_BENCHMARKS)
-    find_package(OpenMP REQUIRED)


Feeling comfortable of removing one dependency.

I think so too!

zanmato1984 · 2025-02-20T15:29:29Z

@ursabot please benchmark

ursabot · 2025-02-20T15:29:37Z

Benchmark runs are scheduled for commit c27cf8a. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

kou

+1

kou · 2025-02-20T20:51:40Z

cpp/src/arrow/acero/hash_join_benchmark.cc

+        [&](std::function<Status(size_t)> task) -> Status {
+          return thread_pool_->Spawn([&, task]() { DCHECK_OK(task(thread_indexer_())); });
+        },
+        thread_pool_->GetCapacity(), settings.num_threads == 1));


Why do we need to change to thread_pool_->GetCapacity() (settings.num_threads) from 2 * settings.num_threads? Is the original 2 * settings.num_threads wrong?

This argument controls the max number of concurrent tasks in the scheduler, so any value >= settings.num_threads is fine. (The original doubling isn't wrong though.)

kou · 2025-02-20T21:07:46Z

cpp/src/arrow/acero/hash_join_benchmark.cc

-        settings.num_threads == 1));
+        /*thread_id=*/0,
+        [&](std::function<Status(size_t)> task) -> Status {
+          return thread_pool_->Spawn([&, task]() { DCHECK_OK(task(thread_indexer_())); });


Why did you choose Spawn() + conditional variable not Submit() + Future::status() (or Spawn() + ThreadPool::WaitForIdle())? Is it easy to write/maintain?

Oh right.

Spawn() + WaitForIdle() is easier and suffice (I was wrongly suspecting WaitForIdle() not working so changed to using cond var. But later it turned out the problem was something else and forgot to change it back.)

I'm updating it. Thank you for pointing this out.

kou · 2025-02-20T21:08:39Z

cpp/src/arrow/acero/CMakeLists.txt

@@ -221,36 +221,21 @@ if(ARROW_BUILD_BENCHMARKS)

  add_arrow_acero_benchmark(aggregate_benchmark SOURCES aggregate_benchmark.cc)

-  if(ARROW_BUILD_OPENMP_BENCHMARKS)
-    find_package(OpenMP REQUIRED)


I think so too!

conbench-apache-arrow · 2025-02-20T23:49:05Z

Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit c27cf8a.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

zanmato1984 · 2025-02-21T02:04:09Z

@ursabot please benchmark

ursabot · 2025-02-21T02:04:14Z

Benchmark runs are scheduled for commit 4cba288. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

kou · 2025-02-21T02:15:15Z

cpp/src/arrow/acero/hash_join_benchmark.cc

@@ -28,11 +28,11 @@
 #include "arrow/testing/random.h"
 #include "arrow/util/thread_pool.h"

+#include <condition_variable>


Can we remove this?

Oops, forgot this.

Removing.

kou · 2025-02-21T02:15:32Z

cpp/src/arrow/acero/hash_join_benchmark.cc

 #include <cstdint>
 #include <cstdio>
 #include <memory>
-
-#include <omp.h>
+#include <mutex>


Can we remove this too?

Done. Thanks for reminding!

zanmato1984 · 2025-02-21T06:04:14Z

Though the benchmark result is not ready yet, I've manually checked some of the finished ones and saw hash join benchmarks are executed as expected, w/o baseline as expected as well (they never run before).

So everything seems good now. I'm merging.

Thanks @kou for reviewing.

conbench-apache-arrow · 2025-02-21T09:07:12Z

Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 4cba288.

There was 1 benchmark result indicating a performance regression:

Pull Request Run on amd64-c6a-4xlarge-linux at 2025-02-21 03:01:17Z
- SetBitsTo (C++) with params=2, source=cpp-micro, suite=arrow-bit-util-benchmark

The full Conbench report has more details.

conbench-apache-arrow · 2025-02-21T12:51:45Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit b36659a.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 21 possible false positives for unstable benchmarks that are known to sometimes produce them.

…enmp from the project (apache#45593) ### Rationale for this change See apache#45591 . ### What changes are included in this PR? 1. Replace the usage of openmp with arrow-native multi-threading primitives; 2. Remove all the occurrences of openmp from the project; 3. Support stats for build side rows in hash join benchmark, and update certain benchmark. ### Are these changes tested? Manually tested. ### Are there any user-facing changes? Removed a public CMake option but I think it shouldn't affect the user. * GitHub Issue: apache#45591 Authored-by: Rossi Sun <[email protected]> Signed-off-by: Rossi Sun <[email protected]>

zanmato1984 requested review from assignUser, jonkeane, kou, raulcd and westonpace as code owners February 20, 2025 15:19

github-actions bot added Component: C++ awaiting review Awaiting review labels Feb 20, 2025

zanmato1984 commented Feb 20, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Feb 20, 2025

zanmato1984 force-pushed the refine-hash-join-benchmark branch from 97b4b22 to c27cf8a Compare February 20, 2025 15:27

zanmato1984 added 3 commits February 20, 2025 23:59

Replace openmp with arrow-native multi-threading primitives

00ef617

Fundamentally remove openmp from arrow

6bac837

Support stats for build side rows and use it in certain benchmark

492b429

zanmato1984 force-pushed the refine-hash-join-benchmark branch from c27cf8a to 492b429 Compare February 20, 2025 15:59

Fix link error on Windows for hash join benchmark

c6460ac

zanmato1984 force-pushed the refine-hash-join-benchmark branch from 06da9aa to c6460ac Compare February 20, 2025 16:55

kou approved these changes Feb 20, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Feb 20, 2025

Address review comments

4cba288

kou reviewed Feb 21, 2025

View reviewed changes

github-actions bot removed the awaiting merge Awaiting merge label Feb 21, 2025

github-actions bot added the awaiting changes Awaiting changes label Feb 21, 2025

Remove useless include

1771c04

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 21, 2025

zanmato1984 merged commit b36659a into apache:main Feb 21, 2025
39 checks passed

zanmato1984 removed the awaiting change review Awaiting change review label Feb 21, 2025

zanmato1984 mentioned this pull request Feb 21, 2025

[C++][Acero] Refine hash join benchmark #45591

Closed

zanmato1984 deleted the refine-hash-join-benchmark branch February 21, 2025 06:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-45591: [C++][Acero] Refine hash join benchmark and remove openmp from the project #45593

GH-45591: [C++][Acero] Refine hash join benchmark and remove openmp from the project #45593

zanmato1984 commented Feb 20, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Feb 20, 2025

zanmato1984 Feb 20, 2025

zanmato1984 Feb 20, 2025

kou Feb 20, 2025

zanmato1984 commented Feb 20, 2025

ursabot commented Feb 20, 2025

kou left a comment

kou Feb 20, 2025

zanmato1984 Feb 21, 2025

kou Feb 20, 2025

zanmato1984 Feb 21, 2025

zanmato1984 Feb 21, 2025

kou Feb 20, 2025

conbench-apache-arrow bot commented Feb 20, 2025

zanmato1984 commented Feb 21, 2025

ursabot commented Feb 21, 2025

kou Feb 21, 2025

zanmato1984 Feb 21, 2025

zanmato1984 Feb 21, 2025

kou Feb 21, 2025

zanmato1984 Feb 21, 2025

zanmato1984 commented Feb 21, 2025

conbench-apache-arrow bot commented Feb 21, 2025

conbench-apache-arrow bot commented Feb 21, 2025

GH-45591: [C++][Acero] Refine hash join benchmark and remove openmp from the project #45593

GH-45591: [C++][Acero] Refine hash join benchmark and remove openmp from the project #45593

Conversation

zanmato1984 commented Feb 20, 2025 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 commented Feb 20, 2025

ursabot commented Feb 20, 2025

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Feb 20, 2025

zanmato1984 commented Feb 21, 2025

ursabot commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 commented Feb 21, 2025

conbench-apache-arrow bot commented Feb 21, 2025

conbench-apache-arrow bot commented Feb 21, 2025

zanmato1984 commented Feb 20, 2025 •

edited by github-actions bot

Loading