Fix #5562 Improve HTTP Field cache allocation (#5565)

* Fix #5562 Improve HTTP Field cache allocation

Fix #5562 by initially putting cacheable fields into a inexpensive arraylist.
Only create the Trie (with space and complexity costs) if a second request is received.

* Fixed NPE

* Feedback from review

Create `HttpHeader.isPseudo()`` method
improved clarity with `createFieldCacheIfNeeded()``

* Feedback from review

Only defer Trie creation to first cacheable field, not until next request.

* Updates from review

* Update from review

 + more javadoc
 + empty set return

jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java

@@ -128,13 +128,13 @@ public enum HttpHeader
     /**
      * HTTP2 Fields.
      */
-    C_METHOD(":method"),
+    C_METHOD(":method", true),
-    C_SCHEME(":scheme"),
+    C_SCHEME(":scheme", true),
-    C_AUTHORITY(":authority"),
+    C_AUTHORITY(":authority", true),
-    C_PATH(":path"),
+    C_PATH(":path", true),
-    C_STATUS(":status"),
+    C_STATUS(":status", true),
 
-    UNKNOWN("::UNKNOWN::");
+    UNKNOWN("::UNKNOWN::", true);
 
     public static final Trie<HttpHeader> CACHE = new ArrayTrie<>(630);
 
@@ -153,14 +153,21 @@ public enum HttpHeader
     private final byte[] _bytes;
     private final byte[] _bytesColonSpace;
     private final ByteBuffer _buffer;
+    private final boolean _pseudo;
 
     HttpHeader(String s)
+    {
+        this(s, false);
+    }
+
+    HttpHeader(String s, boolean pseudo)
     {
         _string = s;
         _lowerCase = StringUtil.asciiToLowerCase(s);
         _bytes = StringUtil.getBytes(s);
         _bytesColonSpace = StringUtil.getBytes(s + ": ");
         _buffer = ByteBuffer.wrap(_bytes);
+        _pseudo = pseudo;
     }
 
     public String lowerCaseName()
@@ -188,6 +195,14 @@ public enum HttpHeader
         return _string.equalsIgnoreCase(s);
     }
 
+    /**
+     * @return True if the header is a HTTP2 Pseudo header (eg ':path')
+     */
+    public boolean isPseudo()
+    {
+        return _pseudo;
+    }
+
     public String asString()
     {
         return _string;

jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java

@@ -103,6 +103,7 @@ public class HttpParser
      * </ul>
      */
     public static final Trie<HttpField> CACHE = new ArrayTrie<>(2048);
+    private static final Trie<HttpField> NO_CACHE = Trie.empty(true);
 
     // States
     public enum FieldState
@@ -152,6 +153,7 @@ public class HttpParser
     private final int _maxHeaderBytes;
     private final HttpCompliance _compliance;
     private final EnumSet<HttpComplianceSection> _compliances;
+    private final Utf8StringBuilder _uri = new Utf8StringBuilder(INITIAL_URI_LENGTH);
     private HttpField _field;
     private HttpHeader _header;
     private String _headerString;
@@ -167,7 +169,6 @@ public class HttpParser
     private HttpMethod _method;
     private String _methodString;
     private HttpVersion _version;
-    private Utf8StringBuilder _uri = new Utf8StringBuilder(INITIAL_URI_LENGTH); // Tune?
     private EndOfContent _endOfContent;
     private boolean _hasContentLength;
     private boolean _hasTransferEncoding;
@@ -231,7 +232,7 @@ public class HttpParser
         // Add headers with null values so HttpParser can avoid looking up name again for unknown values
         for (HttpHeader h : HttpHeader.values())
         {
-            if (!CACHE.put(new HttpField(h, (String)null)))
+            if (!h.isPseudo() && !CACHE.put(new HttpField(h, (String)null)))
                 throw new IllegalStateException("CACHE FULL");
         }
     }
@@ -884,11 +885,6 @@ public class HttpParser
                             }
                             checkVersion();
 
-                            // Should we try to cache header fields?
-                            int headerCache = _handler.getHeaderCacheSize();
-                            if (_fieldCache == null && _version.getVersion() >= HttpVersion.HTTP_1_1.getVersion() && headerCache > 0)
-                                _fieldCache = new ArrayTernaryTrie<>(headerCache);
-
                             setState(State.HEADER);
 
                             _requestHandler.startRequest(_methodString, _uri.toString(), _version);
