Bill commented on a change in pull request #6343:
URL: https://github.com/apache/geode/pull/6343#discussion_r617956355
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/tcp/Connection.java
##########
@@ -3228,28 +3221,23 @@ private void setThreadName(int dominoNumber) {
+ " local port=" + socket.getLocalPort() + " remote port=" +
socket.getPort());
}
- private void compactOrResizeBuffer(int messageLength) {
- final int oldBufferSize = inputBuffer.capacity();
- int allocSize = messageLength + MSG_HEADER_BYTES;
- if (oldBufferSize < allocSize) {
- // need a bigger buffer
- logger.info("Allocating larger network read buffer, new size is {} old
size was {}.",
- allocSize, oldBufferSize);
- ByteBuffer oldBuffer = inputBuffer;
- inputBuffer = getBufferPool().acquireDirectReceiveBuffer(allocSize);
-
- if (oldBuffer != null) {
- int oldByteCount = oldBuffer.remaining();
- inputBuffer.put(oldBuffer);
- inputBuffer.position(oldByteCount);
- getBufferPool().releaseReceiveBuffer(oldBuffer);
- }
- } else {
- if (inputBuffer.position() != 0) {
- inputBuffer.compact();
+ private void compactOrResizeBuffer(int messageLength) throws IOException {
+ try (final ByteBufferSharing inputSharing = inputBufferVendor.open()) {
Review comment:
In order to avoid the try-with-resources block here, I could pass in the
`ByteBufferSharing` from the caller. I'm reluctant to do that though because it
expands the scope of that try-with-resources block in the caller. It's clear
enough while we're reviewing the PR right now, but I've been avoiding passing
those `AutoCloseable`s around. The protocol works, but only if everybody
follows the rules. Passing them around kinda breaks (or at least bends) a rule.
This compact method is called once per buffer. Since the caller holds the
lock, there will be no contention on the lock in the `open()` call here. There
is possible contention incrementing the reference count (contending with former
lock holders decrementing the count). The extra close call just decrements an
atomic.
From that description, do you think it would be better to pass in the
`ByteBufferSharing` or do you think it would be reasonable to let this merge
and we let performance benchmarks tell us if it's hurting us? Or alternately,
do you think I should run benchmarks on this branch ahead of merging?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
