Skip to content

Conversation

@pitrou
Copy link
Member

@pitrou pitrou commented Mar 26, 2025

Rationale for this change

Benchmarks fail compiling with Google Benchmark 1.9.2. The errors seem triggered by this change upstream: google/benchmark#1948

What changes are included in this PR?

  • Move some benchmark functions to a dedicated namespace so that there is no ambiguity with other functions
  • Reduce reliance on templating in benchmark functions to avoid resolution ambiguities
  • Other required fixes

Note that we don't rename the benchmark functions, to avoid breaking continuous benchmarking history.

Are these changes tested?

By continuous benchmarking.

Are there any user-facing changes?

No.

@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

@ursabot please benchmark lang=C++

@ursabot
Copy link

ursabot commented Mar 26, 2025

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

@github-actions
Copy link

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

@github-actions
Copy link

⚠️ GitHub issue #45939 has no components, please add labels for components.

@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

@ursabot please benchmark lang=C++

@ursabot
Copy link

ursabot commented Mar 26, 2025

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

@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

@ursabot please benchmark lang=C++

@ursabot
Copy link

ursabot commented Mar 26, 2025

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

@conbench-apache-arrow
Copy link

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

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

The full Conbench report has more details.

@conbench-apache-arrow
Copy link

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

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

The full Conbench report has more details.

@conbench-apache-arrow
Copy link

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

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

The full Conbench report has more details.

@pitrou pitrou force-pushed the gh45939-benchmark-fix branch from b8d6cd5 to 048aed0 Compare March 26, 2025 11:05
@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

@ursabot please benchmark lang=C++

@ursabot
Copy link

ursabot commented Mar 26, 2025

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

@pitrou pitrou marked this pull request as ready for review March 26, 2025 11:15
Benchmarks fail compiling with Google Benchmark 1.9.2.
This is because the benchmark function names conflict with Arrow API function names.

We don't want to break continuous benchmarking history by changing the benchmark names, so instead move the benchmarks to a dedicated namespace.
@pitrou pitrou force-pushed the gh45939-benchmark-fix branch from 048aed0 to d6fa8e4 Compare March 26, 2025 12:25
@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

@ursabot please benchmark lang=C++

@ursabot
Copy link

ursabot commented Mar 26, 2025

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

@pitrou pitrou requested a review from zanmato1984 March 26, 2025 12:43
@pitrou pitrou changed the title GH-45939: [C++][Benchmarking] Fix compilation failure GH-45939: [C++][Benchmarking] Fix compilation failures Mar 26, 2025
@conbench-apache-arrow
Copy link

Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 048aed0c2d85cba8b4f17a695a1874b4fdf06c19.

There were 60 benchmark results indicating a performance regression:

The full Conbench report has more details.

@conbench-apache-arrow
Copy link

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

There were 62 benchmark results indicating a performance regression:

The full Conbench report has more details.

@pitrou
Copy link
Member Author

pitrou commented Mar 26, 2025

The benchmark builds have all passed now. The perf regressions (or pseudo-regressions) are unrelated.

Copy link
Contributor

@zanmato1984 zanmato1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for fixing the failure so quickly!

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 26, 2025
@pitrou pitrou merged commit 9b7875c into apache:main Mar 26, 2025
39 of 40 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Mar 26, 2025
@pitrou pitrou deleted the gh45939-benchmark-fix branch March 26, 2025 17:00
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

zanmato1984 pushed a commit to zanmato1984/arrow that referenced this pull request Apr 15, 2025
…45942)

### Rationale for this change

Benchmarks fail compiling with Google Benchmark 1.9.2. The errors seem triggered by this change upstream: google/benchmark#1948

### What changes are included in this PR?

* Move some benchmark functions to a dedicated namespace so that there is no ambiguity with other functions
* Reduce reliance on templating in benchmark functions to avoid resolution ambiguities
* Other required fixes

Note that we don't rename the benchmark functions, to avoid breaking continuous benchmarking history.

### Are these changes tested?

By continuous benchmarking.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#45939

Authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants