Subject: Re: [quicwg/base-drafts] QPACK [editorial] Misc minor editorial changes. (#2942)

From: afrind <[email-masked]>
Date: Tue, 15 Oct 2019 16:40:28 -0700

afrind commented on this pull request.

> @@ -445,19 +443,23 @@ encoder sends a Set Dynamic Table Capacity instruction
 ({{set-dynamic-capacity}}) with a non-zero capacity to begin using the dynamic
 table.
 
-Before a new entry is added to the dynamic table, entries are evicted from the
-end of the dynamic table until the size of the dynamic table is less than or
-equal to (table capacity - size of new entry). The encoder MUST NOT evict a
-blocking dynamic table entry (see {{blocked-insertion}}).  The entry is then
-added to the table.  It is an error if the encoder attempts to add an entry that
-is larger than the dynamic table capacity; the decoder MUST treat this as a
+When a new entry is added to the dynamic table, the minimum number of entries
+are evicted from the end of the dynamic table so that the final table size
+including the new entry does not exceed the table capacity.  This can be

This is an awfully long sentence without punctuation.  Maybe set off 'including the new entry' with commas?

> +including the new entry does not exceed the table capacity.  This can be
+achieved by adding the new entry and then removing entries from the end until
+the table size is less than or equal to the table capacity.  Another option is
+to first evict entries until the size of the dynamic table is less than or equal
+to (table capacity - size of new entry), then add the new entry.  Note that a

Actually, this language comes directly from HPACK.  Why is it necessary to offer the add+remove option, instead of remove+add?  There isn't normative language here, so an implementation can do it either way and still be compliant.

> @@ -603,7 +607,8 @@ is used without modification.
 This document expands the definition of string literals and permits them to
 begin other than on a byte boundary.  An "N-bit prefix string literal" begins
 with the same Huffman flag, followed by the length encoded as an (N-1)-bit
-prefix integer.  The remainder of the string literal is unmodified.
+prefix integer.  The prefix size, N, can have a value between 2 and 8.  The

I didn't see a 2-bit prefix anywhere, should this be 3 and 8 (inclusive)?

But I see this as a bit of overkill.  If we allow 1 or 2 in QPACK2 we have to remember to change this.

 A header block that does not reference the dynamic table can use any value for
-the Base; setting Delta Base to zero is the most efficient encoding.
+the Base; setting Delta Base to zero is one of the most efficient encodings.

It's true there are other 2 byte encodings, but I think we should encourage 0/0.  Maybe we should even require it?

> @@ -952,10 +954,8 @@ entry.
 
 ### Indexed Header Field
 
-An indexed header field representation identifies an entry in either the static
-table or the dynamic table and causes that header field to be added to the
-decoded header list, as described in Section 3.2 of [RFC7541].
-<!-- is the 7541 reference still helpful here -->
+An indexed header field instruction identifies an entry in either the static

Why did you change this from representation to instruction?  Though this isn't currently spelled out directly, the encoder/decoder streams contain "instructions" - which cause the receiver to take an action.  Header blocks contain representations, which emit headers fields.  The word instruction doesn't appear in this section.

The other representations don't seem to call themselves such, however.  And only this one contains the language about adding to the decoded header list.  I guess I prefer emit also, but maybe we need to make another consistency pass.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#pullrequestreview-302247684

---

From: Bence Béky <[email-masked]>
Date: Wed, 16 Oct 2019 14:18:59 +0000 (UTC)

bencebeky commented on this pull request.

 ### Indexed Header Field

Sorry about "instruction".  I had not realized the distinction between instructions and representations when I started working on this PR, and even though later I reverted most of my related changes, I missed this one.  I'm changing it back to "representation".

As far as "add" versus "emit" is concerned, 4.5.2 has "causes to be added", 4.5.3 does not say anything explicitly, 4.5.4 and 4.5.5 has "represents", and 4.5.6 has "encodes".  Let me address this in another PR.  Until then I might as well revert to "causes to be added".

My motivation for the change to this sentence is that I consider the statement "setting Delta Base to zero is the most efficient encoding" to be incorrect.  I agree that it is nice to encourage 0/0, and I think this is still the case after my proposed change.  As far as requiring it, I see no advantage to that, but in any case that change might not be editorial, so I think it should be done in a separate PR.

For comparison, RFC7541 5.1 specificly allows the prefix length to be between 1 and 8, inclusive, even though prefix lengths 1, 2, and 8 are never used in HPACK.  And that's great because now QPACK does not have to call out that the prefix can be 8 bits (for example, for Encoder Required Insert Count).  So I suggest to be as permissive here as reasonable.

If we allow N to be 1, then after the Huffman bit, the length is encoded as an 8-bit prefix integer, and not an N-1 = 0 bit prefix integer.  Is this worth adding here given that it's not used?

It's a good point that this section is not normative.  Let me just revert most of my changes to this paragraph for sake of simplicity.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r335487671

---

From: Bence Béky <[email-masked]>
Date: Wed, 16 Oct 2019 07:19:14 -0700

@bencebeky pushed 3 commits.

466eae3e1be2eb902bbc228a7b4125e4677cc5b8  Merge remote-tracking branch 'upstream/master' into bencebeky-qpack-minor-editorial
6d0da2315f5c8fbb2321389a92ddbdaf0fe8a652  indexed header field representation
8e11662efb954f27c8f350780b48964e614386d4  eviction when adding a new entry

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942/files/d29333ebc1ac11a02c5835d821a1d8cc5a939e9a..8e11662efb954f27c8f350780b48964e614386d4

---

From: afrind <[email-masked]>
Date: Wed, 16 Oct 2019 09:36:22 -0700

> @@ -477,28 +479,27 @@ it can choose to use a lower dynamic table capacity (see
 
 For clients using 0-RTT data in HTTP/3, the server's maximum table capacity is
 the remembered value of the setting, or zero if the value was not previously
-sent.  When the client's 0-RTT value of the SETTING is 0, the server MAY set it

Yeah, I guess zero pairs better with non-zero?  I don't think it makes a huge difference either way.

Ah, right, N=1 doesn't make sense.  2 and 8 inclusive?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r335584122

---

From: Bence Béky <[email-masked]>
Date: Wed, 16 Oct 2019 11:04:04 -0700

@bencebeky pushed 1 commit.

11edccf6ed3697ba785578712b54a42051bb090d  Add word inclusive.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942/files/8e11662efb954f27c8f350780b48964e614386d4..11edccf6ed3697ba785578712b54a42051bb090d

---

From: Bence Béky <[email-masked]>
Date: Wed, 16 Oct 2019 11:05:48 -0700

Okay.  I guess it is the word "inclusive" that you are missing, so I'm adding it.

It is interesting (but irrelevant) to note that RFC7541 5.1 does not explicitly include the word "inclusive", though the next sentence implies that 8 is allowed:
"The prefix size, N, is always between 1 and 8 bits. An integer starting at an octet boundary will have an 8-bit prefix."

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r335629459

---

From: Bence Béky <[email-masked]>
Date: Wed, 16 Oct 2019 11:08:02 -0700

Thanks for chiming in.  In that case I'm keeping this change.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r335630528

---

From: afrind <[email-masked]>
Date: Mon, 21 Oct 2019 08:52:41 -0700

Merged #2942 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#event-2730234104

---

From: Dmitri Tikhonov <[email-masked]>
Date: Mon, 21 Oct 2019 11:41:46 -0700

dtikhonov commented on this pull request.

> @@ -587,8 +585,10 @@ d = count of entries dropped
 ### Prefixed Integers
 
 The prefixed integer from Section 5.1 of [RFC7541] is used heavily throughout
-this document.  The format from [RFC7541] is used unmodified.  QPACK
-implementations MUST be able to decode integers up to 62 bits long.
+this document.  The format from [RFC7541] is used unmodified.  Note, however,
+that QPACK uses some prefix sizes not actually used in HPACK.
+
+QPACK implementations MUST be able to decode integers up to 62 bits long.

As long as we are being pedantic: "up to *and including*?"

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#pullrequestreview-304769794

---

From: Bence Béky <[email-masked]>
Date: Wed, 23 Oct 2019 06:02:01 -0700

### Indexed Header Field

Created issue #3133 as the "another consistency pass".

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r338032473

---

From: Bence Béky <[email-masked]>
Date: Wed, 23 Oct 2019 06:02:43 -0700

### Prefixed Integers

Created issue #3134   

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2942#discussion_r338032828