Skip to content

Perf: Fix mis-usage of Arena in HPACK #6495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
masaori335 wants to merge 1 commit into from
Closed

Perf: Fix mis-usage of Arena in HPACK #6495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions proxy/hdrs/XPACK.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,56 @@ xpack_encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *valu

return p - buf_start;
}

int64_t
xpack_encode_string(Arena &arena, uint8_t *buf_start, const uint8_t *buf_end, const char *value, uint64_t value_len, uint8_t n)
{
uint8_t *p = buf_start;
bool use_huffman = true;
char *data = nullptr;
int64_t data_len = 0;

// TODO Choose whether to use Huffman encoding wisely
// cppcheck-suppress knownConditionTrueFalse; leaving "use_huffman" for wise huffman usage in the future
if (use_huffman && value_len) {
data = arena.str_alloc(value_len * 4);
data_len = huffman_encode(reinterpret_cast<uint8_t *>(data), reinterpret_cast<const uint8_t *>(value), value_len);
}

// Length
const int64_t len = xpack_encode_integer(p, buf_end, data_len, n);
if (len == -1) {
if (use_huffman && value_len) {
arena.str_free(data);
}
Copy link
Contributor

@ywkaras ywkaras Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use https://github.com/apache/trafficserver/blob/master/include/tscpp/util/PostScript.h to avoid having to repeat this free 3 times.

Copy link
Contributor Author

@masaori335 masaori335 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know we have this. I was thinking to introduce "defer" like this.

Copy link
Contributor

@ywkaras ywkaras Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dr. Zret picked the name PostScript.


return -1;
}

if (use_huffman) {
*p |= 0x01 << n;
} else {
*p &= ~(0x01 << n);
}
p += len;

if (buf_end < p || buf_end - p < data_len) {
if (use_huffman && value_len) {
arena.str_free(data);
}

return -1;
}

// Value
if (data_len) {
memcpy(p, data, data_len);
p += data_len;
}

if (use_huffman && value_len) {
arena.str_free(data);
}

return p - buf_start;
}
Copy link
Contributor

@ywkaras ywkaras Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative to using an arena here would be something like:

template <std::size_t EstSizeBound = 1024>
class LocalBuffer
{
public:
  LocalBuffer(std::size_t size) : _ptr(size > EstSizeBound ? new char[size] : _buf) {}

   char * get() const { return _ptr; }

  ~LocalBuffer()
  {
    if (_ptr != buf) {
      delete [] _ptr;
    }
  }

private:
  char _buf[EstSizeBound];
  char * const _ptr;
};

(Where 1024 is the default arena block size.). I'm guessing it would have better performance in the most likely scenarios.

Copy link
Contributor Author

@masaori335 masaori335 Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks smarter than what I told as alternative approach (switching alloca & malloc ).
I agree with performance is better in most cases. I'll try this way.

4 changes: 4 additions & 0 deletions proxy/hdrs/XPACK.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ const static int XPACK_ERROR_SIZE_EXCEEDED_ERROR = -2;
// These primitives are shared with HPACK and QPACK.
int64_t xpack_encode_integer(uint8_t *buf_start, const uint8_t *buf_end, uint64_t value, uint8_t n);
int64_t xpack_decode_integer(uint64_t &dst, const uint8_t *buf_start, const uint8_t *buf_end, uint8_t n);

// TODO: replace all xpack_encode_string() (without Arena) calls in QPACK
int64_t xpack_encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, uint64_t value_len, uint8_t n = 7);
int64_t xpack_encode_string(Arena &arena, uint8_t *buf_start, const uint8_t *buf_end, const char *value, uint64_t value_len,
uint8_t n = 7);
int64_t xpack_decode_string(Arena &arena, char **str, uint64_t &str_length, const uint8_t *buf_start, const uint8_t *buf_end,
uint8_t n = 7);
38 changes: 25 additions & 13 deletions proxy/http2/HPACK.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,12 @@ HpackIndexingTable::update_maximum_size(uint32_t new_size)
return _dynamic_table->update_maximum_size(new_size);
}

Arena &
HpackIndexingTable::arena()
{
return this->_arena;
}

//
// HpackDynamicTable
//
Expand Down Expand Up @@ -538,9 +544,10 @@ encode_literal_header_field_with_indexed_name(uint8_t *buf_start, const uint8_t
p += len;

// Value String
Arena &arena = indexing_table.arena();
int value_len;
const char *value = header.value_get(&value_len);
len = xpack_encode_string(p, buf_end, value, value_len);
len = xpack_encode_string(arena, p, buf_end, value, value_len);
if (len == -1) {
return -1;
}
Expand Down Expand Up @@ -581,16 +588,17 @@ encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf

// Convert field name to lower case to follow HTTP2 spec.
// This conversion is needed because WKSs in MIMEFields is old fashioned
Arena arena;
Arena &arena = indexing_table.arena();
int name_len;
const char *name = header.name_get(&name_len);
char *lower_name = arena.str_store(name, name_len);
char *lower_name = arena.str_alloc(name_len);
for (int i = 0; i < name_len; i++) {
lower_name[i] = ParseRules::ink_tolower(lower_name[i]);
lower_name[i] = ParseRules::ink_tolower(name[i]);
}

// Name String
len = xpack_encode_string(p, buf_end, lower_name, name_len);
len = xpack_encode_string(arena, p, buf_end, lower_name, name_len);
arena.str_free(lower_name);
if (len == -1) {
return -1;
}
Expand All @@ -599,7 +607,7 @@ encode_literal_header_field_with_new_name(uint8_t *buf_start, const uint8_t *buf
// Value String
int value_len;
const char *value = header.value_get(&value_len);
len = xpack_encode_string(p, buf_end, value, value_len);
len = xpack_encode_string(arena, p, buf_end, value, value_len);
if (len == -1) {
return -1;
}
Expand Down Expand Up @@ -647,9 +655,7 @@ decode_indexed_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,
int decoded_value_len;
const char *decoded_value = header.value_get(&decoded_value_len);

Arena arena;
Debug("hpack_decode", "Decoded field: %s: %s", arena.str_store(decoded_name, decoded_name_len),
arena.str_store(decoded_value, decoded_value_len));
Debug("hpack_decode", "Decoded field: %.*s: %.*s", decoded_name_len, decoded_name, decoded_value_len, decoded_value);
}

return len;
Expand All @@ -669,6 +675,7 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,
int64_t len = 0;
HpackField ftype = hpack_parse_field_type(*p);
bool has_http2_violation = false;
Arena &arena = indexing_table.arena();

if (ftype == HpackField::INDEXED_LITERAL) {
len = xpack_decode_integer(index, p, buf_end, 6);
Expand All @@ -686,8 +693,6 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,

p += len;

Arena arena;

// Decode header field name
if (index) {
indexing_table.get_header_field(index, header);
Expand All @@ -697,6 +702,9 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,

len = xpack_decode_string(arena, &name_str, name_str_len, p, buf_end);
if (len == XPACK_ERROR_COMPRESSION_ERROR) {
if (name_str) {
arena.str_free(name_str);
}
return HPACK_ERROR_COMPRESSION_ERROR;
}

Expand All @@ -711,6 +719,7 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,

p += len;
header.name_set(name_str, name_str_len);
arena.str_free(name_str);
}

// Decode header field value
Expand All @@ -719,11 +728,15 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,

len = xpack_decode_string(arena, &value_str, value_str_len, p, buf_end);
if (len == XPACK_ERROR_COMPRESSION_ERROR) {
if (value_str) {
arena.str_free(value_str);
}
return HPACK_ERROR_COMPRESSION_ERROR;
}

p += len;
header.value_set(value_str, value_str_len);
arena.str_free(value_str);

// Incremental Indexing adds header to header table as new entry
if (isIncremental) {
Expand All @@ -737,8 +750,7 @@ decode_literal_header_field(MIMEFieldWrapper &header, const uint8_t *buf_start,
int decoded_value_len;
const char *decoded_value = header.value_get(&decoded_value_len);

Debug("hpack_decode", "Decoded field: %s: %s", arena.str_store(decoded_name, decoded_name_len),
arena.str_store(decoded_value, decoded_value_len));
Debug("hpack_decode", "Decoded field: %.*s: %.*s", decoded_name_len, decoded_name, decoded_value_len, decoded_value);
}

if (has_http2_violation) {
Expand Down
3 changes: 3 additions & 0 deletions proxy/http2/HPACK.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ class HpackIndexingTable
uint32_t size() const;
bool update_maximum_size(uint32_t new_size);

Arena &arena();

private:
Arena _arena;
HpackDynamicTable *_dynamic_table;
};

Expand Down