GH-3484 Eliminate per-page heap allocation for CRC32 checksums when using direct ByteBufferAllocator#3485
Open
arouel wants to merge 1 commit intoapache:masterfrom
Open
GH-3484 Eliminate per-page heap allocation for CRC32 checksums when using direct ByteBufferAllocator#3485arouel wants to merge 1 commit intoapache:masterfrom
ByteBufferAllocator#3485arouel wants to merge 1 commit intoapache:masterfrom
Conversation
…when using direct `ByteBufferAllocator` Why this is safe - CRC32.update(ByteBuffer) exists since Java 9, processes bytes from position to limit, advancing position. - toByteBuffer(releaser) returns either a slice() of the internal buffer (independent position) or a freshly allocated copy. Either way, the original BytesInput is unaffected for the subsequent buf.collect() call, because ByteBufferBytesInput.writeInto() uses buffer.duplicate(). - When the allocator is direct, toByteBuffer(releaser) returns the direct buffer directly -- zero heap copy. When the allocator is heap-based, behavior is functionally equivalent to the old toByteArray() path. - The releaser field already exists on ColumnChunkPageWriter (line 124) and manages buffer lifecycle.
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
1 participant
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.
Rationale for this change
ColumnChunkPageWriter.writePage()andwritePageV2()callBytesInput.toByteArray()to feed compressed page data intoCRC32.update(byte[]). When the writer uses a directByteBufferAllocator, this forces a full heap copy of every compressed page solely for checksumming. Since page write checksums are enabled by default (DEFAULT_PAGE_WRITE_CHECKSUM_ENABLED = true), this allocation occurs on every page write and negates part of the benefit of using a direct allocator.What changes are included in this PR?
Replace
crc.update(x.toByteArray())withcrc.update(x.toByteBuffer(releaser))writePage()(V1): 1 call forcompressedByteswritePageV2(): 3 calls forrepetitionLevels,definitionLevels, andcompressedDataCRC32.update(ByteBuffer)has been available since Java 9 and operates directly on the buffer's memory without copying. Thereleaserfield already exists onColumnChunkPageWriterand provides the allocator-awareByteBufferlifecycle management needed. When the allocator is heap-based,toByteBuffer(releaser)returns a heap buffer and behavior is functionally equivalent to the previous code.Are these changes tested?
The existing
TestColumnChunkPageWriteStorecovers both heap and direct allocator paths (testandtestWithDirectBuffers) and exerciseswritePageV2with checksums enabled by default.TestDataPageChecksumscovers V1 and V2 pages with checksums on/off.Are there any user-facing changes?
No. This is an internal optimization. CRC32 checksums are computed identically; only the intermediate memory representation changes.
Closes #3484