Encoder uses the static table. by jstordeur · Pull Request #10 · quic-go/qpack

jstordeur · 2020-08-02T22:48:20Z

A first pass at this, I haven't written any go in a while so bear with me.
I assume I should also add some support for the integration tests, how does this part work?

marten-seemann

Thank your for the PR. This looks pretty good already, just a few comments to simplify the code a bit.
I guess you introduced additional function parameters in preparation for dynamic table lookups, but I’d like to keep things as simple as possible in this PR.

Please also rebase your PR on the current master branch. I removed the failing Fuzzit build (they're shutting down their service), and added some linters.

marten-seemann · 2020-08-04T05:25:29Z

encoder.go

 	}

-	e.writeLiteralFieldWithoutNameReference(f)
+	nameFound, valueFound, idx := e.getHeaderFieldIndex(&f)


It seems this would be easier if you don't define a getHeaderFieldIndex function.
It should also be faster, as you can skip the map lookup for valueFound if nameFound is false.

Yeah, initially I thought I'd call it in several places but I ended up just making it harder to read.
Also, while inlining it I also found a bug:
I'm writing an indexed field if nameFound and len(f.Value) == 0 but that would be incorrect for some input.
For example {":method", "" } cannot be encoded with an indexed field.
Will add more test coverage.

encoder.go

marten-seemann · 2020-08-04T05:48:40Z

encoder.go

+// Encodes a header field whose name is present in one of the
+// tables. TBit is true if the idx refers to the static table.
+func (e *Encoder) writeLiteralFieldWithNameReference(
+	f *HeaderField, idx int, NBit, TBit bool) {


You don’t need NBit and TBit here. They are always false and true, respectively.

Ok I'll remove them for this PR.
The T bit is strictly related to the dynamic table but I'm not sure the N bit is. For example the N bit is present in the encoding of a Literal Field Line Without Name Reference even though this doesn't use the dynamic table at all.
The RFC says

The following bit, 'N', indicates whether an intermediary is permitted to add this field line to the dynamic table on subsequent hops.When the 'N' bit is set, the encoded field line MUST always be encoded with a literal representation. In particular, when a peer sends a field line that it received represented as a literal field line with the 'N' bit set, it MUST use a literal representation to forward this field line. This bit is intended for protecting field values that are not to be put at risk by compressing them; see Section 7 for more details.

So even if our implementation doesn't use the dynamic table we should offer the caller the possibility to set the N bit so that the value is not cached by a subsequent proxy down the line.
That being said I also think this can be done in a subsequent PR.

marten-seemann · 2020-08-04T05:53:20Z

static_table.go

+// There's a second level of mapping for the headers that have some predefined
+// values in the static table.
+var encoderMap = map[string]indexAndValues{
+	":authority":          indexAndValues{0, map[string]int{}},


Why not use nil for values that don’t have a value?

I just forgot this was possible in Go :)

marten-seemann · 2020-08-04T05:55:09Z

static_table.go

 }
