-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Eliminate temporary buffer allocations during bson serialization #1628
base: main
Are you sure you want to change the base?
Conversation
@Edarke please open a Jira issue in the JAVA project for this PR, as per our contribution guide. Thanks! |
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.
Looks good! I just have a few minor comments.
throw new IllegalArgumentException(format("position must be >= 0 but was %d", absolutePosition)); | ||
} | ||
if (absolutePosition > buffer.position() - bytesToWrite) { | ||
throw new IllegalArgumentException(format("position must be <= %d but was %d", buffer.position() - 1, absolutePosition)); |
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.
throw new IllegalArgumentException(format("position must be <= %d but was %d", buffer.position() - 1, absolutePosition)); | |
throw new IllegalArgumentException(format("position must be <= %d but was %d", buffer.position() - bytesToWrite, absolutePosition)); |
throw new IllegalArgumentException(); | ||
} | ||
position = newPosition; | ||
((Buffer) buffer).position(newPosition); |
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.
((Buffer) buffer).position(newPosition); | |
// The cast is required for compatibility with JDK 9+ where ByteBuffer's position method is inherited from Buffer. | |
((Buffer) buffer).position(newPosition); |
} | ||
|
||
@Override | ||
public void writeByte(final int value) { | ||
ensureOpen(); | ||
|
||
ensure(1); | ||
buffer[position++] = (byte) (0xFF & value); | ||
buffer.put((byte) (0xFF & value)); | ||
} | ||
|
||
@Override | ||
protected void write(final int absolutePosition, final int value) { |
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.
Now that the writeInt32(final int value)
method has been updated to use ByteBuffer
's bulk operation putInt(...)
, this method is no longer covered by tests. Previously, it was indirectly tested through writeInt32
. Could you add a dedicated test case for this?
bsonOutput.position == 4 | ||
bsonOutput.size == 4 | ||
} | ||
|
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.
def 'toByteArray creates a copy'() { | |
given: | |
def bsonOutput = new BasicOutputBuffer(10) | |
bsonOutput.writeBytes([1, 2, 3, 4] as byte[]) | |
when: | |
def first = bsonOutput.toByteArray() | |
def second = bsonOutput.toByteArray() | |
then: | |
first !== second | |
first == [1, 2, 3, 4] as byte[] | |
second == [1, 2, 3, 4] as byte[] | |
} |
Fixes: JAVA-5786
Outputbuffer.toByteArray()
to avoid extraneous array copyBsonBinaryWriter
to callOutputBuffers.writeObjectId
so implementations can opt to useputToByteBuffer
rather than allocating a temporary byte[]Motivation
Atlas Search uses the Java driver to communicate with the Mongodb Server. We serialize up to ~300k documents per batch, primarily consisting of ObjectID and numbers. For these large batches, serialization has measurable overhead. This PR proposes some simple optimizations for
BasicOutputBuffer
without altering the API.JMH Benchmarks