From 208893aac98ecb94ec190f1834db928e2e7908a0 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Sun, 14 Aug 2016 15:54:06 -0700 Subject: [PATCH] HTTP/2 Hpack Encoder Cleanup Motivation: The HTTP/2 HPACK Encoder class has some code which is only used for test purposes. This code can be removed to reduce complexity and member variable count. Modifications: - Remove test code and update unit tests - Other minor cleanup Result: Test code is removed from operational code. --- .../codec/http2/internal/hpack/Encoder.java | 68 ++++--------------- .../codec/http2/internal/hpack/TestCase.java | 5 +- .../hpack/testdata/testDuplicateHeaders.json | 5 +- .../internal/hpack/testdata/testEviction.json | 9 ++- .../testdata/testMaxHeaderTableSize.json | 12 ++-- .../hpack/testdata/testSpecExampleC2_1.json | 5 +- .../hpack/testdata/testSpecExampleC2_2.json | 7 +- .../hpack/testdata/testSpecExampleC2_3.json | 4 +- .../hpack/testdata/testSpecExampleC3.json | 11 ++- .../hpack/testdata/testSpecExampleC5.json | 22 +++--- .../hpack/testdata/testSpecExampleC6.json | 3 +- .../testdata/testStaticTableEntries.json | 1 - .../internal/hpack/EncoderBenchmark.java | 8 ++- 13 files changed, 54 insertions(+), 106 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java index 4051dc3704..a4c9e86140 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java @@ -46,11 +46,6 @@ import static java.lang.Math.max; import static java.lang.Math.min; public final class Encoder { - // for testing - private final boolean useIndexing; - private final boolean forceHuffmanOn; - private final boolean forceHuffmanOff; - // a linked hash map of header fields private final HeaderEntry[] headerFields; private final HeaderEntry head = new HeaderEntry(-1, AsciiString.EMPTY_STRING, @@ -71,25 +66,9 @@ public final class Encoder { * Creates a new encoder. */ public Encoder(int maxHeaderTableSize, int arraySizeHint) { - this(maxHeaderTableSize, true, false, false, arraySizeHint); - } - - /** - * Constructor for testing only. - */ - Encoder( - int maxHeaderTableSize, - boolean useIndexing, - boolean forceHuffmanOn, - boolean forceHuffmanOff, - int arraySizeHint - ) { if (maxHeaderTableSize < 0) { throw new IllegalArgumentException("Illegal Capacity: " + maxHeaderTableSize); } - this.useIndexing = useIndexing; - this.forceHuffmanOn = forceHuffmanOn; - this.forceHuffmanOff = forceHuffmanOff; capacity = maxHeaderTableSize; // Enforce a bound of [2, 128] because hashMask is a byte. The max possible value of hashMask is one less // than the length of this array, and we want the mask to be > 0. @@ -144,16 +123,9 @@ public final class Encoder { // Section 6.1. Indexed Header Field Representation encodeInteger(out, 0x80, 7, staticTableIndex); } else { - int nameIndex = getNameIndex(name); - if (useIndexing) { - ensureCapacity(headerSize); - } - HpackUtil.IndexType indexType = - useIndexing ? INCREMENTAL : NONE; - encodeLiteral(out, name, value, indexType, nameIndex); - if (useIndexing) { - add(name, value); - } + ensureCapacity(headerSize); + encodeLiteral(out, name, value, INCREMENTAL, getNameIndex(name)); + add(name, value); } } } @@ -184,24 +156,17 @@ public final class Encoder { * Encode integer according to Section 5.1. */ private static void encodeInteger(ByteBuf out, int mask, int n, int i) { - if (n < 0 || n > 8) { - throw new IllegalArgumentException("N: " + n); - } + assert n >= 0 && n <= 8 : "N: " + n; int nbits = 0xFF >>> (8 - n); if (i < nbits) { out.writeByte(mask | i); } else { out.writeByte(mask | nbits); int length = i - nbits; - for (;;) { - if ((length & ~0x7F) == 0) { - out.writeByte(length); - return; - } else { - out.writeByte((length & 0x7F) | 0x80); - length >>>= 7; - } + for (; (length & ~0x7F) != 0; length >>>= 7) { + out.writeByte((length & 0x7F) | 0x80); } + out.writeByte(length); } } @@ -210,7 +175,7 @@ public final class Encoder { */ private void encodeStringLiteral(ByteBuf out, CharSequence string) { int huffmanLength = huffmanEncoder.getEncodedLength(string); - if ((huffmanLength < string.length() && !forceHuffmanOff) || forceHuffmanOn) { + if (huffmanLength < string.length()) { encodeInteger(out, 0x80, 7, huffmanLength); huffmanEncoder.encode(out, string); } else { @@ -232,26 +197,21 @@ public final class Encoder { */ private void encodeLiteral(ByteBuf out, CharSequence name, CharSequence value, HpackUtil.IndexType indexType, int nameIndex) { - int mask; - int prefixBits; + boolean nameIndexValid = nameIndex != -1; switch (indexType) { case INCREMENTAL: - mask = 0x40; - prefixBits = 6; + encodeInteger(out, 0x40, 6, nameIndexValid ? nameIndex : 0); break; case NONE: - mask = 0x00; - prefixBits = 4; + encodeInteger(out, 0x00, 4, nameIndexValid ? nameIndex : 0); break; case NEVER: - mask = 0x10; - prefixBits = 4; + encodeInteger(out, 0x10, 4, nameIndexValid ? nameIndex : 0); break; default: - throw new IllegalStateException("should not reach here"); + throw new Error("should not reach here"); } - encodeInteger(out, mask, prefixBits, nameIndex == -1 ? 0 : nameIndex); - if (nameIndex == -1) { + if (!nameIndexValid) { encodeStringLiteral(out, name); } encodeStringLiteral(out, value); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/TestCase.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/TestCase.java index dfc7e39122..92458c01bb 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/TestCase.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/TestCase.java @@ -61,10 +61,7 @@ final class TestCase { .create(); int maxHeaderTableSize = -1; - boolean useIndexing = true; boolean sensitiveHeaders; - boolean forceHuffmanOn; - boolean forceHuffmanOff; List headerBlocks; @@ -161,7 +158,7 @@ final class TestCase { maxHeaderTableSize = Integer.MAX_VALUE; } - return new Encoder(maxHeaderTableSize, useIndexing, forceHuffmanOn, forceHuffmanOff, 16); + return new Encoder(maxHeaderTableSize); } private Decoder createDecoder() { diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testDuplicateHeaders.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testDuplicateHeaders.json index 7c95a71da5..8e07effb24 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testDuplicateHeaders.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testDuplicateHeaders.json @@ -1,5 +1,4 @@ { - "force_huffman_on": true, "header_blocks": [ { @@ -9,7 +8,7 @@ ], "encoded": [ "4487 6107 a4b5 8d33 ff40 86f2 b12d 424f", - "4f83 ee3a 3f" + "4f03 7661 6c" ], "dynamic_table": [ { "x-custom": "val" }, @@ -40,7 +39,7 @@ { "x-custom": "val" } ], "encoded": [ - "bfbe 4082 94e7 838c 767f bf" + "bfbe 4082 94e7 0362 6172 bf" ], "dynamic_table": [ { "foo": "bar" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testEviction.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testEviction.json index 25693d9f10..fb5ddcf416 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testEviction.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testEviction.json @@ -1,6 +1,5 @@ { "max_header_table_size": 128, - "force_huffman_on": true, "header_blocks": [ { @@ -28,8 +27,8 @@ { "x-custom": "val6" } ], "encoded": [ - "4487 6107 a4b5 8d33 ff7f 0083 ee3a 1a7e", - "83ee 3a1b 7e83 ee3a 1c" + "4487 6107 a4b5 8d33 ff40 86f2 b12d 424f", + "4f83 ee3a 1a7e 83ee 3a1b 7e83 ee3a 1c" ], "dynamic_table": [ { "x-custom": "val6" }, @@ -45,8 +44,8 @@ { "x-custom": "val3" } ], "encoded": [ - "4487 6107 a4b5 8d33 ff7f 0083 ee3a 037e", - "83ee 3a05 7e83 ee3a 19" + "4487 6107 a4b5 8d33 ff40 86f2 b12d 424f", + "4f83 ee3a 037e 83ee 3a05 7e83 ee3a 19" ], "dynamic_table": [ { "x-custom": "val3" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testMaxHeaderTableSize.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testMaxHeaderTableSize.json index 274e11c8e0..69d3b8c183 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testMaxHeaderTableSize.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testMaxHeaderTableSize.json @@ -1,6 +1,5 @@ { "max_header_table_size": 128, - "force_huffman_off": true, "header_blocks": [ { @@ -10,9 +9,8 @@ { "name3": "val3" } ], "encoded": [ - "4005 6e61 6d65 3104 7661 6c31 4005 6e61", - "6d65 3204 7661 6c32 4005 6e61 6d65 3304", - "7661 6c33" + "4084 a874 943f 83ee 3a03 4084 a874 945f", + "83ee 3a05 4084 a874 959f 83ee 3a19" ], "dynamic_table": [ { "name3": "val3" }, @@ -28,7 +26,7 @@ { "name2": "val2" } ], "encoded": [ - "3f32 be40 056e 616d 6532 0476 616c 32" + "3f32 be40 84a8 7494 5f83 ee3a 05" ], "dynamic_table": [ { "name2": "val2" } @@ -43,8 +41,8 @@ { "name3": "val3" } ], "encoded": [ - "3f61 4005 6e61 6d65 3104 7661 6c31 bf40", - "056e 616d 6533 0476 616c 33" + "3f61 4084 a874 943f 83ee 3a03 bf40 84a8", + "7495 9f83 ee3a 19" ], "dynamic_table": [ { "name3": "val3" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_1.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_1.json index 6ea9b6f9d6..0838aba003 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_1.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_1.json @@ -1,5 +1,4 @@ { - "force_huffman_off": true, "header_blocks": [ { @@ -7,8 +6,8 @@ { "custom-key": "custom-header" } ], "encoded": [ - "400a 6375 7374 6f6d 2d6b 6579 0d63 7573", - "746f 6d2d 6865 6164 6572" + "4088 25a8 49e9 5ba9 7d7f 8925 a849 e95a", + "728e 42d9" ], "dynamic_table": [ { "custom-key": "custom-header" } diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_2.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_2.json index 5a222a7af6..01fb8fe5c0 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_2.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_2.json @@ -1,6 +1,4 @@ { - "use_indexing": false, - "force_huffman_off": true, "header_blocks": [ { @@ -8,11 +6,12 @@ { ":path": "/sample/path" } ], "encoded": [ - "040c 2f73 616d 706c 652f 7061 7468" + "4489 6103 a6ba 0ac5 634c ff" ], "dynamic_table": [ + { ":path": "/sample/path" } ], - "table_size": 0 + "table_size": 49 } ] } diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_3.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_3.json index 534e8f308b..50892d647e 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_3.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC2_3.json @@ -1,6 +1,5 @@ { "sensitive_headers": true, - "force_huffman_off": true, "header_blocks": [ { @@ -8,8 +7,7 @@ { "password": "secret" } ], "encoded": [ - "1008 7061 7373 776f 7264 0673 6563 7265", - "74" + "1086 ac68 4783 d927 8441 4961 53" ], "dynamic_table": [ ], diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC3.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC3.json index 93ece94709..3e8f658b43 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC3.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC3.json @@ -1,5 +1,4 @@ { - "force_huffman_off": true, "header_blocks": [ { @@ -10,8 +9,8 @@ { ":authority": "www.example.com" } ], "encoded": [ - "8286 8441 0f77 7777 2e65 7861 6d70 6c65", - "2e63 6f6d" + "8286 8441 8cf1 e3c2 e5f2 3a6b a0ab 90f4", + "ff" ], "dynamic_table": [ { ":authority": "www.example.com" } @@ -27,7 +26,7 @@ { "cache-control": "no-cache" } ], "encoded": [ - "8286 84be 5808 6e6f 2d63 6163 6865" + "8286 84be 5886 a8eb 1064 9cbf" ], "dynamic_table": [ { "cache-control": "no-cache" }, @@ -44,8 +43,8 @@ { "custom-key": "custom-value" } ], "encoded": [ - "8287 85bf 400a 6375 7374 6f6d 2d6b 6579", - "0c63 7573 746f 6d2d 7661 6c75 65" + "8287 85bf 4088 25a8 49e9 5ba9 7d7f 8925", + "a849 e95b b8e8 b4bf" ], "dynamic_table": [ { "custom-key": "custom-value" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC5.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC5.json index 93884b2b28..28ec5036c2 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC5.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC5.json @@ -1,6 +1,5 @@ { "max_header_table_size": 256, - "force_huffman_off": true, "header_blocks": [ { @@ -11,11 +10,10 @@ { "location": "https://www.example.com" } ], "encoded": [ - "4803 3330 3258 0770 7269 7661 7465 611d", - "4d6f 6e2c 2032 3120 4f63 7420 3230 3133", - "2032 303a 3133 3a32 3120 474d 546e 1768", - "7474 7073 3a2f 2f77 7777 2e65 7861 6d70", - "6c65 2e63 6f6d" + "4882 6402 5885 aec3 771a 4b61 96d0 7abe", + "9410 54d4 44a8 2005 9504 0b81 66e0 82a6", + "2d1b ff6e 919d 29ad 1718 63c7 8f0b 97c8", + "e9ae 82ae 43d3" ], "dynamic_table": [ { "location": "https://www.example.com" }, @@ -53,13 +51,11 @@ { "set-cookie": "foo=ASDJKHQKBZXOQWEOPIUAXQWEOIU; max-age=3600; version=1" } ], "encoded": [ - "88c1 611d 4d6f 6e2c 2032 3120 4f63 7420", - "3230 3133 2032 303a 3133 3a32 3220 474d", - "54c0 5a04 677a 6970 7738 666f 6f3d 4153", - "444a 4b48 514b 425a 584f 5157 454f 5049", - "5541 5851 5745 4f49 553b 206d 6178 2d61", - "6765 3d33 3630 303b 2076 6572 7369 6f6e", - "3d31" + "88c1 6196 d07a be94 1054 d444 a820 0595", + "040b 8166 e084 a62d 1bff c05a 839b d9ab", + "77ad 94e7 821d d7f2 e6c7 b335 dfdf cd5b", + "3960 d5af 2708 7f36 72c1 ab27 0fb5 291f", + "9587 3160 65c0 03ed 4ee5 b106 3d50 07" ], "dynamic_table": [ { "set-cookie": "foo=ASDJKHQKBZXOQWEOPIUAXQWEOIU; max-age=3600; version=1" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC6.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC6.json index 78b00e751a..28ec5036c2 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC6.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testSpecExampleC6.json @@ -1,6 +1,5 @@ { "max_header_table_size": 256, - "force_huffman_on": true, "header_blocks": [ { @@ -32,7 +31,7 @@ { "location": "https://www.example.com" } ], "encoded": [ - "4883 640e ffc1 c0bf" + "4803 3330 37c1 c0bf" ], "dynamic_table": [ { ":status": "307" }, diff --git a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testStaticTableEntries.json b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testStaticTableEntries.json index 1e3e1683d8..fcac28aea9 100644 --- a/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testStaticTableEntries.json +++ b/codec-http2/src/test/resources/io/netty/handler/codec/http2/internal/hpack/testdata/testStaticTableEntries.json @@ -1,5 +1,4 @@ { - "force_huffman_on": true, "header_blocks": [ { diff --git a/microbench/src/main/java/io/netty/microbench/http2/internal/hpack/EncoderBenchmark.java b/microbench/src/main/java/io/netty/microbench/http2/internal/hpack/EncoderBenchmark.java index 37200e9a19..a14d738f16 100644 --- a/microbench/src/main/java/io/netty/microbench/http2/internal/hpack/EncoderBenchmark.java +++ b/microbench/src/main/java/io/netty/microbench/http2/internal/hpack/EncoderBenchmark.java @@ -40,8 +40,11 @@ import org.openjdk.jmh.annotations.Fork; import org.openjdk.jmh.annotations.Level; import org.openjdk.jmh.annotations.Measurement; import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; import org.openjdk.jmh.annotations.TearDown; import org.openjdk.jmh.annotations.Threads; import org.openjdk.jmh.annotations.Warmup; @@ -49,11 +52,14 @@ import org.openjdk.jmh.infra.Blackhole; import java.io.IOException; import java.util.List; +import java.util.concurrent.TimeUnit; @Fork(1) @Threads(1) +@State(Scope.Benchmark) @Warmup(iterations = 5) @Measurement(iterations = 5) +@OutputTimeUnit(TimeUnit.NANOSECONDS) public class EncoderBenchmark extends AbstractMicrobenchmark { @Param @@ -86,7 +92,7 @@ public class EncoderBenchmark extends AbstractMicrobenchmark { } @Benchmark - @BenchmarkMode(Mode.Throughput) + @BenchmarkMode(Mode.AverageTime) public void encode(Blackhole bh) throws IOException { Encoder encoder = new Encoder(maxTableSize); output.clear();