-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Enhance buffer handling to reduce direct list access. #1675
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
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM. Let's add a secondary.
driver-core/src/main/com/mongodb/internal/connection/ByteBufferBsonOutput.java
Outdated
Show resolved
Hide resolved
…rBsonOutput.java Co-authored-by: Maxim Katcharov <[email protected]>
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.
LGTM shouldn't we release the cached buffer when calling close? (the underlying Native C/C++ buffer could be reference counted and needs to drop to 0 to actually free the memory)
if (curByteBuffer.hasRemaining()) { | ||
return curByteBuffer; | ||
if (currentByteBuffer == null) { | ||
currentByteBuffer = getByteBufferAtIndex(curBufferIndex); |
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.
I guess this is fine if it's guaranteed to be thread-confined?
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.
Yes, ByteBufferBsonOutput
is not thread-safe and should only be updated in one thread.
@@ -259,6 +263,10 @@ public void truncateToPosition(final int newPosition) { | |||
|
|||
bufferList.get(bufferPositionPair.bufferIndex).position(bufferPositionPair.position); | |||
|
|||
if (bufferPositionPair.bufferIndex + 1 < bufferList.size()) { |
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.
I think size()
iterates all the elements to count, can't we cache it as well?
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.
You're right that some List
implementations may compute size()
by iterating. In our case, though, it's an ArrayList
, which maintains a cached size
field that's updated on mutation. Since this method isn't on a hot path, i think we should currently avoid adding extra caching logic that could increase complexity.
In this case, That said, this is a good point. It made me realize it would be clearer to explicitly nullify the |
Summary
The PR improves buffer handling by introducing a
currentByteBuffer
field to cache the currentByteBuf
, reducing direct list access which is costly because of bounds checking. Additionally, cachingcurrentByteBuffer
helps with JIT (Just-In-Time) inlining based on compilation profiling. By making the buffer access more predictable and, the JIT compiler can better inline thegetCurrentByteBuffer
method, leading to improved runtime performance.Perf analyzer results: Link
JAVA-5846