-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LUCENE-10657: CopyBytes now saves one memory copy on ByteBuffersDataOutput #1034
Conversation
…eDoc -> bufferdDocs.copyBytes(DataInput input) Abstract method copyBytes need to copy from input to a buffer and then write into ByteBuffersDataOutput, i think there is unnecessary, we can override it, copy directly from input into output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic makes sense to me, can you add unit tests?
lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
Outdated
Show resolved
Hide resolved
1. Add UnitTest For ByteBuffersDataOutput.copyBytes
Thanks for your review, in commits 926dd i added check for the direct memory, and i added unit tests for different allocator in copyBytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, can you add a CHANGES entry that copyBytes now saves one memory copy on ByteBuffersDataOutput?
@jpountz Thanks for your review and suggestions, I have been updated the CHANGES entry |
lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/store/ByteBuffersDataOutput.java
Outdated
Show resolved
Hide resolved
2. Fixed currentBlock array type check
…utput (#1034) Abstract method copyBytes need to copy from input to a buffer and then write into ByteBuffersDataOutput, i think there is unnecessary, we can override it, copy directly from input into output
This is derived from LUCENE-10627 at #987
Jira: https://issues.apache.org/jira/browse/LUCENE-10657
The abstract method
copyBytes
in DataOutput have to copy from input to a copyBuffer and then write into ByteBuffersDataOutput.blocks, i think there is unnecessary, we can override it, copy directly from input into output.with override this method, we can:
Lucene90CompressingStoredFieldsWriter#copyOneDoc
->bufferdDocs.copyBytes(DataInput input)
Lucene90CompoundFormat.writeCompoundFile
->data.copyBytes
when input isBufferedChecksumIndexinput
and output isByteBuffersDataOutput
IndexWriter#copySegmentAsIs
->CopyFrom -> copyBytes