From 61bcfd034a464e7873ea9c38b9e47ae3ad4e09cb Mon Sep 17 00:00:00 2001 From: ahadadi Date: Thu, 18 Aug 2022 11:36:38 +0300 Subject: [PATCH 1/8] Optimize HpackStaticTable by using a perfect hash function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation: HpackStaticTable performance can be improved by using a perfect hash function. Modifications: Use 2 tables, one for mapping header name -> index and one for mapping header name + value -> index. Choose the tables and the hash function in such a way that each entry maps to a single hash bucket. Results: Benchmark (optimized) Mode Cnt Score Error Units HpackStaticTableBenchmark.lookupHttp false avgt 10 15.998 ± 1.646 ns/op HpackStaticTableBenchmark.lookupHttp true avgt 10 10.457 ± 0.274 ns/op HpackStaticTableBenchmark.lookupHttps false avgt 10 20.942 ± 1.365 ns/op HpackStaticTableBenchmark.lookupHttps true avgt 10 10.618 ± 0.138 ns/op HpackStaticTableBenchmark.lookupNameOnlyMatch false avgt 10 13.710 ± 0.273 ns/op HpackStaticTableBenchmark.lookupNameOnlyMatch true avgt 10 3.156 ± 0.052 ns/op HpackStaticTableBenchmark.lookupNoNameMatch false avgt 10 3.528 ± 0.047 ns/op HpackStaticTableBenchmark.lookupNoNameMatch true avgt 10 3.145 ± 0.031 ns/op Caveats: This implementation couples HpackStaticTable implementation to the implementation of AsciiString.hashCode, relying on the values it returns for the static table headers to yield a perfect hash function. If AsciiString.hashCode implementation changes, HpackStaticTable implementation will also need to change. Moreover, if AsciiString.hashCode can return different values on different platforms (maybe due to endianness) or in general in different jvm instances, then it invalidates the approach taken here (or at least makes its implementation much more complex). --- .../handler/codec/http2/HpackStaticTable.java | 159 +++++++++++------- .../codec/http2/HpackStaticTableTest.java | 123 ++++++++++++++ .../http2/HpackStaticTableBenchmark.java | 19 +-- 3 files changed, 231 insertions(+), 70 deletions(-) create mode 100644 codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 389d8ef150f..702ce64f4e3 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -31,13 +31,13 @@ */ package io.netty.handler.codec.http2; -import io.netty.handler.codec.UnsupportedValueConverter; import io.netty.util.AsciiString; import java.util.Arrays; import java.util.List; import static io.netty.handler.codec.http2.HpackUtil.equalsVariableTime; +import static io.netty.util.internal.ObjectUtil.checkNotNull; final class HpackStaticTable { @@ -117,9 +117,53 @@ private static HpackHeaderField newHeaderField(String name, String value) { return new HpackHeaderField(AsciiString.cached(name), AsciiString.cached(value)); } - private static final CharSequenceMap STATIC_INDEX_BY_NAME = createMap(); + private static final int HEADER_NAMES_TABLE_SIZE = 1 << 9; - private static final int MAX_SAME_NAME_FIELD_INDEX = maxSameNameFieldIndex(); + // a bit shift chosen so that each header name will hash into a single bucket + private static final int HEADER_NAMES_TABLE_SHIFT = 18; + + // A table holding header names and their associated indexes. + private static final HeaderNameIndex[] HEADER_NAMES = new HeaderNameIndex[HEADER_NAMES_TABLE_SIZE]; + static { + // Iterate through the static table in reverse order to + // save the smallest index for a given name in the table. + for (int index = STATIC_TABLE.size(); index > 0; index--) { + HpackHeaderField entry = getEntry(index); + int bucket = headerNameBucket(entry.name); + HeaderNameIndex tableEntry = HEADER_NAMES[bucket]; + if (tableEntry != null && !equalsVariableTime(tableEntry.name, entry.name)) { + // Can happen if AsciiString.hashCode changes + throw new IllegalStateException("Hash bucket collision between " + + tableEntry.name + " and " + entry.name); + } + HEADER_NAMES[bucket] = new HeaderNameIndex(entry.name, index, entry.value.length() == 0); + } + } + + private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE = 1 << 6; + + // a bit shift chosen so that each header will hash into a single bucket + private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SHIFT = 6; + + // A table holding header names and values for non-empty values. + // This table is keyed by value which is possible since each non-empty value is unique. + private static final HeaderIndex[] HEADERS_WITH_NON_EMPTY_VALUES = + new HeaderIndex[HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE]; + static { + for (int index = STATIC_TABLE.size(); index > 0; index--) { + HpackHeaderField entry = getEntry(index); + if (entry.value.length() > 0) { + int bucket = headerBucket(entry.value); + HeaderIndex tableEntry = HEADERS_WITH_NON_EMPTY_VALUES[bucket]; + if (tableEntry != null) { + // Can happen if AsciiString.hashCode changes + throw new IllegalStateException("Hash bucket collision between " + + tableEntry.value + " and " + entry.value); + } + HEADERS_WITH_NON_EMPTY_VALUES[bucket] = new HeaderIndex(entry.name, entry.value, index); + } + } + } /** * The number of header fields in the static table. @@ -138,11 +182,9 @@ static HpackHeaderField getEntry(int index) { * -1 if the header field name is not in the static table. */ static int getIndex(CharSequence name) { - Integer index = STATIC_INDEX_BY_NAME.get(name); - if (index == null) { - return NOT_FOUND; - } - return index; + checkNotNull(name, "name"); + HeaderNameIndex entry = getEntry(name); + return entry == null ? NOT_FOUND : entry.index; } /** @@ -150,70 +192,69 @@ static int getIndex(CharSequence name) { * header field is not in the static table. */ static int getIndexInsensitive(CharSequence name, CharSequence value) { - int index = getIndex(name); - if (index == NOT_FOUND) { + checkNotNull(name, "name"); + if (value == null) { return NOT_FOUND; } - - // Compare values for the first name match - HpackHeaderField entry = getEntry(index); - if (equalsVariableTime(value, entry.value)) { - return index; - } - - // Note this assumes all entries for a given header field are sequential. - index++; - while (index <= MAX_SAME_NAME_FIELD_INDEX) { - entry = getEntry(index); - if (!equalsVariableTime(name, entry.name)) { - // As far as fields with the same name are placed in the table sequentially - // and INDEX_BY_NAME returns index of the fist position, - it's safe to - // exit immediately. + if (value.length() == 0) { + HeaderNameIndex entry = getEntry(name); + return entry == null || !entry.emptyValue ? NOT_FOUND : entry.index; + } else { + int bucket = headerBucket(value); + HeaderIndex header = HEADERS_WITH_NON_EMPTY_VALUES[bucket]; + if (header == null) { return NOT_FOUND; } - if (equalsVariableTime(value, entry.value)) { - return index; + if (equalsVariableTime(header.name, name) && equalsVariableTime(header.value, value)) { + return header.index; } - index++; + return NOT_FOUND; } + } - return NOT_FOUND; + private static HeaderNameIndex getEntry(CharSequence name) { + int bucket = headerNameBucket(name); + HeaderNameIndex entry = HEADER_NAMES[bucket]; + if (entry == null) { + return null; + } + return equalsVariableTime(entry.name, name) ? entry : null; } - // create a map CharSequenceMap header name to index value to allow quick lookup - private static CharSequenceMap createMap() { - int length = STATIC_TABLE.size(); - @SuppressWarnings("unchecked") - CharSequenceMap ret = new CharSequenceMap(true, - UnsupportedValueConverter.instance(), length); - // Iterate through the static table in reverse order to - // save the smallest index for a given name in the map. - for (int index = length; index > 0; index--) { - HpackHeaderField entry = getEntry(index); - CharSequence name = entry.name; - ret.set(name, index); + private static int headerNameBucket(CharSequence name) { + return bucket(name, HEADER_NAMES_TABLE_SHIFT, HEADER_NAMES_TABLE_SIZE - 1); + } + + private static int headerBucket(CharSequence value) { + return bucket(value, HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SHIFT, HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE - 1); + } + + private static int bucket(CharSequence s, int shift, int mask) { + return (AsciiString.hashCode(s) >> shift) & mask; + } + + private static final class HeaderNameIndex { + private final CharSequence name; + private final int index; + private final boolean emptyValue; + + private HeaderNameIndex(CharSequence name, int index, boolean emptyValue) { + this.name = name; + this.index = index; + this.emptyValue = emptyValue; } - return ret; } - /** - * Returns the last position in the array that contains multiple - * fields with the same name. Starting from this position, all - * names are unique. Similar to {@link #getIndexInsensitive(CharSequence, CharSequence)} method - * assumes all entries for a given header field are sequential - */ - private static int maxSameNameFieldIndex() { - final int length = STATIC_TABLE.size(); - HpackHeaderField cursor = getEntry(length); - for (int index = length - 1; index > 0; index--) { - HpackHeaderField entry = getEntry(index); - if (equalsVariableTime(entry.name, cursor.name)) { - return index + 1; - } else { - cursor = entry; - } + private static final class HeaderIndex { + private final CharSequence name; + private final CharSequence value; + private final int index; + + private HeaderIndex(CharSequence name, CharSequence value, int index) { + this.name = name; + this.value = value; + this.index = index; } - return length; } // singleton diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java new file mode 100644 index 00000000000..49c84342dda --- /dev/null +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java @@ -0,0 +1,123 @@ +/* + * Copyright 2022 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: + * + * https://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.handler.codec.http2; + +import io.netty.util.AsciiString; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +public class HpackStaticTableTest { + + @Test + public void testNullHeaderName() { + assertThrows(NullPointerException.class, new Executable() { + @Override + public void execute() { + HpackStaticTable.getIndex(null); + } + }); + } + + @Test + public void testEmptyHeaderName() { + assertEquals(-1, HpackStaticTable.getIndex("")); + } + + @Test + public void testNullHeaderNameEmptyValue() { + assertThrows(NullPointerException.class, new Executable() { + @Override + public void execute() { + HpackStaticTable.getIndexInsensitive(null, ""); + } + }); + } + + @Test + public void testEmptyHeaderNameNullValue() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive("", null)); + } + + @Test + public void testNullHeaderNameAndValue() { + assertThrows(NullPointerException.class, new Executable() { + @Override + public void execute() { + HpackStaticTable.getIndexInsensitive(null, null); + } + }); + } + + @Test + public void testMissingHeaderName() { + assertEquals(-1, HpackStaticTable.getIndex("missing")); + } + + @Test + public void testExistingHeaderName() { + assertEquals(6, HpackStaticTable.getIndex(":scheme")); + } + + @Test + public void testMissingHeaderNameAndValue() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive("missing", "value")); + } + + @Test + public void testMissingHeaderNameButValueExists() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive("missing", "https")); + } + + @Test + public void testExistingHeaderNameAndValueFirstMatch() { + assertEquals(6, HpackStaticTable.getIndexInsensitive(":scheme", "http")); + } + + @Test + public void testExistingHeaderNameAndValueSecondMatch() { + assertEquals(7, HpackStaticTable.getIndexInsensitive( + AsciiString.cached(":scheme"), AsciiString.cached("https"))); + } + + @Test + public void testLongHeaderValue() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "longer then longest static value")); + } + + @Test + public void testExistingHeaderNameAndEmptyValueMismatch() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "")); + } + + @Test + public void testExistingHeaderNameAndEmptyValueMatch() { + assertEquals(27, HpackStaticTable.getIndexInsensitive("content-language", "")); + } + + @Test + public void testExistingHeaderNameAndNullValue() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive("content-language", null)); + } + + @Test + public void testExistingHeaderNameButMissingValue() { + assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "missing")); + } + +} diff --git a/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java b/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java index e771cfcea88..2a75c7e1dd3 100644 --- a/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java +++ b/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java @@ -43,6 +43,9 @@ public class HpackStaticTableBenchmark extends AbstractMicrobenchmark { private static final CharSequence X_CONTENT_ENCODING = new AsciiString("x-content-encoding".getBytes(CharsetUtil.US_ASCII), false); private static final CharSequence X_GZIP = new AsciiString("x-gzip".getBytes(CharsetUtil.US_ASCII), false); + private static final CharSequence SCHEME = new AsciiString(":scheme".getBytes(CharsetUtil.US_ASCII), false); + private static final CharSequence HTTP = new AsciiString("http".getBytes(CharsetUtil.US_ASCII), false); + private static final CharSequence HTTPS = new AsciiString("https".getBytes(CharsetUtil.US_ASCII), false); private static final CharSequence STATUS = new AsciiString(":status".getBytes(CharsetUtil.US_ASCII), false); private static final CharSequence STATUS_200 = new AsciiString("200".getBytes(CharsetUtil.US_ASCII), false); private static final CharSequence STATUS_500 = new AsciiString("500".getBytes(CharsetUtil.US_ASCII), false); @@ -63,25 +66,19 @@ public int lookupNoNameMatch() { @Benchmark @BenchmarkMode(Mode.AverageTime) - public int lookupNameAndValueMatchFirst() { - return HpackStaticTable.getIndexInsensitive(STATUS, STATUS_200); + public int lookupHttp() { + return HpackStaticTable.getIndexInsensitive(SCHEME, HTTP); } @Benchmark @BenchmarkMode(Mode.AverageTime) - public int lookupNameAndValueMatchLast() { - return HpackStaticTable.getIndexInsensitive(STATUS, STATUS_500); + public int lookupHttps() { + return HpackStaticTable.getIndexInsensitive(SCHEME, HTTPS); } @Benchmark @BenchmarkMode(Mode.AverageTime) - public int lookupNameOnlyMatchBeginTable() { - return HpackStaticTable.getIndexInsensitive(AUTHORITY, AUTHORITY_NETTY); - } - - @Benchmark - @BenchmarkMode(Mode.AverageTime) - public int lookupNameOnlyMatchEndTable() { + public int lookupNameOnlyMatch() { return HpackStaticTable.getIndexInsensitive(USER_AGENT, USER_AGENT_CURL); } } From 54dbb47ee2c48c0eefd7f909da3b0821cec356ab Mon Sep 17 00:00:00 2001 From: ahadadi Date: Thu, 18 Aug 2022 12:25:38 +0300 Subject: [PATCH 2/8] Apply a bit shift according to the platform's endianness --- .../java/io/netty/handler/codec/http2/HpackStaticTable.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 702ce64f4e3..1b977766d2b 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -32,6 +32,7 @@ package io.netty.handler.codec.http2; import io.netty.util.AsciiString; +import io.netty.util.internal.PlatformDependent; import java.util.Arrays; import java.util.List; @@ -120,7 +121,7 @@ private static HpackHeaderField newHeaderField(String name, String value) { private static final int HEADER_NAMES_TABLE_SIZE = 1 << 9; // a bit shift chosen so that each header name will hash into a single bucket - private static final int HEADER_NAMES_TABLE_SHIFT = 18; + private static final int HEADER_NAMES_TABLE_SHIFT = PlatformDependent.BIG_ENDIAN_NATIVE_ORDER ? 22 : 18; // A table holding header names and their associated indexes. private static final HeaderNameIndex[] HEADER_NAMES = new HeaderNameIndex[HEADER_NAMES_TABLE_SIZE]; @@ -143,7 +144,8 @@ private static HpackHeaderField newHeaderField(String name, String value) { private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE = 1 << 6; // a bit shift chosen so that each header will hash into a single bucket - private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SHIFT = 6; + private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SHIFT = + PlatformDependent.BIG_ENDIAN_NATIVE_ORDER ? 0 : 6; // A table holding header names and values for non-empty values. // This table is keyed by value which is possible since each non-empty value is unique. From bcdf66d5e38b482adade3d2563daf88b140c1868 Mon Sep 17 00:00:00 2001 From: ahadadi Date: Thu, 18 Aug 2022 15:24:17 +0300 Subject: [PATCH 3/8] Revert some changes to the HpackStaticTableBenchmark in order to remain API compatible. --- .../http2/HpackStaticTableBenchmark.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java b/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java index 2a75c7e1dd3..4cc7b2fe96a 100644 --- a/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java +++ b/microbench/src/main/java/io/netty/handler/codec/http2/HpackStaticTableBenchmark.java @@ -64,6 +64,24 @@ public int lookupNoNameMatch() { return HpackStaticTable.getIndexInsensitive(X_CONTENT_ENCODING, X_GZIP); } + @Benchmark + @BenchmarkMode(Mode.AverageTime) + public int lookupNameAndValueMatchFirst() { + return HpackStaticTable.getIndexInsensitive(STATUS, STATUS_200); + } + + @Benchmark + @BenchmarkMode(Mode.AverageTime) + public int lookupNameAndValueMatchLast() { + return HpackStaticTable.getIndexInsensitive(STATUS, STATUS_500); + } + + @Benchmark + @BenchmarkMode(Mode.AverageTime) + public int lookupNameOnlyMatchBeginTable() { + return HpackStaticTable.getIndexInsensitive(AUTHORITY, AUTHORITY_NETTY); + } + @Benchmark @BenchmarkMode(Mode.AverageTime) public int lookupHttp() { @@ -78,7 +96,7 @@ public int lookupHttps() { @Benchmark @BenchmarkMode(Mode.AverageTime) - public int lookupNameOnlyMatch() { + public int lookupNameOnlyMatchEndTable() { return HpackStaticTable.getIndexInsensitive(USER_AGENT, USER_AGENT_CURL); } } From ea59e8cf6dd97ab1faecede8bfcc77b6676f0947 Mon Sep 17 00:00:00 2001 From: ahadadi Date: Sat, 27 Aug 2022 12:42:31 +0300 Subject: [PATCH 4/8] Make private methods in inner classes package private to avoid compiler generated synthetic methods. --- .../handler/codec/http2/HpackStaticTable.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 1b977766d2b..3a918d5af47 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -236,11 +236,11 @@ private static int bucket(CharSequence s, int shift, int mask) { } private static final class HeaderNameIndex { - private final CharSequence name; - private final int index; - private final boolean emptyValue; + final CharSequence name; + final int index; + final boolean emptyValue; - private HeaderNameIndex(CharSequence name, int index, boolean emptyValue) { + HeaderNameIndex(CharSequence name, int index, boolean emptyValue) { this.name = name; this.index = index; this.emptyValue = emptyValue; @@ -248,11 +248,11 @@ private HeaderNameIndex(CharSequence name, int index, boolean emptyValue) { } private static final class HeaderIndex { - private final CharSequence name; - private final CharSequence value; - private final int index; + final CharSequence name; + final CharSequence value; + final int index; - private HeaderIndex(CharSequence name, CharSequence value, int index) { + HeaderIndex(CharSequence name, CharSequence value, int index) { this.name = name; this.value = value; this.index = index; From e5c600a6b16c922317a05bf67933599a3e1524a8 Mon Sep 17 00:00:00 2001 From: ahadadi Date: Sun, 28 Aug 2022 09:59:05 +0300 Subject: [PATCH 5/8] Remove irrelevant test. --- .../io/netty/handler/codec/http2/HpackStaticTableTest.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java index 49c84342dda..e947440eff1 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java @@ -95,11 +95,6 @@ public void testExistingHeaderNameAndValueSecondMatch() { AsciiString.cached(":scheme"), AsciiString.cached("https"))); } - @Test - public void testLongHeaderValue() { - assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "longer then longest static value")); - } - @Test public void testExistingHeaderNameAndEmptyValueMismatch() { assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "")); From 74e24d442eba227142cf79454ca4394a104da040 Mon Sep 17 00:00:00 2001 From: ahadadi Date: Sun, 28 Aug 2022 23:27:00 +0300 Subject: [PATCH 6/8] Remove redundant null checks and associated tests. Analyzing HpackEncoder code, we cannot reach the relevant HpackStaticTable methods with null name or value. --- .../handler/codec/http2/HpackStaticTable.java | 6 --- .../codec/http2/HpackStaticTableTest.java | 42 ------------------- 2 files changed, 48 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 3a918d5af47..5906d152b37 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -38,7 +38,6 @@ import java.util.List; import static io.netty.handler.codec.http2.HpackUtil.equalsVariableTime; -import static io.netty.util.internal.ObjectUtil.checkNotNull; final class HpackStaticTable { @@ -184,7 +183,6 @@ static HpackHeaderField getEntry(int index) { * -1 if the header field name is not in the static table. */ static int getIndex(CharSequence name) { - checkNotNull(name, "name"); HeaderNameIndex entry = getEntry(name); return entry == null ? NOT_FOUND : entry.index; } @@ -194,10 +192,6 @@ static int getIndex(CharSequence name) { * header field is not in the static table. */ static int getIndexInsensitive(CharSequence name, CharSequence value) { - checkNotNull(name, "name"); - if (value == null) { - return NOT_FOUND; - } if (value.length() == 0) { HeaderNameIndex entry = getEntry(name); return entry == null || !entry.emptyValue ? NOT_FOUND : entry.index; diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java index e947440eff1..6f238f3c4c7 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackStaticTableTest.java @@ -17,53 +17,16 @@ import io.netty.util.AsciiString; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.function.Executable; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertThrows; public class HpackStaticTableTest { - @Test - public void testNullHeaderName() { - assertThrows(NullPointerException.class, new Executable() { - @Override - public void execute() { - HpackStaticTable.getIndex(null); - } - }); - } - @Test public void testEmptyHeaderName() { assertEquals(-1, HpackStaticTable.getIndex("")); } - @Test - public void testNullHeaderNameEmptyValue() { - assertThrows(NullPointerException.class, new Executable() { - @Override - public void execute() { - HpackStaticTable.getIndexInsensitive(null, ""); - } - }); - } - - @Test - public void testEmptyHeaderNameNullValue() { - assertEquals(-1, HpackStaticTable.getIndexInsensitive("", null)); - } - - @Test - public void testNullHeaderNameAndValue() { - assertThrows(NullPointerException.class, new Executable() { - @Override - public void execute() { - HpackStaticTable.getIndexInsensitive(null, null); - } - }); - } - @Test public void testMissingHeaderName() { assertEquals(-1, HpackStaticTable.getIndex("missing")); @@ -105,11 +68,6 @@ public void testExistingHeaderNameAndEmptyValueMatch() { assertEquals(27, HpackStaticTable.getIndexInsensitive("content-language", "")); } - @Test - public void testExistingHeaderNameAndNullValue() { - assertEquals(-1, HpackStaticTable.getIndexInsensitive("content-language", null)); - } - @Test public void testExistingHeaderNameButMissingValue() { assertEquals(-1, HpackStaticTable.getIndexInsensitive(":scheme", "missing")); From caf4beda84dc6f33e780374cd42677cff079c942 Mon Sep 17 00:00:00 2001 From: ahadadi Date: Sun, 28 Aug 2022 23:43:28 +0300 Subject: [PATCH 7/8] Improving comments. --- .../io/netty/handler/codec/http2/HpackStaticTable.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 5906d152b37..46b3ccfd1d7 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -117,12 +117,12 @@ private static HpackHeaderField newHeaderField(String name, String value) { return new HpackHeaderField(AsciiString.cached(name), AsciiString.cached(value)); } + // The table size and bit shift are chosen so that each hash bucket contains a single header name. private static final int HEADER_NAMES_TABLE_SIZE = 1 << 9; - // a bit shift chosen so that each header name will hash into a single bucket private static final int HEADER_NAMES_TABLE_SHIFT = PlatformDependent.BIG_ENDIAN_NATIVE_ORDER ? 22 : 18; - // A table holding header names and their associated indexes. + // A table mapping header names to their associated indexes. private static final HeaderNameIndex[] HEADER_NAMES = new HeaderNameIndex[HEADER_NAMES_TABLE_SIZE]; static { // Iterate through the static table in reverse order to @@ -140,14 +140,13 @@ private static HpackHeaderField newHeaderField(String name, String value) { } } + // The table size and bit shift are chosen so that each hash bucket contains a single header. private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE = 1 << 6; - // a bit shift chosen so that each header will hash into a single bucket private static final int HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SHIFT = PlatformDependent.BIG_ENDIAN_NATIVE_ORDER ? 0 : 6; - // A table holding header names and values for non-empty values. - // This table is keyed by value which is possible since each non-empty value is unique. + // A table mapping headers with non-empty values to their associated indexes. private static final HeaderIndex[] HEADERS_WITH_NON_EMPTY_VALUES = new HeaderIndex[HEADERS_WITH_NON_EMPTY_VALUES_TABLE_SIZE]; static { From 48e88bec5ec5b30dbcda52251ea73ed3620b2656 Mon Sep 17 00:00:00 2001 From: amirhadadi Date: Fri, 2 Sep 2022 15:48:53 +0300 Subject: [PATCH 8/8] Style changes Co-authored-by: Norman Maurer --- .../handler/codec/http2/HpackStaticTable.java | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 46b3ccfd1d7..666c14c3b18 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java @@ -194,17 +194,16 @@ static int getIndexInsensitive(CharSequence name, CharSequence value) { if (value.length() == 0) { HeaderNameIndex entry = getEntry(name); return entry == null || !entry.emptyValue ? NOT_FOUND : entry.index; - } else { - int bucket = headerBucket(value); - HeaderIndex header = HEADERS_WITH_NON_EMPTY_VALUES[bucket]; - if (header == null) { - return NOT_FOUND; - } - if (equalsVariableTime(header.name, name) && equalsVariableTime(header.value, value)) { - return header.index; - } + } + int bucket = headerBucket(value); + HeaderIndex header = HEADERS_WITH_NON_EMPTY_VALUES[bucket]; + if (header == null) { return NOT_FOUND; } + if (equalsVariableTime(header.name, name) && equalsVariableTime(header.value, value)) { + return header.index; + } + return NOT_FOUND; } private static HeaderNameIndex getEntry(CharSequence name) {