+
+type indexAndValues struct {
+	idx    int


This could be a uint8, right?

marten-seemann · 2020-08-04T05:57:39Z

static_table.go

+// and what value should be used to encode it.
+// There's a second level of mapping for the headers that have some predefined
+// values in the static table.
+var encoderMap = map[string]indexAndValues{


Can you add a unit test that checks that encoderMap and staticTableEntries are consistent, e.g. by looping over each one of them and checking that the indexes exist and match in the other table?

marten-seemann · 2020-08-04T06:04:22Z

Regarding integration tests, the first step would be to add some in integration_test.go. I’d pick some entries from the static table (at random), and encode and then decode them. You could then check that the encoded value is shorter than it would be if the value hadn’t used the static table, e.g. by running two encodings: first with the actual value, then with the same value, but one letter replaced by something else (such that the table lookup fails).
If you want, I can add the integration test. Just let me know.

jstordeur

I also haven' t used git in a while so I might have merged instead of rebase...
I'm also adding a commit to address the comment, no idea how this is going to play out with the comments.

jstordeur · 2020-08-06T02:19:05Z

encoder.go

 	}

-	e.writeLiteralFieldWithoutNameReference(f)
+	nameFound, valueFound, idx := e.getHeaderFieldIndex(&f)


Yeah, initially I thought I'd call it in several places but I ended up just making it harder to read.
Also, while inlining it I also found a bug:
I'm writing an indexed field if nameFound and len(f.Value) == 0 but that would be incorrect for some input.
For example {":method", "" } cannot be encoded with an indexed field.
Will add more test coverage.

jstordeur · 2020-08-06T02:29:19Z

encoder.go

+// Encodes a header field whose name is present in one of the
+// tables. TBit is true if the idx refers to the static table.
+func (e *Encoder) writeLiteralFieldWithNameReference(
+	f *HeaderField, idx int, NBit, TBit bool) {


Ok I'll remove them for this PR.
The T bit is strictly related to the dynamic table but I'm not sure the N bit is. For example the N bit is present in the encoding of a Literal Field Line Without Name Reference even though this doesn't use the dynamic table at all.
The RFC says

The following bit, 'N', indicates whether an intermediary is permitted to add this field line to the dynamic table on subsequent hops.When the 'N' bit is set, the encoded field line MUST always be encoded with a literal representation. In particular, when a peer sends a field line that it received represented as a literal field line with the 'N' bit set, it MUST use a literal representation to forward this field line. This bit is intended for protecting field values that are not to be put at risk by compressing them; see Section 7 for more details.

So even if our implementation doesn't use the dynamic table we should offer the caller the possibility to set the N bit so that the value is not cached by a subsequent proxy down the line.
That being said I also think this can be done in a subsequent PR.

jstordeur · 2020-08-06T02:33:46Z

static_table.go

+// There's a second level of mapping for the headers that have some predefined
+// values in the static table.
+var encoderMap = map[string]indexAndValues{
+	":authority":          indexAndValues{0, map[string]int{}},


I just forgot this was possible in Go :)

jstordeur · 2020-08-06T02:42:01Z

static_table.go

 }
+
+type indexAndValues struct {
+	idx    int


jstordeur · 2020-08-06T02:50:51Z

static_table.go

+// and what value should be used to encode it.
+// There's a second level of mapping for the headers that have some predefined
+// values in the static table.
+var encoderMap = map[string]indexAndValues{


codecov-commenter · 2020-08-06T02:55:46Z

Codecov Report

Merging #10 into master will increase coverage by 0.94%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   91.13%   92.07%   +0.94%     
==========================================
  Files           4        4              
  Lines         203      227      +24     
==========================================
+ Hits          185      209      +24     
  Misses          9        9              
  Partials        9        9

Impacted Files	Coverage Δ
encoder.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b7462f...59d364f. Read the comment docs.

marten-seemann · 2020-08-06T04:09:21Z

Hello @jstordeur, thank you! This looks good now. Can you please rebase on top of the current master and squash all commits into one?

Also, there's a linter error here that we need to fix before merging. On master I also activated the gofmt linter and it complains about static_table.go not being properly gofmted.

marten-seemann · 2020-08-08T03:35:25Z

Hi @jstordeur, would be great if you could help push this PR to the finish line. I'm planning to cut a new release of quic-go very soon, and would like to include this change.

jstordeur · 2020-08-08T03:54:02Z

I think this should be OK now, my git is very rusty...
What about the go.mod and go.sum file changes? I suspect maybe my IDE made those changes in my back, I certainly didn't. Should I revert them?

marten-seemann · 2020-08-08T04:08:06Z

What about the go.mod and go.sum file changes? I suspect maybe my IDE made those changes in my back, I certainly didn't. Should I revert them?

Yes, that would be good. As far as I can see, you didn't remove / include any new dependencies, so go.mod and go.sum should be unchanged.

jstordeur · 2020-08-08T04:12:30Z

done, I'm off for today. Hopefully this is good to merge now.

marten-seemann

Looks good to me!
Thank you for your contribution, @jstordeur. This will give us significant compression savings on H3 requests and responses.

marten-seemann reviewed Aug 4, 2020

View reviewed changes

jstordeur commented Aug 6, 2020

View reviewed changes

jstordeur force-pushed the master branch 2 times, most recently from 7035845 to 977e16d Compare August 8, 2020 03:50

Encoder uses the static table.

59d364f

jstordeur force-pushed the master branch from 977e16d to 59d364f Compare August 8, 2020 04:11

marten-seemann approved these changes Aug 8, 2020

View reviewed changes

marten-seemann merged commit 8ef5426 into quic-go:master Aug 8, 2020

This was referenced Aug 8, 2020

use the static table #8

Closed

add more tests for encoding using the static table #15

Merged

aschmahmann mentioned this pull request Sep 22, 2020

Release 0.7.0 ipfs/kubo#7560

Closed

72 tasks

Uh oh!

Conversation

jstordeur commented Aug 2, 2020

Uh oh!

marten-seemann left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marten-seemann Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marten-seemann commented Aug 4, 2020

Uh oh!

jstordeur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marten-seemann commented Aug 6, 2020

Uh oh!

marten-seemann commented Aug 8, 2020

Uh oh!

jstordeur commented Aug 8, 2020

Uh oh!

marten-seemann commented Aug 8, 2020

Uh oh!

jstordeur commented Aug 8, 2020

Uh oh!

marten-seemann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marten-seemann left a comment •

edited

Loading

marten-seemann Aug 4, 2020 •

edited

Loading

marten-seemann Aug 4, 2020 •

edited

Loading

marten-seemann Aug 4, 2020 •

edited

Loading

marten-seemann Aug 4, 2020 •

edited

Loading

marten-seemann Aug 4, 2020 •

edited

Loading

codecov-commenter commented Aug 6, 2020 •

edited

Loading