http-client-branch: review comment - HPACK misc. improvements (2)
--- a/src/java.net.http/share/classes/jdk/internal/net/http/hpack/Decoder.java Wed May 30 12:01:03 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/hpack/Decoder.java Thu May 31 13:55:51 2018 +0100
@@ -28,6 +28,7 @@
import java.io.IOException;
import java.nio.ByteBuffer;
+import java.util.List;
import java.util.concurrent.atomic.AtomicLong;
import static jdk.internal.net.http.hpack.HPACK.Logger.Level.EXTRA;
@@ -66,7 +67,8 @@
private final Logger logger;
private static final AtomicLong DECODERS_IDS = new AtomicLong();
- private static final State[] states = new State[256];
+ /* An immutable list of states */
+ private static final List<State> states;
static {
// To be able to do a quick lookup, each of 256 possibilities are mapped
@@ -78,21 +80,23 @@
// I do it mainly for better debugging (to not go each time step by step
// through if...else tree). As for performance win for the decoding, I
// believe is negligible.
- for (int i = 0; i < states.length; i++) {
+ State[] s = new State[256];
+ for (int i = 0; i < s.length; i++) {
if ((i & 0b1000_0000) == 0b1000_0000) {
- states[i] = State.INDEXED;
+ s[i] = State.INDEXED;
} else if ((i & 0b1100_0000) == 0b0100_0000) {
- states[i] = State.LITERAL_WITH_INDEXING;
+ s[i] = State.LITERAL_WITH_INDEXING;
} else if ((i & 0b1110_0000) == 0b0010_0000) {
- states[i] = State.SIZE_UPDATE;
+ s[i] = State.SIZE_UPDATE;
} else if ((i & 0b1111_0000) == 0b0001_0000) {
- states[i] = State.LITERAL_NEVER_INDEXED;
+ s[i] = State.LITERAL_NEVER_INDEXED;
} else if ((i & 0b1111_0000) == 0b0000_0000) {
- states[i] = State.LITERAL;
+ s[i] = State.LITERAL;
} else {
throw new InternalError(String.valueOf(i));
}
}
+ states = List.of(s);
}
private final long id;
@@ -278,7 +282,7 @@
private void resumeReady(ByteBuffer input) {
int b = input.get(input.position()) & 0xff; // absolute read
- State s = states[b];
+ State s = states.get(b);
if (logger.isLoggable(EXTRA)) {
logger.log(EXTRA, () -> format("next binary representation %s (first byte 0x%02x)",
s, b));
@@ -388,15 +392,17 @@
try {
if (firstValueIndex) {
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal without indexing ('%s', '%s')",
- intValue, value));
+ logger.log(NORMAL, () -> format(
+ "literal without indexing (%s, '%s', huffman=%b)",
+ intValue, value, valueHuffmanEncoded));
}
SimpleHeaderTable.HeaderField f = getHeaderFieldAt(intValue);
action.onLiteral(intValue, f.name, value, valueHuffmanEncoded);
} else {
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal without indexing ('%s', '%s')",
- name, value));
+ logger.log(NORMAL, () -> format(
+ "literal without indexing ('%s', huffman=%b, '%s', huffman=%b)",
+ name, nameHuffmanEncoded, value, valueHuffmanEncoded));
}
action.onLiteral(name, nameHuffmanEncoded, value, valueHuffmanEncoded);
}
@@ -445,8 +451,9 @@
String v = value.toString();
if (firstValueIndex) {
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal with incremental indexing ('%s', '%s')",
- intValue, value));
+ logger.log(NORMAL, () -> format(
+ "literal with incremental indexing (%s, '%s', huffman=%b)",
+ intValue, value, valueHuffmanEncoded));
}
SimpleHeaderTable.HeaderField f = getHeaderFieldAt(intValue);
n = f.name;
@@ -454,8 +461,9 @@
} else {
n = name.toString();
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal with incremental indexing ('%s', '%s')",
- n, value));
+ logger.log(NORMAL, () -> format(
+ "literal with incremental indexing ('%s', huffman=%b, '%s', huffman=%b)",
+ n, nameHuffmanEncoded, value, valueHuffmanEncoded));
}
action.onLiteralWithIndexing(n, nameHuffmanEncoded, v, valueHuffmanEncoded);
}
@@ -496,15 +504,17 @@
try {
if (firstValueIndex) {
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal never indexed ('%s', '%s')",
- intValue, value));
+ logger.log(NORMAL, () -> format(
+ "literal never indexed (%s, '%s', huffman=%b)",
+ intValue, value, valueHuffmanEncoded));
}
SimpleHeaderTable.HeaderField f = getHeaderFieldAt(intValue);
action.onLiteralNeverIndexed(intValue, f.name, value, valueHuffmanEncoded);
} else {
if (logger.isLoggable(NORMAL)) {
- logger.log(NORMAL, () -> format("literal never indexed ('%s', '%s')",
- name, value));
+ logger.log(NORMAL, () -> format(
+ "literal never indexed ('%s', huffman=%b, '%s', huffman=%b)",
+ name, nameHuffmanEncoded, value, valueHuffmanEncoded));
}
action.onLiteralNeverIndexed(name, nameHuffmanEncoded, value, valueHuffmanEncoded);
}
--- a/src/java.net.http/share/classes/jdk/internal/net/http/hpack/Encoder.java Wed May 30 12:01:03 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/hpack/Encoder.java Thu May 31 13:55:51 2018 +0100
@@ -105,8 +105,8 @@
private static final AtomicLong ENCODERS_IDS = new AtomicLong();
- // TODO: enum: no huffman/smart huffman/always huffman
- private static final boolean DEFAULT_HUFFMAN = true;
+ /* Used to calculate the number of bytes required for Huffman encoding */
+ private final QuickHuffman.Writer huffmanWriter = new QuickHuffman.Writer();
private final Logger logger;
private final long id;
@@ -241,21 +241,30 @@
int index = t.indexOf(name, value);
if (index > 0) {
indexed(index);
- } else if (index < 0) {
- if (sensitive) {
- literalNeverIndexed(-index, value, DEFAULT_HUFFMAN);
+ } else {
+ boolean huffmanValue = isHuffmanBetterFor(value);
+ if (index < 0) {
+ if (sensitive) {
+ literalNeverIndexed(-index, value, huffmanValue);
+ } else {
+ literal(-index, value, huffmanValue);
+ }
} else {
- literal(-index, value, DEFAULT_HUFFMAN);
- }
- } else {
- if (sensitive) {
- literalNeverIndexed(name, DEFAULT_HUFFMAN, value, DEFAULT_HUFFMAN);
- } else {
- literal(name, DEFAULT_HUFFMAN, value, DEFAULT_HUFFMAN);
+ boolean huffmanName = isHuffmanBetterFor(name);
+ if (sensitive) {
+ literalNeverIndexed(name, huffmanName, value, huffmanValue);
+ } else {
+ literal(name, huffmanName, value, huffmanValue);
+ }
}
}
}
+ private boolean isHuffmanBetterFor(CharSequence value) {
+ // prefer Huffman encoding only if it is strictly smaller than Latin-1
+ return huffmanWriter.lengthOf(value) < value.length();
+ }
+
/**
* Sets a maximum capacity of the header table.
*
@@ -299,7 +308,7 @@
if (calculated < 0 || calculated > capacity) {
throw new IllegalArgumentException(
format("0 <= calculated <= capacity: calculated=%s, capacity=%s",
- calculated, capacity));
+ calculated, capacity));
}
capacityUpdate = true;
// maxCapacity needs to be updated unconditionally, so the encoder
@@ -416,8 +425,9 @@
boolean useHuffman)
throws IndexOutOfBoundsException {
if (logger.isLoggable(EXTRA)) {
- logger.log(EXTRA, () -> format("literal without indexing ('%s', '%s')",
- index, value));
+ logger.log(EXTRA, () -> format(
+ "literal without indexing (%s, '%s', huffman=%b)",
+ index, value, useHuffman));
}
checkEncoding();
encoding = true;
@@ -428,10 +438,12 @@
protected final void literal(CharSequence name,
boolean nameHuffman,
CharSequence value,
- boolean valueHuffman) {
+ boolean valueHuffman)
+ {
if (logger.isLoggable(EXTRA)) {
- logger.log(EXTRA, () -> format("literal without indexing ('%s', '%s')",
- name, value));
+ logger.log(EXTRA, () -> format(
+ "literal without indexing ('%s', huffman=%b, '%s', huffman=%b)",
+ name, nameHuffman, value, valueHuffman));
}
checkEncoding();
encoding = true;
@@ -442,10 +454,12 @@
protected final void literalNeverIndexed(int index,
CharSequence value,
boolean valueHuffman)
- throws IndexOutOfBoundsException {
+ throws IndexOutOfBoundsException
+ {
if (logger.isLoggable(EXTRA)) {
- logger.log(EXTRA, () -> format("literal never indexed ('%s', '%s')",
- index, value));
+ logger.log(EXTRA, () -> format(
+ "literal never indexed (%s, '%s', huffman=%b)",
+ index, value, valueHuffman));
}
checkEncoding();
encoding = true;
@@ -458,8 +472,9 @@
CharSequence value,
boolean valueHuffman) {
if (logger.isLoggable(EXTRA)) {
- logger.log(EXTRA, () -> format("literal never indexed ('%s', '%s')",
- name, value));
+ logger.log(EXTRA, () -> format(
+ "literal never indexed ('%s', huffman=%b, '%s', huffman=%b)",
+ name, nameHuffman, value, valueHuffman));
}
checkEncoding();
encoding = true;
@@ -472,8 +487,9 @@
boolean valueHuffman)
throws IndexOutOfBoundsException {
if (logger.isLoggable(EXTRA)) {
- logger.log(EXTRA, () -> format("literal with incremental indexing ('%s', '%s')",
- index, value));
+ logger.log(EXTRA, () -> format(
+ "literal with incremental indexing (%s, '%s', huffman=%b)",
+ index, value, valueHuffman));
}
checkEncoding();
encoding = true;
@@ -485,9 +501,10 @@
boolean nameHuffman,
CharSequence value,
boolean valueHuffman) {
- if (logger.isLoggable(EXTRA)) { // TODO: include huffman info?
- logger.log(EXTRA, () -> format("literal with incremental indexing ('%s', '%s')",
- name, value));
+ if (logger.isLoggable(EXTRA)) {
+ logger.log(EXTRA, () -> format(
+ "literal with incremental indexing ('%s', huffman=%b, '%s', huffman=%b)",
+ name, nameHuffman, value, valueHuffman));
}
checkEncoding();
encoding = true;
@@ -506,7 +523,7 @@
if (capacity > this.maxCapacity) {
throw new IllegalArgumentException(
format("capacity <= maxCapacity: capacity=%s, maxCapacity=%s",
- capacity, maxCapacity));
+ capacity, maxCapacity));
}
writer = sizeUpdateWriter.maxHeaderTableSize(capacity);
}
--- a/src/java.net.http/share/classes/jdk/internal/net/http/hpack/HeaderTable.java Wed May 30 12:01:03 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/hpack/HeaderTable.java Thu May 31 13:55:51 2018 +0100
@@ -28,7 +28,6 @@
import java.util.Deque;
import java.util.HashMap;
-import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.Map;
@@ -104,16 +103,24 @@
// Long.MAX_VALUE :-)
//
- private static final Map<String, LinkedHashMap<String, Integer>> staticIndexes;
+ /* An immutable map of static header fields' indexes */
+ private static final Map<String, Map<String, Integer>> staticIndexes;
static {
- staticIndexes = new HashMap<>(STATIC_TABLE_LENGTH); // TODO: Map.of
+ Map<String, Map<String, Integer>> map
+ = new HashMap<>(STATIC_TABLE_LENGTH);
for (int i = 1; i <= STATIC_TABLE_LENGTH; i++) {
- HeaderField f = staticTable[i];
- Map<String, Integer> values = staticIndexes
- .computeIfAbsent(f.name, k -> new LinkedHashMap<>());
+ HeaderField f = staticTable.get(i);
+ Map<String, Integer> values
+ = map.computeIfAbsent(f.name, k -> new HashMap<>());
values.put(f.value, i);
}
+ // create an immutable deep copy
+ Map<String, Map<String, Integer>> copy = new HashMap<>(map.size());
+ for (Map.Entry<String, Map<String, Integer>> e : map.entrySet()) {
+ copy.put(e.getKey(), Map.copyOf(e.getValue()));
+ }
+ staticIndexes = Map.copyOf(copy);
}
// name -> (value -> [index])
--- a/src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java Wed May 30 12:01:03 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/hpack/QuickHuffman.java Thu May 31 13:55:51 2018 +0100
@@ -643,7 +643,7 @@
} else if (!isLast) { // exit the method to accept more input
return;
} else if (bufferLen > 0) { // no more data is expected, pad
- // (this padding may be done more than once)
+ // (this padding may be done more than once)
buffer |= ((0xff00000000000000L >>> bufferLen)
& 0xff00000000000000L);
// do not update bufferLen, since all those ones are
--- a/src/java.net.http/share/classes/jdk/internal/net/http/hpack/SimpleHeaderTable.java Wed May 30 12:01:03 2018 +0100
+++ b/src/java.net.http/share/classes/jdk/internal/net/http/hpack/SimpleHeaderTable.java Thu May 31 13:55:51 2018 +0100
@@ -26,6 +26,7 @@
import jdk.internal.net.http.hpack.HPACK.Logger;
+import java.util.List;
import java.util.NoSuchElementException;
import static jdk.internal.net.http.common.Utils.pow2Size;
@@ -41,8 +42,9 @@
*/
class SimpleHeaderTable {
- protected static final HeaderField[] staticTable = {
- null, // To make index 1-based, instead of 0-based
+ /* An immutable list of static header fields */
+ protected static final List<HeaderField> staticTable = List.of(
+ new HeaderField(""), // A dummy to make the list index 1-based, instead of 0-based
new HeaderField(":authority"),
new HeaderField(":method", "GET"),
new HeaderField(":method", "POST"),
@@ -103,10 +105,9 @@
new HeaderField("user-agent"),
new HeaderField("vary"),
new HeaderField("via"),
- new HeaderField("www-authenticate")
- };
+ new HeaderField("www-authenticate"));
- protected static final int STATIC_TABLE_LENGTH = staticTable.length - 1;
+ protected static final int STATIC_TABLE_LENGTH = staticTable.size() - 1;
protected static final int ENTRY_SIZE = 32;
private final Logger logger;
@@ -134,7 +135,7 @@
HeaderField get(int index) {
checkIndex(index);
if (index <= STATIC_TABLE_LENGTH) {
- return staticTable[index];
+ return staticTable.get(index);
} else {
return buffer.get(index - STATIC_TABLE_LENGTH - 1);
}
@@ -263,23 +264,6 @@
public String toString() {
return value.isEmpty() ? name : name + ": " + value;
}
-
- @Override // TODO: remove since used only for testing
- public boolean equals(Object o) {
- if (this == o) {
- return true;
- }
- if (o == null || getClass() != o.getClass()) {
- return false;
- }
- HeaderField that = (HeaderField) o;
- return name.equals(that.name) && value.equals(that.value);
- }
-
- @Override // TODO: remove since used only for testing
- public int hashCode() {
- return 31 * name.hashCode() + value.hashCode();
- }
}
private final CircularBuffer<HeaderField> buffer = new CircularBuffer<>(0);
--- a/test/jdk/java/net/httpclient/http2/java.net.http/jdk/internal/net/http/hpack/HeaderTableTest.java Wed May 30 12:01:03 2018 +0100
+++ b/test/jdk/java/net/httpclient/http2/java.net.http/jdk/internal/net/http/hpack/HeaderTableTest.java Thu May 31 13:55:51 2018 +0100
@@ -26,9 +26,12 @@
import org.testng.annotations.Test;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import static org.testng.Assert.assertEquals;
+import static org.testng.AssertJUnit.assertTrue;
public class HeaderTableTest extends SimpleHeaderTableTest {
@@ -42,41 +45,39 @@
HeaderTable table = createHeaderTable(0);
Map<Integer, HeaderField> staticHeaderFields = createStaticEntries();
- Map<String, Integer> minimalIndexes = new HashMap<>();
+ Map<String, Set<Integer>> indexes = new HashMap<>();
for (Map.Entry<Integer, HeaderField> e : staticHeaderFields.entrySet()) {
Integer idx = e.getKey();
- String hName = e.getValue().name;
- Integer midx = minimalIndexes.get(hName);
- if (midx == null) {
- minimalIndexes.put(hName, idx);
- } else {
- minimalIndexes.put(hName, Math.min(idx, midx));
- }
+ String name = e.getValue().name;
+ indexes.merge(name, Set.of(idx), (v1, v2) -> {
+ HashSet<Integer> s = new HashSet<>();
+ s.addAll(v1);
+ s.addAll(v2);
+ return s;
+ });
}
- staticHeaderFields.entrySet().forEach(
- e -> {
- // lookup
- HeaderField actualHeaderField = table.get(e.getKey());
- HeaderField expectedHeaderField = e.getValue();
- assertEquals(actualHeaderField, expectedHeaderField);
+ staticHeaderFields.forEach((key, expectedHeaderField) -> {
+ // lookup
+ HeaderField actualHeaderField = table.get(key);
+ assertEquals(actualHeaderField.name, expectedHeaderField.name);
+ assertEquals(actualHeaderField.value, expectedHeaderField.value);
- // reverse lookup (name, value)
- String hName = expectedHeaderField.name;
- String hValue = expectedHeaderField.value;
- int expectedIndex = e.getKey();
- int actualIndex = table.indexOf(hName, hValue);
-
- assertEquals(actualIndex, expectedIndex);
+ // reverse lookup (name, value)
+ String hName = expectedHeaderField.name;
+ String hValue = expectedHeaderField.value;
+ int expectedIndex = key;
+ int actualIndex = table.indexOf(hName, hValue);
- // reverse lookup (name)
- int expectedMinimalIndex = minimalIndexes.get(hName);
- int actualMinimalIndex = table.indexOf(hName, "blah-blah");
+ assertEquals(actualIndex, expectedIndex);
- assertEquals(-actualMinimalIndex, expectedMinimalIndex);
- }
- );
+ // reverse lookup (name)
+ Set<Integer> expectedIndexes = indexes.get(hName);
+ int actualMinimalIndex = table.indexOf(hName, "blah-blah");
+
+ assertTrue(expectedIndexes.contains(-actualMinimalIndex));
+ });
}
@Test
--- a/test/jdk/java/net/httpclient/http2/java.net.http/jdk/internal/net/http/hpack/SimpleHeaderTableTest.java Wed May 30 12:01:03 2018 +0100
+++ b/test/jdk/java/net/httpclient/http2/java.net.http/jdk/internal/net/http/hpack/SimpleHeaderTableTest.java Thu May 31 13:55:51 2018 +0100
@@ -47,7 +47,7 @@
// https://tools.ietf.org/html/rfc7541#appendix-A
//
// @formatter:off
- private static final String SPEC =
+ private static final String SPECIFICATION =
" | 1 | :authority | |\n" +
" | 2 | :method | GET |\n" +
" | 3 | :method | POST |\n" +
@@ -125,7 +125,8 @@
Map<Integer, HeaderField> staticHeaderFields = createStaticEntries();
staticHeaderFields.forEach((index, expectedHeaderField) -> {
SimpleHeaderTable.HeaderField actualHeaderField = table.get(index);
- assertEquals(actualHeaderField, expectedHeaderField);
+ assertEquals(actualHeaderField.name, expectedHeaderField.name);
+ assertEquals(actualHeaderField.value, expectedHeaderField.value);
});
}
@@ -318,7 +319,7 @@
static Map<Integer, HeaderField> createStaticEntries() {
Pattern line = Pattern.compile(
"\\|\\s*(?<index>\\d+?)\\s*\\|\\s*(?<name>.+?)\\s*\\|\\s*(?<value>.*?)\\s*\\|");
- Matcher m = line.matcher(SPEC);
+ Matcher m = line.matcher(SPECIFICATION);
Map<Integer, HeaderField> result = new HashMap<>();
while (m.find()) {
int index = Integer.parseInt(m.group("index"));