Merge pull request #15418 from AspirinSJL/smaller_hpack

Decrease GRPC_CHTTP2_HPACKC_NUM_VALUES

src/core/ext/transport/chttp2/transport/hpack_encoder.cc

@@ -41,14 +41,18 @@
 #include "src/core/lib/transport/static_metadata.h"
 #include "src/core/lib/transport/timeout_encoding.h"
 
-#define HASH_FRAGMENT_1(x) ((x)&255)
-#define HASH_FRAGMENT_2(x) ((x >> 8) & 255)
-#define HASH_FRAGMENT_3(x) ((x >> 16) & 255)
-#define HASH_FRAGMENT_4(x) ((x >> 24) & 255)
+#define HASH_FRAGMENT_MASK (GRPC_CHTTP2_HPACKC_NUM_VALUES - 1)
+#define HASH_FRAGMENT_1(x) ((x)&HASH_FRAGMENT_MASK)
+#define HASH_FRAGMENT_2(x) \
+  (((x) >> GRPC_CHTTP2_HPACKC_NUM_VALUES_BITS) & HASH_FRAGMENT_MASK)
+#define HASH_FRAGMENT_3(x) \
+  (((x) >> (GRPC_CHTTP2_HPACKC_NUM_VALUES_BITS * 2)) & HASH_FRAGMENT_MASK)
+#define HASH_FRAGMENT_4(x) \
+  (((x) >> (GRPC_CHTTP2_HPACKC_NUM_VALUES_BITS * 3)) & HASH_FRAGMENT_MASK)
 
 /* if the probability of this item being seen again is < 1/x then don't add
    it to the table */
-#define ONE_ON_ADD_PROBABILITY 128
+#define ONE_ON_ADD_PROBABILITY (GRPC_CHTTP2_HPACKC_NUM_VALUES >> 1)
 /* don't consider adding anything bigger than this to the hpack table */
 #define MAX_DECODER_SPACE_USAGE 512
 
@@ -135,7 +139,7 @@ static void inc_filter(uint8_t idx, uint32_t* sum, uint8_t* elems) {
   } else {
     int i;
     *sum = 0;
-    for (i = 0; i < GRPC_CHTTP2_HPACKC_NUM_FILTERS; i++) {
+    for (i = 0; i < GRPC_CHTTP2_HPACKC_NUM_VALUES; i++) {
       elems[i] /= 2;
       (*sum) += elems[i];
     }

src/core/ext/transport/chttp2/transport/hpack_encoder.h

@@ -28,8 +28,9 @@
 #include "src/core/lib/transport/metadata_batch.h"
 #include "src/core/lib/transport/transport.h"
 
-#define GRPC_CHTTP2_HPACKC_NUM_FILTERS 256
-#define GRPC_CHTTP2_HPACKC_NUM_VALUES 256
+// This should be <= 8. We use 6 to save space.
+#define GRPC_CHTTP2_HPACKC_NUM_VALUES_BITS 6
+#define GRPC_CHTTP2_HPACKC_NUM_VALUES (1 << GRPC_CHTTP2_HPACKC_NUM_VALUES_BITS)
 /* initial table size, per spec */
 #define GRPC_CHTTP2_HPACKC_INITIAL_TABLE_SIZE 4096
 /* maximum table size we'll actually use */
@@ -58,7 +59,7 @@ typedef struct {
      a new literal should be added to the compression table or not.
      They track a single integer that counts how often a particular value has
      been seen. When that count reaches max (255), all values are halved. */
-  uint8_t filter_elems[GRPC_CHTTP2_HPACKC_NUM_FILTERS];
+  uint8_t filter_elems[GRPC_CHTTP2_HPACKC_NUM_VALUES];
 
   /* entry tables for keys & elems: these tables track values that have been
      seen and *may* be in the decompressor table */

test/core/transport/chttp2/hpack_encoder_test.cc

@@ -160,6 +160,8 @@ static void encode_int_to_str(int i, char* p) {
 }
 
 static void test_decode_table_overflow() {
+  // Decrease the default table size to make decode table overflow easier.
+  grpc_chttp2_hpack_compressor_set_max_table_size(&g_compressor, 1024);
   int i;
   char key[3], value[3];
   char* expect;
@@ -170,27 +172,20 @@ static void test_decode_table_overflow() {
       false,
   };
 
-  for (i = 0; i < 114; i++) {
+  for (i = 0; i < 29; i++) {
     encode_int_to_str(i, key);
     encode_int_to_str(i + 1, value);
-
-    if (i + 61 >= 127) {
+    if (i == 0) {
+      // 3fe107 corresponds to the table size update.
       gpr_asprintf(&expect,
-                   "000009 0104 deadbeef ff%02x 40 02%02x%02x 02%02x%02x",
-                   i + 61 - 127, key[0], key[1], value[0], value[1]);
-    } else if (i > 0) {
+                   "00000a 0104 deadbeef 3fe107 40 02%02x%02x 02%02x%02x",
+                   key[0], key[1], value[0], value[1]);
+      verify(params, expect, 1, key, value);
+    } else {
       gpr_asprintf(&expect,
                    "000008 0104 deadbeef %02x 40 02%02x%02x 02%02x%02x",
                    0x80 + 61 + i, key[0], key[1], value[0], value[1]);
-    } else {
-      gpr_asprintf(&expect, "000007 0104 deadbeef 40 02%02x%02x 02%02x%02x",
-                   key[0], key[1], value[0], value[1]);
-    }
-
-    if (i > 0) {
       verify(params, expect, 2, "aa", "ba", key, value);
-    } else {
-      verify(params, expect, 1, key, value);
     }
     gpr_free(expect);
   }