Skip to content

DO NOT MERGE: EXP: [C++] VisitTwoBitRuns changes #50392

Draft
pitrou wants to merge 5 commits into
apache:mainfrom
pitrou:visit_two_bit_runs_exp
Draft

DO NOT MERGE: EXP: [C++] VisitTwoBitRuns changes #50392
pitrou wants to merge 5 commits into
apache:mainfrom
pitrou:visit_two_bit_runs_exp

Conversation

@pitrou

@pitrou pitrou commented Jul 6, 2026

Copy link
Copy Markdown
Member

Based on #50281

@pitrou pitrou changed the title Visit two bit runs exp DO NOT MERGE: EXP: [C++] VisitTwoBitRuns changes Jul 6, 2026
@github-actions github-actions Bot added the awaiting review Awaiting review label Jul 6, 2026
@pitrou

pitrou commented Jul 6, 2026

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@rok

rok commented Jul 6, 2026

Copy link
Copy Markdown
Member

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

@conbench-apache-arrow

Copy link
Copy Markdown

Thanks for your patience. Conbench analyzed the 2 benchmarking runs that have been run so far on PR commit 710cedd.

There were 48 benchmark results indicating a performance regression:

The full Conbench report has more details.

@HuaHuaY

HuaHuaY commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

The results were somewhat unexpected. My standard local development environment is a MacBook, but I was unable to reproduce the test-mac-arm benchmark results.

@pitrou

pitrou commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Yes, compiler code generation can be quite fickle. But overall it seems to me that those benchmark numbers point to a significant improvement.

@HuaHuaY

HuaHuaY commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

But overall it seems to me that those benchmark numbers point to a significant improvement.

Could you please explain how to interpret this report? It seems to me that there are many performance regressions when there are no null values.

Incidentally, adding ARROW_FORCE_INLINE to VisitSetBitRuns provides some optimization for ArrayArrayKernel in my local environment. However, I'm unsure if this optimization always applies to other scenarios.

@pitrou

pitrou commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Could you please explain how to interpret this report? It seems to me that there are many performance regressions when there are no null values.

The ArrayArrayKernel benchmarks but also the ScalarTemporalBinary benchmarks exercise VisitTwoArrayValuesInline. The fact that some show regressions and some improvements hints that the compiler is not always able to do a good job here.

However, there are also a number of large improvements on the Filter benchmarks.

Incidentally, adding ARROW_FORCE_INLINE to VisitSetBitRuns provides some optimization for ArrayArrayKernel in my local environment.

Hmm, I could try it in this PR.

@pitrou

pitrou commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@ursabot please benchmark lang=C++

@rok

rok commented Jul 7, 2026

Copy link
Copy Markdown
Member

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

@pitrou

pitrou commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Could you please explain how to interpret this report?

Oh, I forgot: the z-score measures statistical significance. I personally only look at the numbers where abs(z) > 3.
(ideally there should be a way to hide non-significant results @rok )

@rok

rok commented Jul 7, 2026

Copy link
Copy Markdown
Member

@pitrou yes, having a filter like that sounds like a good idea :)

@conbench-apache-arrow

Copy link
Copy Markdown

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

There were 41 benchmark results indicating a performance regression:

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants