Do not use ArrayQueue for HPACK #751

jetty-http2/http2-hpack/src/main/java/org/eclipse/jetty/http2/hpack/HpackContext.java

@@ -28,7 +28,6 @@ import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpScheme;
-import org.eclipse.jetty.util.ArrayQueue;
 import org.eclipse.jetty.util.ArrayTernaryTrie;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.Trie;
@@ -117,6 +116,7 @@ public class HpackContext
     private static final Trie<StaticEntry> __staticNameMap = new ArrayTernaryTrie<>(true,512);
     private static final StaticEntry[] __staticTableByHeader = new StaticEntry[HttpHeader.UNKNOWN.ordinal()];
     private static final StaticEntry[] __staticTable=new StaticEntry[STATIC_TABLE.length];
+    private static final int STATIC_SIZE = STATIC_TABLE.length-1;
     static
     {
         Set<String> added = new HashSet<>();
@@ -196,7 +196,7 @@ public class HpackContext
     {
         _maxDynamicTableSizeInBytes=maxDynamicTableSize;
         int guesstimateEntries = 10+maxDynamicTableSize/(32+10+10);
-        _dynamicTable=new DynamicTable(guesstimateEntries,guesstimateEntries+10);
+        _dynamicTable=new DynamicTable(guesstimateEntries);
         if (LOG.isDebugEnabled())
             LOG.debug(String.format("HdrTbl[%x] created max=%d",hashCode(),maxDynamicTableSize));
     }
@@ -206,9 +206,7 @@ public class HpackContext
         if (LOG.isDebugEnabled())
             LOG.debug(String.format("HdrTbl[%x] resized max=%d->%d",hashCode(),_maxDynamicTableSizeInBytes,newMaxDynamicTableSize));
         _maxDynamicTableSizeInBytes=newMaxDynamicTableSize;
-        int guesstimateEntries = 10+newMaxDynamicTableSize/(32+10+10);
+        _dynamicTable.evict();
-        evict();
-        _dynamicTable.resizeUnsafe(guesstimateEntries);
     }
 
     public Entry get(HttpField field)
@@ -229,14 +227,10 @@ public class HpackContext
 
     public Entry get(int index)
     {
-        if (index<__staticTable.length)
+        if (index<=STATIC_SIZE)
             return __staticTable[index];
 
-        int d=_dynamicTable.size()-index+__staticTable.length-1;
+        return _dynamicTable.get(index);
-
-        if (d>=0)
-            return _dynamicTable.getUnsafe(d);
-        return null;
     }
 
     public Entry get(HttpHeader header)
@@ -254,8 +248,7 @@ public class HpackContext
 
     public Entry add(HttpField field)
     {
-        int slot=_dynamicTable.getNextSlotUnsafe();
+        Entry entry=new Entry(field);
-        Entry entry=new Entry(slot,field);
         int size = entry.getSize();
         if (size>_maxDynamicTableSizeInBytes)
         {
@@ -264,13 +257,13 @@ public class HpackContext
             return null;
         }
         _dynamicTableSizeInBytes+=size;
-        _dynamicTable.addUnsafe(entry);
+        _dynamicTable.add(entry);
         _fieldMap.put(field,entry);
         _nameMap.put(StringUtil.asciiToLowerCase(field.getName()),entry);
 
         if (LOG.isDebugEnabled())
             LOG.debug(String.format("HdrTbl[%x] added %s",hashCode(),entry));
-        evict();
+        _dynamicTable.evict();
         return entry;
     }
 
@@ -305,7 +298,7 @@ public class HpackContext
         if (entry.isStatic())
             return entry._slot;
 
-        return _dynamicTable.index(entry)+__staticTable.length-1;
+        return _dynamicTable.index(entry);
     }
 
     public static int staticIndex(HttpHeader header)
@@ -315,26 +308,9 @@ public class HpackContext
         Entry entry=__staticNameMap.get(header.asString());
         if (entry==null)
             return 0;
-        return entry.getSlot();
+        return entry._slot;
     }
 
-    private void evict()
-    {
-        while (_dynamicTableSizeInBytes>_maxDynamicTableSizeInBytes)
-        {
-            Entry entry = _dynamicTable.pollUnsafe();
-            if (LOG.isDebugEnabled())
-                LOG.debug(String.format("HdrTbl[%x] evict %s",hashCode(),entry));
-            _dynamicTableSizeInBytes-=entry.getSize();
-            entry._slot=-1;
-            _fieldMap.remove(entry.getHttpField());
-            String lc=StringUtil.asciiToLowerCase(entry.getHttpField().getName());
-            if (entry==_nameMap.get(lc))
-                _nameMap.remove(lc);
-        }
-        if (LOG.isDebugEnabled())
-            LOG.debug(String.format("HdrTbl[%x] entries=%d, size=%d, max=%d",hashCode(),_dynamicTable.size(),_dynamicTableSizeInBytes,_maxDynamicTableSizeInBytes));
-    }
 
     @Override
     public String toString()
