Repository navigation
GH-47774: [C++] Force-inline Status::~Status() to improve reading performance - #50749
Merged
Merged
Conversation
|
|
Contributor
|
@ursabot please benchmark lang=C++ |
Member
|
Benchmark runs are scheduled for commit c445460. 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. |
Contributor
|
This PR looks interesting. I call the bot to run the benchmark and let's see whether there's a significant impact. |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit c445460. There were 35 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit c445460. There were 35 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit c445460. There were 35 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
The representative benchmark is:
The remote benchmark was run on Skylake (about 2017)
The local A/B results were:
The local result was therefore effectively unchanged |
|
I am not a CPU microarchitecture expert, so the following is only a hypothesis. I also used AI assistance while investigating it. The possible issue is that forcing I would like to learn and provide more info about my local run if asks |
Contributor
|
There are significant improvements in some benchmarks, such as
I use AI to summarize the results. @pitrou Please take a look and help analyze whether these benchmark results are acceptable. |
Contributor
|
Ping @pitrou. I think it is a simple yet interesting change. |
pitrou
force-pushed
the
status-destruction
branch
from
October 26, 2026 14:33
c445460 to
14c8d55
Compare
Member
|
@ursabot please benchmark |
Member
|
Benchmark runs are scheduled for commit 14c8d55. 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. |
Member
|
@github-actions crossbow submit -g cpp |
|
Revision: 14c8d55 Submitted crossbow builds: ursacomputing/crossbow @ actions-edf0518985
|
Member
|
I pushed an improvement to minimize the force-inlined portion of the destructor. I see healthy benefits locally against git main (only running Parquet benchmarks here): |
Member
|
I also see significant improvements in the JSON and CSV components, interestingly: |
pitrou
approved these changes
Oct 27, 2026
|
|
Member
|
The continuous benchmarking results are a bit difficult to interpret but they seem more on the speedup than slowdown side generally. |
github-actions
Bot
added
awaiting committer review
pitrou
changed the title
|
|
pitrou
reviewed
Oct 27, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Statusis returned by nearly every API on hot paths (e.g. IPC/Parquet reading), so its destructor runs extremely frequently. The destructor body is already optimized for the common success case: it only checksstate_ != NULL(withARROW_PREDICT_FALSE) and returns immediately for an OK status, delegating the rare error cleanup to the out-of-lineDeleteState().However, without an explicit inlining hint, some compilers/optimization levels may not inline this destructor, turning the trivial
state_ == NULLfast path into a real function call at every call site. Forcing it inline removes this call overhead on hot read paths and improves read throughput.In our perf test (reads a parquet file of 6.7GB),
Status::~Statuscost about 20 seconds, as the following flamegraph shows:After inlining
Status::~Status, the hotspot disappeared. End-to-end latency was also reduced by 20 seconds, as expected.What changes are included in this PR?
Status::~Status()incpp/src/arrow/status.hwithARROW_FORCE_INLINE(__attribute__((always_inline)) inlineon GCC/Clang,__forceinlineon MSVC), so the cheap null-check fast path is always inlined while the cold error path still goes through the non-inlinedDeleteState().Are these changes tested?
Yes, by existing tests. This change does not alter any behavior of
Status; the existingStatusunit tests and the full C++ test suite cover it. The performance improvement can be observed with the existing read-path benchmarks.Are there any user-facing changes?
No. This is an internal performance optimization with no API or behavior change.