From f4c32e788ad534fb8dce3183cf6ce62c59c8ca33 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 12 Nov 2020 17:05:32 +0100 Subject: [PATCH] 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 --- .../org/eclipse/jetty/http/HttpHeader.java | 27 ++++-- .../org/eclipse/jetty/http/HttpParser.java | 41 ++++---- .../jetty/server/HttpConfiguration.java | 5 +- .../java/org/eclipse/jetty/util/Trie.java | 96 +++++++++++++++++++ 4 files changed, 145 insertions(+), 24 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java index 7a3d3654051e..11fedafa87d9 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpHeader.java @@ -128,13 +128,13 @@ public enum HttpHeader /** * HTTP2 Fields. */ - C_METHOD(":method"), - C_SCHEME(":scheme"), - C_AUTHORITY(":authority"), - C_PATH(":path"), - C_STATUS(":status"), + C_METHOD(":method", true), + C_SCHEME(":scheme", true), + C_AUTHORITY(":authority", true), + C_PATH(":path", true), + C_STATUS(":status", true), - UNKNOWN("::UNKNOWN::"); + UNKNOWN("::UNKNOWN::", true); public static final Trie 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 boolean is(String s) 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; diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 9cad979abd44..8f9eab2f3569 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -103,6 +103,7 @@ public class HttpParser * */ public static final Trie CACHE = new ArrayTrie<>(2048); + private static final Trie NO_CACHE = Trie.empty(true); // States public enum FieldState @@ -152,6 +153,7 @@ public enum State private final int _maxHeaderBytes; private final HttpCompliance _compliance; private final EnumSet _compliances; + private final Utf8StringBuilder _uri = new Utf8StringBuilder(INITIAL_URI_LENGTH); private HttpField _field; private HttpHeader _header; private String _headerString; @@ -167,7 +169,6 @@ public enum State 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 enum State // 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 @@ else if (n == HttpTokens.LINE_FEED) } 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 @@ private void parsedHeader() // Handle known headers if (_header != null) { - boolean addToConnectionTrie = false; + boolean addToFieldCache = false; switch (_header) { case CONTENT_LENGTH: @@ -1034,14 +1030,16 @@ else if (_endOfContent == EndOfContent.CHUNKED_CONTENT) _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)) - _fieldCache = null; + if (_field == 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 @@ else if (_endOfContent == EndOfContent.CHUNKED_CONTENT) 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) - _field = new HttpField(_header, caseInsensitiveHeader(_headerString, _header.asString()), _valueString); - _fieldCache.put(_field); + if (_fieldCache == null) + { + _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)); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java index e0841f256e75..883b103c28ec 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java @@ -198,7 +198,7 @@ public int getResponseHeaderSize() 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 void setResponseHeaderSize(int responseHeaderSize) } /** - * @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) { diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/Trie.java b/jetty-util/src/main/java/org/eclipse/jetty/util/Trie.java index 31536d9f67ad..258e6fd7f57d 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/Trie.java +++ b/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 boolean isCaseInsensitive(); void clear(); + + static Trie empty(final boolean caseInsensitive) + { + return new Trie() + { + @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 keySet() + { + return Collections.emptySet(); + } + + @Override + public boolean isFull() + { + return true; + } + + @Override + public boolean isCaseInsensitive() + { + return caseInsensitive; + } + + @Override + public void clear() + { + } + }; + } }