Issue #6974 - Bring additional ByteBufferPool fixes back to 9.4 (#7203)

- allow empty buffers to be pooled - improve testMaxMemory for Array & Mapped ByteBufferPools - Remove flakey WebSocketBufferPoolTest
jetty · Dec 5, 2021 · d34beb9 · d34beb9
1 parent 1adb4a4
commit d34beb9
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 280 deletions.
diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ArrayByteBufferPool.java
@@ -109,13 +109,13 @@ public ArrayByteBufferPool(int minCapacity, int factor, int maxCapacity, int max
         _minCapacity = minCapacity;
 
         // Initialize all buckets in constructor and never modify the array again.
-        int length = bucketFor(maxCapacity);
+        int length = bucketFor(maxCapacity) + 1;
         _direct = new ByteBufferPool.Bucket[length];
         _indirect = new ByteBufferPool.Bucket[length];
         for (int i = 0; i < length; i++)
         {
-            _direct[i] = newBucket(i + 1, true);
-            _indirect[i] = newBucket(i + 1, false);
+            _direct[i] = newBucket(i, true);
+            _indirect[i] = newBucket(i, false);
         }
     }
 
@@ -214,11 +214,11 @@ private ByteBufferPool.Bucket bucketFor(int capacity, boolean direct)
     {
         if (capacity < _minCapacity)
             return null;
-        int index = bucketFor(capacity) - 1;
-        if (index >= _direct.length)
+        int bucket = bucketFor(capacity);
+        if (bucket >= _direct.length)
             return null;
         Bucket[] buckets = bucketsFor(direct);
-        return buckets[index];
+        return buckets[bucket];
     }
 
     @ManagedAttribute("The number of pooled direct ByteBuffers")

diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java b/jetty-io/src/main/java/org/eclipse/jetty/io/MappedByteBufferPool.java
@@ -181,7 +181,7 @@ protected void releaseMemory(boolean direct)
         }
         if (index >= 0)
         {
-            Bucket bucket = buckets.get(index);
+            Bucket bucket = buckets.remove(index);
             // Null guard in case this.clear() is called concurrently.
             if (bucket != null)
                 bucket.clear();

diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java
@@ -27,10 +27,9 @@
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
 import static org.hamcrest.Matchers.is;
-import static org.hamcrest.Matchers.lessThan;
-import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertSame;
@@ -202,7 +201,7 @@ public void testMaxQueue()
     public void testMaxMemory()
     {
         int factor = 1024;
-        int maxMemory = 11 * 1024;
+        int maxMemory = 11 * factor;
         ArrayByteBufferPool bufferPool = new ArrayByteBufferPool(-1, factor, -1, -1, -1, maxMemory);
         Bucket[] buckets = bufferPool.bucketsFor(true);
 
@@ -212,26 +211,38 @@ public void testMaxMemory()
         {
             int capacity = factor * i;
             ByteBuffer buffer = bufferPool.acquire(capacity, true);
+            assertThat(buffer.capacity(), equalTo(capacity));
             bufferPool.release(buffer);
         }
 
+        // Check state of buckets.
+        assertThat(bufferPool.getMemory(true), equalTo(10L * factor));
+        assertThat(buckets[1].size(), equalTo(1));
+        assertThat(buckets[2].size(), equalTo(1));
+        assertThat(buckets[3].size(), equalTo(1));
+        assertThat(buckets[4].size(), equalTo(1));
+
         // Create and release a buffer to exceed the max memory.
-        ByteBuffer buffer = bufferPool.newByteBuffer(2 * factor, true);
+        int capacity = 2 * factor;
+        ByteBuffer buffer = bufferPool.newByteBuffer(capacity, true);
+        assertThat(buffer.capacity(), equalTo(capacity));
         bufferPool.release(buffer);
 
         // Now the oldest buffer should be gone and we have: 1+2x2+3=8
-        long memory = bufferPool.getMemory(true);
-        assertThat(memory, lessThan((long)maxMemory));
-        assertTrue(buckets[3].isEmpty());
+        assertThat(bufferPool.getMemory(true), equalTo(8L * factor));
+        assertThat(buckets[1].size(), equalTo(1));
+        assertThat(buckets[2].size(), equalTo(2));
+        assertThat(buckets[3].size(), equalTo(1));
 
         // Create and release a large buffer.
         // Max memory is exceeded and buckets 3 and 1 are cleared.
         // We will have 2x2+7=11.
-        buffer = bufferPool.newByteBuffer(7 * factor, true);
+        capacity = 7 * factor;
+        buffer = bufferPool.newByteBuffer(capacity, true);
         bufferPool.release(buffer);
-        memory = bufferPool.getMemory(true);
-        assertThat(memory, lessThanOrEqualTo((long)maxMemory));
-        assertTrue(buckets[0].isEmpty());
-        assertTrue(buckets[2].isEmpty());
+
+        assertThat(bufferPool.getMemory(true), equalTo(11L * factor));
+        assertThat(buckets[2].size(), equalTo(2));
+        assertThat(buckets[7].size(), equalTo(1));
     }
 }
diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/MappedByteBufferPoolTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/MappedByteBufferPoolTest.java
@@ -28,9 +28,8 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThanOrEqualTo;
-import static org.hamcrest.Matchers.lessThan;
-import static org.hamcrest.Matchers.lessThanOrEqualTo;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -143,7 +142,7 @@ public void testMaxQueue()
     public void testMaxMemory()
     {
         int factor = 1024;
-        int maxMemory = 11 * 1024;
+        int maxMemory = 11 * factor;
         MappedByteBufferPool bufferPool = new MappedByteBufferPool(factor, -1, null, -1, maxMemory);
         ConcurrentMap<Integer, Bucket> buckets = bufferPool.bucketsFor(true);
 
@@ -153,26 +152,39 @@ public void testMaxMemory()
         {
             int capacity = factor * i;
             ByteBuffer buffer = bufferPool.acquire(capacity, true);
+            assertThat(buffer.capacity(), equalTo(capacity));
             bufferPool.release(buffer);
         }
 
+        // Check state of buckets.
+        assertThat(bufferPool.getMemory(true), equalTo(10L * factor));
+        assertThat(buckets.get(1).size(), equalTo(1));
+        assertThat(buckets.get(2).size(), equalTo(1));
+        assertThat(buckets.get(3).size(), equalTo(1));
+        assertThat(buckets.get(4).size(), equalTo(1));
+
         // Create and release a buffer to exceed the max memory.
-        ByteBuffer buffer = bufferPool.newByteBuffer(2 * factor, true);
+        int capacity = 2 * factor;
+        ByteBuffer buffer = bufferPool.newByteBuffer(capacity, true);
+        assertThat(buffer.capacity(), equalTo(capacity));
         bufferPool.release(buffer);
 
         // Now the oldest buffer should be gone and we have: 1+2x2+3=8
-        long memory = bufferPool.getMemory(true);
-        assertThat(memory, lessThan((long)maxMemory));
-        assertTrue(buckets.get(4).isEmpty());
+        assertThat(bufferPool.getMemory(true), equalTo(8L * factor));
+        assertThat(buckets.get(1).size(), equalTo(1));
+        assertThat(buckets.get(2).size(), equalTo(2));
+        assertThat(buckets.get(3).size(), equalTo(1));
 
         // Create and release a large buffer.
         // Max memory is exceeded and buckets 3 and 1 are cleared.
         // We will have 2x2+7=11.
-        buffer = bufferPool.newByteBuffer(7 * factor, true);
+        capacity = 7 * factor;
+        buffer = bufferPool.newByteBuffer(capacity, true);
+        assertThat(buffer.capacity(), equalTo(capacity));
         bufferPool.release(buffer);
-        memory = bufferPool.getMemory(true);
-        assertThat(memory, lessThanOrEqualTo((long)maxMemory));
-        assertTrue(buckets.get(1).isEmpty());
-        assertTrue(buckets.get(3).isEmpty());
+
+        assertThat(bufferPool.getMemory(true), equalTo(11L * factor));
+        assertThat(buckets.get(2).size(), equalTo(2));
+        assertThat(buckets.get(7).size(), equalTo(1));
     }
 }
diff --git a/jetty-server/src/main/config/etc/jetty-bytebufferpool-logarithmic.xml b/jetty-server/src/main/config/etc/jetty-bytebufferpool-logarithmic.xml
@@ -1,4 +1,5 @@
-<?xml version="1.0"?><!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">
+<?xml version="1.0"?>
+<!DOCTYPE Configure PUBLIC "-//Jetty//Configure//EN" "http://www.eclipse.org/jetty/configure_9_3.dtd">
 <Configure>
   <New id="byteBufferPool" class="org.eclipse.jetty.io.LogarithmicArrayByteBufferPool">
     <Arg type="int"><Property name="jetty.byteBufferPool.minCapacity" default="0"/></Arg>