bneradt commented on code in PR #13901:
URL: https://github.com/apache/trafficserver/pull/13901#discussion_r4390849323
##########
src/proxy/hdrs/XPACK.cc:
##########
@@ -191,26 +191,27 @@ xpack_encode_integer(uint8_t *buf_start, const uint8_t
*buf_end, uint64_t value,
int64_t
xpack_encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char
*value, uint64_t value_len, uint8_t n)
{
- uint8_t *p = buf_start;
- constexpr bool use_huffman = true;
+ uint8_t *p = buf_start;
uint32_t const max_expected_size = value_len * 4;
ts::LocalBuffer<uint8_t, 4096> local_buffer(max_expected_size);
uint8_t *data = local_buffer.data();
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) {
+ if (value_len) {
data_len = huffman_encode(data, max_expected_size, reinterpret_cast<const
uint8_t *>(value), value_len);
}
if (data_len < 0) {
return -1;
}
+ // Only use Huffman if the encoded representation is smaller than the raw
string.
+ const bool use_huffman = (value_len == 0 ||
static_cast<uint64_t>(data_len) < value_len);
Review Comment:
Optional / possible follow-up: for incompressible values we now
Huffman-encode the whole string into the `4 * value_len` scratch buffer only to
throw the result away. Since the decision only needs the encoded length, it
could be computed up front by summing the per-symbol code lengths from the
static Huffman table (nghttp2 does this with `nghttp2_hd_huff_encode_count`),
and `huffman_encode` called only when it wins. That would also let the raw path
skip the `LocalBuffer` entirely. Fine to leave for later since the common case
(ASCII header values) compresses.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
