From db5e729853389d5cc5e4c6bf7620efbeea8a95e1 Mon Sep 17 00:00:00 2001 From: ian Date: Mon, 14 Apr 2014 16:57:24 -0400 Subject: [PATCH] Fix error that causes (up to) double memory usage Motivation: PoolArena's 'normalizeCapacity' function was micro-optimized some time ago to remove a while loop. However, there was a change of behavior in the function as a result. Capacities passed into it that are already powers of 2 (and >= 512) are doubled in size. So if I ask for a buffer with a capacity of 1024, I will get back one that actually uses 2048 bytes (stored in maxLength). Aligning to powers of two for book keeping ease is reasonable, and if someone tries to expand a buffer, you might as well use some of the previously wasted space. However, since this distinction between 'easily expanded' and 'costly to expand' space is not supported at all by the APIs, I cannot imagine this change to doubling is desirable or intentional. This is especially costly when using composite buffers. They frequently allocate components with a capacity that is a power of 2, and they never attempt to expand components themselves. The end result is that heavy use of pool-backed composite buffers wastes almost half of the memory pool (the smaller / initial components are <512 and so are not affected by the off-by-one bug). Modifications: Although I find it difficult to believe that such an optimization is really helpful, I left it in and fixed the off-by-one issue by decrementing the value at the start. I also added a simple test to both attempt to verify that the decrement fixes the issue without introducing any other change, and to make it easy for a reviewer to test the existing behavior. PoolArena does not seem to have much testing or testability support though so the test is kind of a hack and will break for unrelated changes. I suggest either removing it or factoring out the single non-static portion of normalizeCapacity so that the fragile dummy PoolArena is not required. Result: Pooled allocators will allocate less resources to the highly inefficient and undocumented buffer section between length and maxLength. Composite buffers of non-trivial size that are backed by pooled allocators will use about half as much memory. --- .../main/java/io/netty/buffer/PoolArena.java | 1 + .../java/io/netty/buffer/PoolArenaTest.java | 35 +++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 buffer/src/test/java/io/netty/buffer/PoolArenaTest.java diff --git a/buffer/src/main/java/io/netty/buffer/PoolArena.java b/buffer/src/main/java/io/netty/buffer/PoolArena.java index 8b24c3711b..5f7998e662 100644 --- a/buffer/src/main/java/io/netty/buffer/PoolArena.java +++ b/buffer/src/main/java/io/netty/buffer/PoolArena.java @@ -233,6 +233,7 @@ abstract class PoolArena { // Doubled int normalizedCapacity = reqCapacity; + normalizedCapacity --; normalizedCapacity |= normalizedCapacity >>> 1; normalizedCapacity |= normalizedCapacity >>> 2; normalizedCapacity |= normalizedCapacity >>> 4; diff --git a/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java new file mode 100644 index 0000000000..f5acd2b4f2 --- /dev/null +++ b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java @@ -0,0 +1,35 @@ +/* + * Copyright 2012 The Netty Project + * + * The Netty Project licenses this file to you under the Apache License, + * version 2.0 (the "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +package io.netty.buffer; + +import org.junit.Assert; +import org.junit.Test; + +import java.nio.ByteBuffer; + +public class PoolArenaTest { + + @Test + public void testNormalizeCapacity() throws Exception { + PoolArena arena = new PoolArena.DirectArena(null, 0, 0, 9, 999999); + int[] reqCapacities = {0, 15, 510, 1024, 1023, 1025}; + int[] expectedResult = {0, 16, 512, 1024, 1024, 2048}; + for (int i = 0; i < reqCapacities.length; i ++) { + Assert.assertEquals(expectedResult[i], arena.normalizeCapacity(reqCapacities[i])); + } + } +}