Repository: netty/netty
Issue: Incorrect allocations value for PoolArena (tiny / small / normal)
Labels: defect

I found some PoolArena allocations value was incorrect, more specifically is:

    long numTinyAllocations();
    long numSmallAllocations();
    long numNormalAllocations();

and

    long numNormalDeallocations();

When I alloc Pooled ByteBuf & and release all these Bufs,

### Expected behavior
 
PoolArena's Metric SHOULD meet the following conditions:
```
           numTinyAllocations() == numTinyDeallocations()
     &&   numSmallAllocations() == numSmallDeallocations()
     &&   numNormalAllocations() == numNormalDeallocations()
```
### Actual behavior

BUT now result:
```
           numTinyAllocations() < numTinyDeallocations()
     &&   numSmallAllocations() < numSmallDeallocations()
     &&   numNormalAllocations() > numNormalDeallocations()
```
It seems some tiny & small allocations increase normal's counter,  I export PoolArenaMetric as MBean by [code](https://github.com/isdom/jocean-http/blob/9d8d528a2a3ee6bf2ca12f04054523dec94d3507/jocean-http/src/main/java/org/jocean/http/util/NettyStats.java) and show MBean by Web using zkoss, see below:

<img width="905" alt="2017-01-25 11 23 34" src="https://cloud.githubusercontent.com/assets/2010691/22338996/a21ee320-e423-11e6-8575-0fee497ae401.png">

In this case, 

`numAllocations(2404) == numDeallocations(2404)`

BUT  
```
   (numTinyDeallocations - numTinyAllocations)       //  == 17
+ (numSmallDeallocations - numSmallAllocations)       //  == 9
```
equals

` (numNormalAllocations - numNormalDeallocations)       // == 26`

### Steps to reproduce

I start test code with VM flag (disable thread local cache): 
```
-XX:MaxDirectMemorySize=96M
-Dio.netty.allocator.tinyCacheSize=0 
-Dio.netty.allocator.smallCacheSize=0 
-Dio.netty.allocator.normalCacheSize=0 
-Dio.netty.allocator.type=pooled
```
then alloc some ByteBuf and release Bufs.

### Minimal yet complete reproducer code (or URL to code)

TestCase to reproduce: https://github.com/isdom/jocean-http/blob/6bc6cfa9ae1e71395a4dd52355b1b0a8365bb530/jocean-http/src/test/java/org/jocean/netty/buffer/PooledByteBufAllocatorTestCase.java

and make sure run this test with VM flag: -XX:MaxDirectMemorySize=96M

It fail at:

```
assertEquals(metric.numTinyDeallocations(), metric.numTinyAllocations());
assertEquals(metric.numSmallDeallocations(), metric.numSmallAllocations());
assertEquals(metric.numNormalDeallocations(), metric.numNormalAllocations());
```
output : 
```
java.lang.AssertionError: expected:<1> but was:<0>
	at org.junit.Assert.fail(Assert.java:88)
```

If this is confirmed as a issue，I can open a PR to fix it.

### Netty version

netty-all-4.1.7.Final

### JVM version (e.g. `java -version`)

java version "1.8.0_102"
Java(TM) SE Runtime Environment (build 1.8.0_102-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.102-b14, mixed mode)

### OS version (e.g. `uname -a`)

Linux xxx 3.10.0-327.22.2.el7.x86_64 #1 SMP Thu Jun 23 17:05:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux


