Punisheroot commented on PR #50710:
URL: https://github.com/apache/arrow/pull/50710#issuecomment-5123630193
Thanks for the Conbench report. I investigated all 15 possible regressions
reported for PR commit 74acebbf0344efcdc9f74593040d43fc2dbb3603 .
The follow-up fix described below is included in commit
59e8845c3f038588835ddefec4900e66a5ee71ec.
## Root cause
Six regressions affected `BM_ArrowBinaryPlain` on the
`amd64-c6a-4xlarge-linux` runner:
- `DecodeArrow_Dense`: 4,096, 32,768, and 65,536 values
- `DecodeArrowNonNull_Dense`: 4,096, 32,768, and 65,536 values
These regressions were real but indirect. The Plain decoding algorithm was
not
changed. Instead, the additional dictionary decoding code changed GCC 14's
inlining decision for `decoder.cc`. GCC stopped inlining the existing
`ArrowBinaryHelper::AppendValue` implementation into the Plain decode loop,
introducing an out-of-line call for every decoded value.
I confirmed this by comparing the generated GCC 14 code for the baseline, the
original PR, and the follow-up fix.
## Classification of the other results
Seven alerts belong to separate Arrow benchmark executables: the two temporal
cases, the ARM allocator case, and the four bit/cast/unpacking cases on the
m5
runner. These executables do not link `libparquet`, and none of the three
files
changed by this PR contributes code to them. Their measured deltas are
genuine
observations from those runs, but there is no source or binary dependency
through
which this PR could change their benchmark code.
The Boolean RLE and FLBA cases require a more cautious classification because
they are part of `parquet-encoding-benchmark` and therefore share the
executable
or `libparquet` affected by the PR. Neither executes the modified dictionary
path:
- the GCC 14 hot function body for Boolean RLE is byte-identical between the
baseline, original PR, and follow-up;
- FLBA calls `arrow::util::internal::ByteStreamSplitDecode` directly rather
than
the modified Parquet decoder.
Paired GCC 14 controls did not reproduce either regression. With the complete
baseline/PR/follow-up executables and libraries, the median changes versus
the
baseline were:
- Boolean RLE: original PR `-0.59%`, follow-up `-0.80%`;
- FLBA: original PR `+0.49%`, follow-up `-0.05%`.
A second experiment using the same executable for every comparison and
changing
only `libparquet` also remained within approximately 0.5%. These local
results
strongly suggest that the Conbench RLE and FLBA alerts were run- or
machine-specific. However, because code layout differs in the linked c6a
binary,
a new Apache-runner result remains the authoritative confirmation.
## Follow-up fix
The follow-up adds a narrowly scoped overload for the known-size `BinaryType`
append path:
- the common per-value path is explicitly kept inline;
- the rare chunk-rollover path is moved to a no-inline helper;
- the existing optional-size path remains unchanged for other callers;
- the dictionary predecode, validation, exact reservation, and reusable
scratch
buffer remain unchanged.
This restores the intended generated code without increasing GCC's global
inline budget. The final GCC 14 `decoder.cc` object contains 277 fewer bytes
of
text than the original PR object.
## Performance validation
I could not reproduce the exact Apache c6a host locally, so these
measurements
do not replace a new Conbench run. I used GCC 14.2, `-O2`, the c6a
optimization
flags, CPU pinning, rotated execution order, and paired comparisons.
For the six originally regressed Plain cases:
- follow-up versus baseline: 25.3% to 51.6% faster;
- follow-up versus the original PR: 38.8% to 66.6% faster.
The dictionary optimization remains intact:
- follow-up versus baseline: 29.0% to 63.9% faster;
- follow-up versus the original PR: median differences from `-0.56%` to
`+0.92%`, with no systematic loss.
All 76 registered `BM_ArrowBinary*` cases were screened in six paired rounds.
No repeatable regression remained. The only short-run outlier was `+2.70%`;
twelve longer pairs for that exact case produced a `-0.05%` median with six
positive and six negative results.
## Correctness validation
The exact final source passed:
- `parquet-encoding-test`: 136/136;
- `parquet-reader-test`: 149 passed, 5 skipped;
- `parquet-arrow-reader-writer-test`: 823 passed, 9 skipped;
- total: 1,108 passed, 14 skipped, 0 failed;
- 16 focused ASan/UBSan Plain and Dictionary cases, with no findings.
A disposable diagnostic build also forced the normally rare chunk rollover at
4 KiB and verified every emitted chunk against the expected array slices for
Plain and Dictionary decoding, with and without nulls.
## Relation to the original PR benchmarks
The original Windows/MSVC measurements remain valid for the dictionary cases
they measured: they showed a 17.2%–25.5% improvement at 65,536 values.
However,
they did not exercise the Linux/GCC Plain-path code-generation side effect
later
exposed by Conbench.
The follow-up therefore supplements the original measurements with the
missing
Linux/GCC controls. It preserves the dictionary improvement while removing
the
indirect Plain regression.
A new Conbench run on the Apache runners is still required for final
validation.
Thank you!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
