You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by maskit <gi...@git.apache.org> on 2015/12/04 01:56:33 UTC
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
GitHub user maskit opened a pull request:
https://github.com/apache/trafficserver/pull/362
TS-4018: Use HPACK huffman encoding always
https://issues.apache.org/jira/browse/TS-4018
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/maskit/trafficserver ts4018
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/362.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #362
----
commit 47232f31f8f5b27c95b0fbea5f76a89d99becbc7
Author: Masakazu Kitajo <mk...@yahoo-corp.jp>
Date: 2015-12-03T07:13:31Z
TS-4018: Use HPACK huffman encoding always
commit cca6709a4875874668291f35fdb4d1e24374ee88
Author: Masakazu Kitajo <mk...@yahoo-corp.jp>
Date: 2015-12-03T07:57:19Z
TS-4018: clang format
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/362#discussion_r46776556
--- Diff: proxy/http2/HPACK.cc ---
@@ -332,18 +332,41 @@ int64_t
encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, size_t value_len)
{
uint8_t *p = buf_start;
+ bool use_huffman = true;
+ char *data = NULL;
+ int64_t data_len = 0;
+
+ // TODO Choose whether to use Huffman encoding wisely
+
+ if (use_huffman) {
+ data = static_cast<char *>(ats_malloc(value_len * 4));
+ if (data == NULL)
+ return -1;
+ data_len = huffman_encode(reinterpret_cast<uint8_t *>(data), reinterpret_cast<const uint8_t *>(value), value_len);
+ } else {
+ data = const_cast<char *>(value);
+ data_len = value_len;
+ }
// Length
- const int64_t len = encode_integer(p, buf_end, value_len, 7);
+ const int64_t len = encode_integer(p, buf_end, data_len, 7);
if (len == -1)
return -1;
+ if (use_huffman) {
+ *p |= 0x80;
+ }
p += len;
- if (buf_end < p || static_cast<size_t>(buf_end - p) < value_len)
+ if (buf_end < p || static_cast<size_t>(buf_end - p) < data_len)
--- End diff --
I get this error when compiling:
HPACK.cc:359:55: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
if (buf_end < p || static_cast<size_t>(buf_end - p) < data_len)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on the pull request:
https://github.com/apache/trafficserver/pull/362#issuecomment-162387824
I fixed the compile error, and squashed the commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by maskit <gi...@git.apache.org>.
Github user maskit commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/362#discussion_r46780362
--- Diff: proxy/http2/HPACK.cc ---
@@ -332,18 +332,41 @@ int64_t
encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, size_t value_len)
{
uint8_t *p = buf_start;
+ bool use_huffman = true;
--- End diff --
I left the flag for further use. There are some cases that plain text is shorter than huffman encoded data, and the codes for both cases would be used when we found a smart way to switch the encoding.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by masaori335 <gi...@git.apache.org>.
Github user masaori335 commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/362#discussion_r46780770
--- Diff: proxy/http2/HPACK.cc ---
@@ -332,18 +332,41 @@ int64_t
encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, size_t value_len)
{
uint8_t *p = buf_start;
+ bool use_huffman = true;
--- End diff --
According to [Compression Ratio](https://github.com/http2jp/hpack-test-case/wiki/Compression-Ratio) in [hpack-test-case](https://github.com/http2jp/hpack-test-case), the best compression rate is `0.31` but compression rate of naive-huffman imprementation (I guess this is similar to this PR) is `0.85`. So we have to back to here and implement smart algorithm.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/362#issuecomment-161873548
FYI it is better to squash the format commits rather than have them appear separately.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/362
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4018: Use HPACK huffman encoding al...
Posted by bryancall <gi...@git.apache.org>.
Github user bryancall commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/362#discussion_r46776397
--- Diff: proxy/http2/HPACK.cc ---
@@ -332,18 +332,41 @@ int64_t
encode_string(uint8_t *buf_start, const uint8_t *buf_end, const char *value, size_t value_len)
{
uint8_t *p = buf_start;
+ bool use_huffman = true;
--- End diff --
Why have a flat to use huffman if you are not going to change it to false?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---