GH-50784: [C++] Align write in TransferBitmap - #50785
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR optimizes TransferBitmap (used by CopyBitmap / InvertBitmap) by aligning the destination bit offset up-front so the main transfer loop can use a writer that assumes byte alignment, reducing per-word split/write overhead for unaligned cases.
Changes:
- Added a shared
TransferReaderWriterhelper to consolidate the word/trailing-byte transfer loops. - Updated
TransferBitmapto copy a small prefix whendest_offsetis bit-unaligned, makingdest_offsetbyte-aligned for the remainder of the transfer. - Switched the main unaligned-read path to use
BitmapWordWriter<uint64_t, /*may_have_byte_offset=*/false>once the destination is aligned.
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 10c08f4. 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. |
|
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 10c08f4. There were 8 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 10c08f4. There were 8 benchmark results indicating a performance regression:
The full Conbench report has more details. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/arrow/util/bitmap_ops.cc:229
- In the generic (non-identity) branch,
last_datais computed using~data[...]instead of the providedop. This happens to work forstd::bit_not<>, but it makes the template internally inconsistent and would be incorrect ifMapBitmapUnarywere reused with any other unary op.
last_data = ~data[num_bytes - 1];
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cpp/src/arrow/util/bitmap_ops.cc:238
MapBitmapUnaryappliesOpto all full bytes but hard-codeslast_data = ~data[...]for the final (possibly partial) byte. This makes the template incorrect for anyOpother than bitwise-not and breaks the stated generic behavior.
constexpr auto op = Op{};
for (int64_t i = 0; i < num_bytes - 1; i++) {
dest[i] = static_cast<uint8_t>(op(data[i]));
}
last_data = op(data[num_bytes - 1]);
cpp/src/arrow/util/bitmap_ops.cc:179
- Docstring typos/wording: “sace” -> “save”; “non bit-aligned input and outputs” -> “non-bit-aligned inputs and outputs”; and this code is aligning to a byte boundary, so “bit-align the writer” is misleading.
/// Map inputs with a given operation and sace to output.
///
/// This function assumes general non bit-aligned input and outputs.
/// It will first process less than a byte in order to bit-align the writer, and then
/// keep on going with an aligned writer.
cpp/src/arrow/util/bitmap_ops.cc:130
- Docstring grammar: “All readers and writer” should read “All readers and the writer”, and “as many input” should be “as many inputs”.
This issue also appears on line 175 of the same file.
/// Map output from readers and save it with the writer.
///
/// All readers and writer must span over the same number of values.
///
/// @tparam Op a function of as many input as there are readers.
cpp/src/arrow/util/bitmap_ops.cc:169
BitmapPtr::operator+doesn’t mutate state and should beconstso it can be used on const-qualified instances (and better reflects intent).
This issue also appears on line 234 of the same file.
BitmapPtr operator+(int64_t extra) { return {.data = data, .offset = offset + extra}; }
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/arrow/util/bitmap_ops.cc:175
- Typo in the new doc comment: "sace" should be "save".
/// Map inputs with a given operation and sace to output.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/arrow/util/bitmap_ops.cc:175
- Typo in doc comment: "sace" should be "save".
/// Map inputs with a given operation and sace to output.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cpp/src/arrow/util/bitmap_ops.cc:175
- Typo in doc comment: "sace" should be "save".
/// Map inputs with a given operation and sace to output.
cpp/src/arrow/util/bitmap_ops.cc:169
BitmapPtr::operator+isn'tconst, which prevents using it withconst BitmapPtrvalues (and makes it harder to reuse this helper safely). Making itconst(and optionallyconstexpr) keeps the API flexible without changing behavior.
BitmapPtr operator+(int64_t extra) { return {.data = data, .offset = offset + extra}; }
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 5f47de7. 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. |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 5f47de7. There were 8 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
These benchmark results make no sense here. I think we lost the initial Locally, I see great improvements (~80% now) for bitmap unary operations (Copy/Invert) when both are offset. What do you think of these changes @cyb70289, seems you were among the last involved here. |
cyb70289
left a comment
cyb70289
left a comment
There was a problem hiding this comment.
Nice improvement, LGTM.
github-actions
Bot
added
awaiting committer review
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
I don't see a meaningful difference on AMD Zen 2, but LGTM anyway.
### Rationale for this change The R macOS CRAN nightly stopped building Arrow C++ after #50785. It uses the macOS 11.3 SDK to match CRAN's builder, and fails on one file with "no template named 'identity' in namespace 'std'" at lines 230, 307 and 323. That SDK's libc++ predates the standard identity functor, which arrived in libc++ 12 with the macOS 12 SDK, so the name is absent whatever standard is selected. The cran-m1 job uses a current SDK and passes, which is why only this builder is red. Before this change the R package cannot be built on CRAN's macOS builder. After it, the file compiles there. Closes #50987. ### What changes are included in this PR? That file is the only one in the C++ tree naming the missing symbol, at the three sites above. It now uses a small equivalent functor in the same translation unit, so the compile-time fast path that skips per-bit work still matches and the bitmap copy keeps its memcpy shortcut. The bitwise-not functor is older and untouched. No behavior or API change. ### Are these changes tested? Reproduced on a real macOS 11.3 SDK, whose internal identity helper sits at the same header line cited in the CI log, confirming the same libc++. From a checkout of this branch: ```console $ curl -fsSL https://github.com/phracker/MacOSX-SDKs/releases/download/11.3/MacOSX11.3.sdk.tar.xz | tar -xJ -C /tmp $ SDK=/tmp/MacOSX11.3.sdk $ F=cpp/src/arrow/util/bitmap_ops.cc $ clang++ -std=gnu++20 -isysroot "$SDK" -I cpp/src -fsyntax-only "$F" && echo ok ok $ git checkout upstream/main -- "$F" $ clang++ -std=gnu++20 -isysroot "$SDK" -I cpp/src -fsyntax-only "$F" 230:57: error: no template named 'identity' in namespace 'std' 307:23: error: no template named 'identity' in namespace 'std' 323:30: error: no template named 'identity' in namespace 'std' 6 errors generated. ``` Behavior is unchanged. The bit utility test target builds and its bitmap copy, invert and reverse cases pass, covering both branches of the mapping helper: the byte-aligned memcpy path and the bit-offset path that calls the functor. ``` [ PASSED ] 4 tests. ``` The local run compiles the failing translation unit, not the full R build, which the CRAN job here covers. ### Are there any user-facing changes? No. * GitHub Issue: #50987 Lead-authored-by: 1fanwang <1fannnw@gmail.com> Co-authored-by: Nic Crane <thisisnic@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
Faster without much more complexity.
Locally (Macbook Pro M3), I'm getting 100% speedup on
CopyBitmapWithOffsetBoth.What changes are included in this PR?
Align the writer in transfer bitmap for more efficient writes in bitmap unary and binary operation.
Simplify the use of bit functions as templates.
Are these changes tested?
Yes with existing tests.
Are there any user-facing changes?
No.
CopyBitmap/InvertBitmap#50784