Repository navigation
Conversation
richarddd
force-pushed
the
faster-overlapping-copy
branch
from
June 20, 2026 20:36
3551900 to
654bda0
Compare
Owner
|
Safe decode with Macbook M4 |
Owner
|
unsafe decode with Macbook M4 |
Owner
|
safe decode on AMD EPYC 7R32 unsafe decode |
richarddd
force-pushed
the
faster-overlapping-copy
branch
from
June 21, 2026 22:51
654bda0 to
423821a
Compare
Author
|
Good catch, you were right. The slowdown was the new doubling copy being slower than the plain byte loop for short overlapping matches, and that's the common case for normal data. The main decode loop uses that path for every match up to 18 bytes, so swapping the tight byte loop for variable-size memcpys there cost a few percent everywhere. Fixed it so short copies keep the byte loop and only long copies (periodic/run data) use the doubling, with the cutoff at length 33. So normal data goes back to the old fast path (actually a touch faster now, since near-end copies do a single memcpy instead of looping), and periodic/run data keeps the big speedup. Numbers on my M4, vs main, both decoders:
I don't have an EPYC to test on though. Could you re-run your numbers there? Should be sorted since it was the same root cause, but would be good to confirm. |
richarddd
force-pushed
the
faster-overlapping-copy
branch
2 times, most recently
from
June 22, 2026 05:20
96ac6e7 to
4daf8cf
Compare
Owner
|
Mixed bag on AMD EPYC 7R32 safe decode unsafe decode |
… run data) Self-referential (offset < match_length) match copies were done byte-by-byte. Long ones (run/periodic data) now grow by doubling the valid prefix with memcpy: O(log n) copies instead of n, ~10-40x faster. This is spliced into the `duplicate` general-match path only, so the hot <=18 fast loop is untouched and general-data decode is unchanged (measured neutral to +4% on AMD EPYC 7R32 / Zen 2 and Apple M4). The doubling helper is cold/inline(never) and takes the output pointer by value (returning the advance) rather than by `&mut`. Passing a `&mut` to a non-inlined function forces the pointer onto the stack across the whole decode loop, which on its own cost ~10% on general data on Zen 2. Unsafe decoder only. The safe decoder is left as-is: it is more codegen-sensitive and regressed when touched, and its offset==1 runs already use a fast fill. Adds tests/overlap_miri.rs and a CI miri job for the unsafe pointer code. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
richarddd
force-pushed
the
faster-overlapping-copy
branch
from
June 23, 2026 12:40
4daf8cf to
7245f12
Compare
Author
|
Tracked it down, and it was real. The slowdown wasn't the algorithm, it was codegen: the doubling code was landing in the hot decode loop, and in one variant a The fix: the doubling now only sits on the long-match / near-end path (the I re-ran on an EPYC 7R32 (your chip) with the binggan block_decompress bench. vs main:
@PSeitz Would you mind re-running your numbers to confirm on your side? |
This was referenced Oct 13, 2026
Open
nat-openai
pushed a commit
to nat-openai/datafusion-comet
that referenced
this pull request
Oct 14, 2026
* perf: store shuffle offsets as lengths with zstd Arrow's offsets buffers are running sums, which compress poorly. With zstd, the shuffle block writer now rewrites every offsets buffer of the record-batch body as lengths split into byte planes before compressing, and flags the block in its codec tag. The reader rebuilds the offsets before Arrow decodes the body. Shuffle files with string, binary, array and map columns get 9% to 40% smaller, and zstd gets no slower. LZ4 and snappy blocks are unchanged: lz4_flex decodes the long runs that lengths compress into slowly (PSeitz/lz4_flex#227). spark.comet.shuffle.compression.offsetsAsLengths.enabled (default true) turns it off. * refactor: decide offsets as lengths once per shuffle block writer ShuffleBlockWriter::try_new_with_offsets_as_lengths replaces the builder. The writer now decides when it is built: it checks once, on an empty batch, that arrow-ipc writes the buffers the mapped roles describe, keeps plain offsets if not, and sets the codec-tag flag in its header. That removes the per-block flag plumbing through every codec arm and the header rewrite. The writer and reader share one buffer-range helper and one rewrite loop. The reader rebuilds offsets before freezing the body. The two Scala tests share a cometShuffleMetric helper. * docs: say where shuffle offsets are stored as lengths The tuning guide and the config description now say that native shuffle and JVM shuffle blocks without dictionary-encoded columns store offsets as lengths with zstd, and that Celeborn shuffle blocks do not. native_shuffle.md describes the codec-tag flag, and the native and JVM shuffle config tables list spark.comet.shuffle.compression.offsetsAsLengths.enabled. * docs: list offsets_as_lengths.rs in the native shuffle guide * test: cover nulls, empty values, binary and struct columns and spilling in the offsets-as-lengths tests
This branch has not been deployed
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.
Long self-referential (overlapping / run) match copies in the unsafe decoder now grow by doubling the valid prefix with memcpy instead of copying byte-by-byte (10-40x on run/periodic data), spliced into the long-match path only and passed by value so the hot fast loop's codegen is unchanged and general-data decode stays flat on both x86 (Zen 2) and ARM. Validated under miri and against the C decoder.