@@ -342,69 +318,94 @@ public class HpackContext
         return String.format("HpackContext@%x{entries=%d,size=%d,max=%d}",hashCode(),_dynamicTable.size(),_dynamicTableSizeInBytes,_maxDynamicTableSizeInBytes);
     }
 
-    private class DynamicTable extends ArrayQueue<HpackContext.Entry>
+    private class DynamicTable 
     {
-        private DynamicTable(int initCapacity, int growBy)
+        Entry[] _entries;
+        int _size;
+        int _offset;
+        int _growby;
+        
+        private DynamicTable(int initCapacity)
         {
-            super(initCapacity,growBy);
+            _entries=new Entry[initCapacity];
+            _growby=initCapacity;
         }
 
-        /**
+        public void add(Entry entry)
-         * @see org.eclipse.jetty.util.ArrayQueue#growUnsafe()
-         */
-        @Override
-        protected void resizeUnsafe(int newCapacity)
         {
-            // Relay on super.growUnsafe to pack all entries 0 to _nextSlot
+            if (_size==_entries.length)
-            super.resizeUnsafe(newCapacity);
+            {
-            for (int s=0;s<_nextSlot;s++)
+                Entry[] entries = new Entry[_entries.length+_growby];
-                ((Entry)_elements[s])._slot=s;
+                for (int i=0;i<_size;i++)
+                {
+                    int slot = (_offset+i)%_entries.length;
+                    entries[i]=_entries[slot];
+                    entries[i]._slot=i;
+                }
+                _entries=entries;
+                _offset=0;
+            }
+            int slot=(_size++ + _offset)%_entries.length;
+            _entries[slot]=entry;
+            entry._slot=slot;
         }
 
-        /**
+        public int index(Entry entry)
-         * @see org.eclipse.jetty.util.ArrayQueue#enqueue(java.lang.Object)
-         */
-        @Override
-        public boolean enqueue(Entry e)
         {
-            return super.enqueue(e);
+            return STATIC_SIZE + _size-(entry._slot-_offset+_entries.length)%_entries.length;
+        }
+        
+        public Entry get(int index)
+        {
+            int d = index-STATIC_SIZE-1;
+            if (d<0 || d>=_size)
+                return null;
+            int slot = (_offset+_size-d-1)%_entries.length;
+            return _entries[slot];
         }
 
-        /**
+        public int size()
-         * @see org.eclipse.jetty.util.ArrayQueue#dequeue()
-         */
-        @Override
-        public Entry dequeue()
         {
-            return super.dequeue();
+            return _size;
         }
 
-        private int index(Entry entry)
+        private void evict()
         {
-            return entry._slot>=_nextE?_size-entry._slot+_nextE:_nextSlot-entry._slot;
+            while (_dynamicTableSizeInBytes>_maxDynamicTableSizeInBytes)
+            {
+                Entry entry = _entries[_offset];
+                _entries[_offset]=null;
+                _offset = (_offset+1)%_entries.length;
+                _size--;
+                if (LOG.isDebugEnabled())
+                    LOG.debug(String.format("HdrTbl[%x] evict %s",hashCode(),entry));
+                _dynamicTableSizeInBytes-=entry.getSize();
+                entry._slot=-1;
+                _fieldMap.remove(entry.getHttpField());
+                String lc=StringUtil.asciiToLowerCase(entry.getHttpField().getName());
+                if (entry==_nameMap.get(lc))
+                    _nameMap.remove(lc);
+
+            }
+            if (LOG.isDebugEnabled())
+                LOG.debug(String.format("HdrTbl[%x] entries=%d, size=%d, max=%d",hashCode(),_dynamicTable.size(),_dynamicTableSizeInBytes,_maxDynamicTableSizeInBytes));
         }
+
     }
 
     public static class Entry
     {
         final HttpField _field;
-        int _slot;
+        int _slot; // The index within it's array
 
         Entry()
         {
-            _slot=0;
+            _slot=-1;
             _field=null;
         }
 
-        Entry(int index,String name, String value)
+        Entry(HttpField field)
         {
-            _slot=index;
-            _field=new HttpField(name,value);
-        }
-
-        Entry(int slot, HttpField field)
-        {
-            _slot=slot;
             _field=field;
         }
 
@@ -429,11 +430,6 @@ public class HpackContext
             return null;
         }
 
