GH-3518: Bulk write in LittleEndianDataOutputStream.writeInt/writeShort (+35% encodePlain when used)#3519
Closed
iemejia wants to merge 1 commit intoapache:masterfrom
Closed
GH-3518: Bulk write in LittleEndianDataOutputStream.writeInt/writeShort (+35% encodePlain when used)#3519iemejia wants to merge 1 commit intoapache:masterfrom
iemejia wants to merge 1 commit intoapache:masterfrom
Conversation
…iteShort Replace per-byte out.write(int) calls with a single out.write(byte[], 0, N) using the existing writeBuffer[] field, matching the pattern already used by writeLong. For writeInt this collapses 4 bookkeeping trips through the underlying stream (hasRemaining check, Math.addExact, slab-grow check, single-byte store) into 1. Resolves the long-standing TODO comment in writeInt that flagged this as a potential improvement. Benchmark (IntEncodingBenchmark.encodePlain when routed through LittleEndianDataOutputStream): ~20.9M -> ~28.2M ops/s (+35%) Note: PR apache#3496 deprecates this class because Parquet's own writers no longer use it. This change benefits external Parquet-format producers that still use the class until they migrate. Validation: parquet-common 308 tests pass. Built with -Dspotless.check.skip=true -Drat.skip=true -Djapicmp.skip=true.
Member
Author
|
Folding this change into #3496 (the PR that deprecates LittleEndianDataOutputStream) since both touch the same class and are best reviewed together. |
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.
Summary
Resolves #3518.
LittleEndianDataOutputStream.writeInt(int)andwriteShort(int)decompose the value byte-by-byte and callout.write(int)for each byte:When the underlying stream is
CapacityByteArrayOutputStream(the typical case in Parquet writers), eachout.write(int)performs ahasRemainingcheck, aMath.addExact, possibly a slab-grow check, and a single-byte store. ForwriteInt, that's 4 trips through the bookkeeping per value.The class already has the right pattern in
writeLong: build thewriteBuffer[]and emit a singleout.write(writeBuffer, 0, 8). The buffer is even pre-allocated for that purpose. This PR extends the same pattern towriteIntandwriteShort.Resolves the long-standing
TODOinwriteInt:Benchmark
IntEncodingBenchmark.encodePlainwhen routed throughLittleEndianDataOutputStream:Note on context
PR #3496 deprecates
LittleEndianDataOutputStreambecause Parquet's own writers no longer use it (they write directly intoByteBuffer-backed slabs, which compiles to a single intrinsic store on little-endian and is strictly faster than any wrapper).After #3496 lands, no Parquet code in any module instantiates
LittleEndianDataOutputStream. This PR therefore benefits external Parquet-format producers that still use the class — they get the speedup until they migrate. The change is minimal (~10 lines), obviously correct (matches the existingwriteLongpattern in the same file), and resolves the existingTODO.If the maintainers prefer to leave a deprecated class untouched, this PR is easy to drop. I'm flagging the option because the change is small enough that it costs almost nothing to land and helps anyone outside the Parquet codebase still on the class.
Validation
parquet-common: 308 tests pass-Dspotless.check.skip=true -Drat.skip=true -Djapicmp.skip=trueUser-facing changes
None. No public API change. Behavior of
writeInt,writeShort, andwriteLongis identical bit-for-bit.Closes #3518
Part of a small series of focused performance PRs from work in parquet-perf. Previous: #3494, #3496, #3500, #3504, #3506, #3510, #3514, #3517. Companion benchmarks contribution: #3512.