Issue comments:
@isdom this is because of the caches... when you deallocate the buffers they will first end up in ThreadLocal caches. Only after these removed from the caches they are really deallocated. You can disable the caches to see it.
@normanmaurer  but I have already disable ThreadLocal caches by set JVM flag: 
```
-Dio.netty.allocator.tinyCacheSize=0 
-Dio.netty.allocator.smallCacheSize=0 
-Dio.netty.allocator.normalCacheSize=0 
```
and Take a look at the following [test code](https://github.com/isdom/jocean-http/blob/88e15e8a3cecab118da550eaa991a9322035f99d/jocean-http/src/test/java/org/jocean/netty/buffer/PoolArenaTestCase.java):

```
    public final void testPoolArenaAllocationCounter() {
        // create PooledByteBufAllocator instance with ThreadLocal Cache disabled
        final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
                true,   //boolean preferDirect, 
                0,      //int nHeapArena, 
                1,      //int nDirectArena, 
                8192,   //int pageSize, 
                11,     //int maxOrder,
                0,      //int tinyCacheSize, 
                0,      //int smallCacheSize, 
                0,      //int normalCacheSize,
                true    //boolean useCacheForAllThreads
                );
        
        // alloc tiny buf
        final ByteBuf b1 = allocator.directBuffer(24);
        
        // alloc small buf
        final ByteBuf b2 = allocator.directBuffer(800);
        
        // alloc normal buf
        final ByteBuf b3 = allocator.directBuffer(8192 * 2);
        
       // make sure alloc buf success 
        assertNotNull(b1);
        assertNotNull(b2);
        assertNotNull(b3);
        
        // then release buf to deallocated buffers memory, and bcs threadlocal cache has been disabled
        // allocations counter value must equals deallocations counter value
        assertTrue(b1.release());
        assertTrue(b2.release());
        assertTrue(b3.release());
        
        assertTrue(allocator.directArenas().size() >= 1);
        
        final PoolArenaMetric metric = allocator.directArenas().get(0);
        
        // 1 tiny allocation + 1 small allocation + 1 normal allocation
        assertEquals(3, metric.numDeallocations());    // assertion PASS
        assertEquals(3, metric.numAllocations());        // assertion PASS
        
        // BUT following assertion fail
        assertEquals(metric.numTinyDeallocations(), metric.numTinyAllocations());
        assertEquals(metric.numSmallDeallocations(), metric.numSmallAllocations());
        assertEquals(metric.numNormalDeallocations(), metric.numNormalAllocations());
    }
```

I think PoolArena try to allocate tiny or small buffer, when PoolSubpage is empty( [head.next == head](https://github.com/netty/netty/blob/5986c229c463f064e7b787a4547c0689d2302a03/buffer/src/main/java/io/netty/buffer/PoolArena.java#L199) ), tiny or small buffer initialized via function [allocateNormal(buf, reqCapacity, normCapacity) ](https://github.com/netty/netty/blob/5986c229c463f064e7b787a4547c0689d2302a03/buffer/src/main/java/io/netty/buffer/PoolArena.java#L228), but **allocateNormal** always increase normal's counter at [code1](https://github.com/netty/netty/blob/5986c229c463f064e7b787a4547c0689d2302a03/buffer/src/main/java/io/netty/buffer/PoolArena.java#L232) and [code2](https://github.com/netty/netty/blob/5986c229c463f064e7b787a4547c0689d2302a03/buffer/src/main/java/io/netty/buffer/PoolArena.java#L239), these two increment action could be replaced by following code:

```
private void updateAllocationCounter(final int normCapacity) {
        if (isTiny(normCapacity)) {
            allocationsTiny.increment();
        } else if (isTinyOrSmall(normCapacity)) {
            allocationsSmall.increment();
        } else {
            ++allocationsNormal;
        }
}
```

Please check PoolArena's allocation increment code (tiny / small / normal ) again, thanks @normanmaurer 

@isdom doh! You are right. Want to do a PR or should I take care ?
@normanmaurer OK，Let me try... 
@isdom ok cool... Would be nice if just would calculate if its tiny/small/normal allocation one time though and not need to do multiple times 👍 
Fixed by https://github.com/netty/netty/pull/6288

Pull request review comments:
@isdom I would prefer if we not need to acquire the `synchronized(this)` two times when we increment for normal allocations. Can you please refactor to ensure we not do ?
OK
@normanmaurer Could you give me some advice for my commit: [acquire the synchronized(this) once when we increment for normal allocations](https://github.com/netty/netty/pull/6288/commits/e0b79fe7e7e70ccaf5c3178be773c0a8a552a1fe)?

Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/PoolArena.java b/buffer/src/main/java/io/netty/buffer/PoolArena.java
index 0b87a96f8df..2faeb09be1f 100644
--- a/buffer/src/main/java/io/netty/buffer/PoolArena.java
+++ b/buffer/src/main/java/io/netty/buffer/PoolArena.java
@@ -201,16 +201,14 @@ private void allocate(PoolThreadCache cache, PooledByteBuf<T> buf, final int req
                     long handle = s.allocate();
                     assert handle >= 0;
                     s.chunk.initBufWithSubpage(buf, handle, reqCapacity);
-
-                    if (tiny) {
-                        allocationsTiny.increment();
-                    } else {
-                        allocationsSmall.increment();
-                    }
+                    incTinySmallNormalAllocation(normCapacity);
                     return;
                 }
             }
-            allocateNormal(buf, reqCapacity, normCapacity);
+            synchronized (this) {
+                allocateNormal(buf, reqCapacity, normCapacity);
+                incTinySmallNormalAllocation(normCapacity);
+            }
             return;
         }
         if (normCapacity <= chunkSize) {
@@ -218,30 +216,41 @@ private void allocate(PoolThreadCache cache, PooledByteBuf<T> buf, final int req
                 // was able to allocate out of the cache so move on
                 return;
             }
-            allocateNormal(buf, reqCapacity, normCapacity);
+            synchronized (this) {
+                allocateNormal(buf, reqCapacity, normCapacity);
+                incTinySmallNormalAllocation(normCapacity);
+            }
         } else {
             // Huge allocations are never served via the cache so just call allocateHuge
             allocateHuge(buf, reqCapacity);
         }
     }
 
-    private synchronized void allocateNormal(PooledByteBuf<T> buf, int reqCapacity, int normCapacity) {
+    private void allocateNormal(PooledByteBuf<T> buf, int reqCapacity, int normCapacity) {
         if (q050.allocate(buf, reqCapacity, normCapacity) || q025.allocate(buf, reqCapacity, normCapacity) ||
             q000.allocate(buf, reqCapacity, normCapacity) || qInit.allocate(buf, reqCapacity, normCapacity) ||
             q075.allocate(buf, reqCapacity, normCapacity)) {
-            ++allocationsNormal;
             return;
         }
 
         // Add a new chunk.
         PoolChunk<T> c = newChunk(pageSize, maxOrder, pageShifts, chunkSize);
         long handle = c.allocate(normCapacity);
-        ++allocationsNormal;
         assert handle > 0;
         c.initBuf(buf, handle, reqCapacity);
         qInit.add(c);
     }
 
+    private void incTinySmallNormalAllocation(final int normCapacity) {
+        if (isTiny(normCapacity)) {
+            allocationsTiny.increment();
+        } else if (isTinyOrSmall(normCapacity)) {
+            allocationsSmall.increment();
+        } else {
+            ++allocationsNormal;
+        }
+    }
+
     private void allocateHuge(PooledByteBuf<T> buf, int reqCapacity) {
         PoolChunk<T> chunk = newUnpooledChunk(reqCapacity);
         activeBytesHuge.add(chunk.chunkSize());


Test patch:
diff --git a/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
index f5acd2b4f22..f27c41ca3ef 100644
--- a/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
+++ b/buffer/src/test/java/io/netty/buffer/PoolArenaTest.java
@@ -32,4 +32,49 @@ public void testNormalizeCapacity() throws Exception {
             Assert.assertEquals(expectedResult[i], arena.normalizeCapacity(reqCapacities[i]));
         }
     }
+
+    @Test
+    public final void testAllocationCounter() {
+        final PooledByteBufAllocator allocator = new PooledByteBufAllocator(
+                true,   // preferDirect
+                0,      // nHeapArena
+                1,      // nDirectArena
+                8192,   // pageSize
+                11,     // maxOrder
+                0,      // tinyCacheSize
+                0,      // smallCacheSize
+                0,      // normalCacheSize
+                true    // useCacheForAllThreads
+                );
+
+        // create tiny buffer
+        final ByteBuf b1 = allocator.directBuffer(24);
+        // create small buffer
+        final ByteBuf b2 = allocator.directBuffer(800);
+        // create normal buffer
+        final ByteBuf b3 = allocator.directBuffer(8192 * 2);
+
+        Assert.assertNotNull(b1);
+        Assert.assertNotNull(b2);
+        Assert.assertNotNull(b3);
+
+        // then release buffer to deallocated memory while threadlocal cache has been disabled
+        // allocations counter value must equals deallocations counter value
+        Assert.assertTrue(b1.release());
+        Assert.assertTrue(b2.release());
+        Assert.assertTrue(b3.release());
+
+        Assert.assertTrue(allocator.directArenas().size() >= 1);
+        final PoolArenaMetric metric = allocator.directArenas().get(0);
+
+        Assert.assertEquals(3, metric.numDeallocations());
+        Assert.assertEquals(3, metric.numAllocations());
+
+        Assert.assertEquals(1, metric.numTinyDeallocations());
+        Assert.assertEquals(1, metric.numTinyAllocations());
+        Assert.assertEquals(1, metric.numSmallDeallocations());
+        Assert.assertEquals(1, metric.numSmallAllocations());
+        Assert.assertEquals(1, metric.numNormalDeallocations());
+        Assert.assertEquals(1, metric.numNormalAllocations());
+    }
 }