-        public int getSlot()
-        {
-            return _slot;
-        }
-
         public String toString()
         {
             return String.format("{%s,%d,%s,%x}",isStatic()?"S":"D",_slot,_field,hashCode());
@@ -447,7 +443,8 @@ public class HpackContext
 
         StaticEntry(int index,HttpField field)
         {
-            super(index,field);
+            super(field);
+            _slot=index;
             String value = field.getValue();
             if (value!=null && value.length()>0)
             {

jetty-http2/http2-hpack/src/test/java/org/eclipse/jetty/http2/hpack/HpackContextTest.java

@@ -216,12 +216,12 @@ public class HpackContextTest
         assertEquals(62,ctx.index(entry[0]));
         assertEquals(entry[0],ctx.get(62));
         
-        // and statics have moved up 0
+        // and statics still OK
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
+        assertEquals(":authority",ctx.get(1).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
+        assertEquals(3,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
+        assertEquals(methodPost,ctx.get(3).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
+        assertEquals("www-authenticate",ctx.get(61).getHttpField().getName());
-        assertEquals(null,ctx.get(62+0+ctx.size()));
+        assertEquals(null,ctx.get(62+ctx.size()));
         
 
         // Add 4 more entries
@@ -238,12 +238,12 @@ public class HpackContextTest
             index--;
         }
 
-        // and statics have moved up 0
+        // and statics still OK
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
+        assertEquals(":authority",ctx.get(1).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
+        assertEquals(3,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
+        assertEquals(methodPost,ctx.get(3).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
+        assertEquals("www-authenticate",ctx.get(61).getHttpField().getName());
-        assertEquals(null,ctx.get(62+0+ctx.size()));
+        assertEquals(null,ctx.get(62+ctx.size()));
         
         // add 1 more entry and this should cause an eviction!
         entry[5]=ctx.add(field[5]);
@@ -260,12 +260,12 @@ public class HpackContextTest
         assertNull(ctx.get(field[0]));
         assertEquals(0,ctx.index(entry[0]));
 
-        // and statics have moved up 0
+        // and statics still OK
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
+        assertEquals(":authority",ctx.get(1).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
+        assertEquals(3,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
+        assertEquals(methodPost,ctx.get(3).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
+        assertEquals("www-authenticate",ctx.get(61).getHttpField().getName());
-        assertEquals(null,ctx.get(62+0+ctx.size()));
+        assertEquals(null,ctx.get(62+ctx.size()));
         
         // Add 4 more entries
         for (int i=6;i<=9;i++)  
@@ -306,7 +306,6 @@ public class HpackContextTest
     {
         // Only enough space for 5 entries
         HpackContext ctx = new HpackContext(38*5);
-        HttpField methodPost = new HttpField(":method","POST");
         
         HttpField[] field = 
         {
@@ -339,15 +338,6 @@ public class HpackContextTest
             index--;
         }
 
-        // and statics have moved up 0
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
-        assertEquals(null,ctx.get(62+ctx.size()));
-        
-        
-        
         // resize so that only 2 entries may be held
         ctx.resize(38*2);
         assertEquals(2,ctx.size());
@@ -360,13 +350,6 @@ public class HpackContextTest
             assertEquals(entry[i],ctx.get(index));
             index--;
         }
-
-        // and statics have moved up 0
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
-        assertEquals(null,ctx.get(62+ctx.size()));
         
         // resize so that 6.5 entries may be held
         ctx.resize(38*6+19);
@@ -380,13 +363,6 @@ public class HpackContextTest
             assertEquals(entry[i],ctx.get(index));
             index--;
         }
-
-        // and statics have moved up 0
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
-        assertEquals(null,ctx.get(62+ctx.size()));
         
 
         // Add 5 entries
@@ -403,13 +379,33 @@ public class HpackContextTest
             assertEquals(entry[i],ctx.get(index));
             index--;
         }
+        
 
-        // and statics have moved up 0
+        // resize so that only 100 entries may be held
-        assertEquals(":authority",ctx.get(1+0).getHttpField().getName());
+        ctx.resize(38*100);
-        assertEquals(3+0,ctx.index(ctx.get(methodPost)));
+        assertEquals(6,ctx.size());
-        assertEquals(methodPost,ctx.get(3+0).getHttpField());
+        // check indexes
-        assertEquals("www-authenticate",ctx.get(61+0).getHttpField().getName());
+        index=67;
-        assertEquals(null,ctx.get(62+ctx.size()));
+        for (int i=4;i<=9;i++)  
+        {
+            assertEquals(index,ctx.index(entry[i]));
+            assertEquals(entry[i],ctx.get(index));
+            index--;
+        }
+        
+        // add 50 fields
+        for (int i=0;i<50;i++)
+            ctx.add(new HttpField("n"+i,"v"+i));
+
+        // check indexes
+        index=67+50;
+        for (int i=4;i<=9;i++)  
+        {
+            assertEquals(index,ctx.index(entry[i]));
+            assertEquals(entry[i],ctx.get(index));
+            index--;
+        }
+        
         
     }