[GitHub] [geode] Bill commented on a change in pull request #6343: GEODE-9141: fixed via ByteBufferSharing on Connection input buffer

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org