GH-47774: [C++] Force-inlince Status::~Status() to improve reading performance - #50749
GH-47774: [C++] Force-inlince Status::~Status() to improve reading performance#50749zhf999 wants to merge 1 commit into
Conversation
|
|
|
@ursabot please benchmark lang=C++ |
|
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. |
|
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. |
GH-47774: [C++] Force-inline
Status::~Statusto improve read performanceRationale 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.