@@ -961,7 +957,7 @@ public class HttpParser
             // Handle known headers
             if (_header != null)
             {
-                boolean addToConnectionTrie = false;
+                boolean addToFieldCache = false;
                 switch (_header)
                 {
                     case CONTENT_LENGTH:
@@ -1034,14 +1030,16 @@ public class HttpParser
                             _field = new HostPortHttpField(_header,
                                 _compliances.contains(HttpComplianceSection.FIELD_NAME_CASE_INSENSITIVE) ? _header.asString() : _headerString,
                                 _valueString);
-                            addToConnectionTrie = _fieldCache != null;
+                            addToFieldCache = true;
                         }
                         break;
 
                     case CONNECTION:
                         // Don't cache headers if not persistent
-                        if (HttpHeaderValue.CLOSE.is(_valueString) || new QuotedCSV(_valueString).getValues().stream().anyMatch(HttpHeaderValue.CLOSE::is))
+                        if (_field == null)
-                            _fieldCache = null;
+                            _field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
+                        if (_handler.getHeaderCacheSize() > 0 && _field.contains(HttpHeaderValue.CLOSE.asString()))
+                            _fieldCache = NO_CACHE;
                         break;
 
                     case AUTHORIZATION:
@@ -1052,18 +1050,29 @@ public class HttpParser
                     case COOKIE:
                     case CACHE_CONTROL:
                     case USER_AGENT:
-                        addToConnectionTrie = _fieldCache != null && _field == null;
+                        addToFieldCache = _field == null;
                         break;
 
                     default:
                         break;
                 }
 
-                if (addToConnectionTrie && !_fieldCache.isFull() && _header != null && _valueString != null)
+                // Cache field?
+                if (addToFieldCache && _header != null && _valueString != null)
                 {
-                    if (_field == null)
+                    if (_fieldCache == null)
-                        _field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
+                    {
-                    _fieldCache.put(_field);
+                        _fieldCache = (_handler.getHeaderCacheSize() > 0 && (_version != null && _version == HttpVersion.HTTP_1_1))
+                            ? new ArrayTernaryTrie<>(_handler.getHeaderCacheSize())
+                            : NO_CACHE;
+                    }
+
+                    if (!_fieldCache.isFull())
+                    {
+                        if (_field == null)
+                            _field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString);
+                        _fieldCache.put(_field);
+                    }
                 }
             }
             _handler.parsedHeader(_field != null ? _field : new HttpField(_header, _headerString, _valueString));

jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java

@@ -198,7 +198,7 @@ public class HttpConfiguration implements Dumpable
         return _responseHeaderSize;
     }
 
-    @ManagedAttribute("The maximum allowed size in bytes for an HTTP header field cache")
+    @ManagedAttribute("The maximum allowed size in Trie nodes for an HTTP header field cache")
     public int getHeaderCacheSize()
     {
         return _headerCacheSize;
@@ -423,7 +423,8 @@ public class HttpConfiguration implements Dumpable
     }
 
     /**
-     * @param headerCacheSize The size in bytes of the header field cache.
+     * @param headerCacheSize The size of the header field cache, in terms of unique characters branches
+     * in the lookup {@link Trie} and associated data structures.
      */
     public void setHeaderCacheSize(int headerCacheSize)
     {

jetty-util/src/main/java/org/eclipse/jetty/util/Trie.java

@@ -19,6 +19,7 @@
 package org.eclipse.jetty.util;
 
 import java.nio.ByteBuffer;
+import java.util.Collections;
 import java.util.Set;
 
 /**
@@ -131,4 +132,99 @@ public interface Trie<V>
     boolean isCaseInsensitive();
 
     void clear();
+
+    static <T> Trie<T> empty(final boolean caseInsensitive)
+    {
+        return new Trie<T>()
+        {
+            @Override
+            public boolean put(String s, Object o)
+            {
+                return false;
+            }
+
+            @Override
+            public boolean put(Object o)
+            {
+                return false;
+            }
+
+            @Override
+            public T remove(String s)
+            {
+                return null;
+            }
+
+            @Override
+            public T get(String s)
+            {
+                return null;
+            }
+
+            @Override
+            public T get(String s, int offset, int len)
+            {
+                return null;
+            }
+
+            @Override
+            public T get(ByteBuffer b)
+            {
+                return null;
+            }
+
+            @Override
+            public T get(ByteBuffer b, int offset, int len)
+            {
+                return null;
+            }
+
+            @Override
+            public T getBest(String s)
+            {
+                return null;
+            }
+
+            @Override
+            public T getBest(String s, int offset, int len)
+            {
+                return null;
+            }
+
+            @Override
+            public T getBest(byte[] b, int offset, int len)
+            {
+                return null;
+            }
+
+            @Override
+            public T getBest(ByteBuffer b, int offset, int len)
+            {
+                return null;
+            }
+
+            @Override
+            public Set<String> keySet()
+            {
+                return Collections.emptySet();
+            }
+
+            @Override
+            public boolean isFull()
+            {
+                return true;
+            }
+
+            @Override
+            public boolean isCaseInsensitive()
+            {
+                return caseInsensitive;
+            }
+
+            @Override
+            public void clear()
+            {
+            }
+        };
+    }
 }