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

Mike Bishop <notifications@github.com> Fri, 16 August 2019 19:00 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id D25DA1208E4 for <quic-issues@ietfa.amsl.com>; Fri, 16 Aug 2019 12:00:43 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -8
X-Spam-Level:
X-Spam-Status: No, score=-8 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7ijloVK4jzKy for <quic-issues@ietfa.amsl.com>; Fri, 16 Aug 2019 12:00:38 -0700 (PDT)
Received: from out-24.smtp.github.com (out-24.smtp.github.com [192.30.252.207]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id EE26512088F for <quic-issues@ietf.org>; Fri, 16 Aug 2019 12:00:37 -0700 (PDT)
Date: Fri, 16 Aug 2019 12:00:37 -0700
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1565982037; bh=FfX/wqTUV/ZNLZ/0L7oEp0QH5GJfLuK+CXAYcMdsTfc=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=SaER6/+el+aQiKoBqRZzr62dKpHTeer8JqvPtS81O8haIr2lEtvV2t9K6iE+CdUU4 7vzvFtwDzllbaCaIkI6epTjrt84IXX/RKYrXxxrQSYEpmXrYNf7OKjZFHAu4H+jfRl QnAHel6IeYVOLhUw9mwmLBOD22buiZqAfVNeYBwQ=
From: Mike Bishop <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+AFTOJK5J26AK2LQVJCDJZ3V3MQX5LEVBNHHBYU6MYI@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/2942/review/276111748@github.com>
In-Reply-To: <quicwg/base-drafts/pull/2942@github.com>
References: <quicwg/base-drafts/pull/2942@github.com>
Subject: Re: [quicwg/base-drafts] QPACK [editorial] Misc minor editorial changes. (#2942)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5d56fd558b0b_14b93f8c15acd95c333030"; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: MikeBishop
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/HbghbbsV4hs4UcZqpSiCIX3sTfg>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 16 Aug 2019 19:00:50 -0000

MikeBishop approved this pull request.

A few nitpicks, but these are all good improvements.  Thanks.

>  table entries which have been acknowledged, but this could mean using
 literals. Since literals make the header block larger, this can result in the
 encoder becoming blocked on congestion or flow control limits.
 
 ### Known Received Count
 
-In order to identify which dynamic table entries can be safely used without a
-stream becoming blocked, the encoder tracks the number of entries received by
-the decoder.  The Known Received Count tracks the total number of acknowledged
-insertions.
+The Known Received Count is the total number of dynamic table insertions and
+duplications acknowledged by the decoder.  The encoder tracks the Known Received
+Count in order to identify which dynamic table entries can be references without

```suggestion
Count in order to identify which dynamic table entries can be referenced without
```

> @@ -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

Well, if you're running close to the table's maximum size, presumably that's an indicator that the other side doesn't ever want the table larger than that, which implies evict-then-add.  However, this is also a holdover from HPACK where adding something larger than the max table size evicted everything, then left you with an empty table when the add failed.  Since that's now a hard error, I'm comfortable framing this as purely implementation choice.

> @@ -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
+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
+new entry can reference an entry in the dynamic table that will be evicted when
+adding this new entry.  Therefore implementations utilizing the latter option

```suggestion
adding this new entry.  Therefore, implementations utilizing the latter option
```

> @@ -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

The usual rule is to write out numbers less than ten.  However, I'm okay either way when we're talking about the literal value of something.  I defer to @afrind's style preferences.

>  
-An endpoint MUST allow its peer to create both encoder and decoder streams

Would "an encoder stream and a decoder stream" make this clearer?

> @@ -830,10 +831,8 @@ modifying it.
 
 Each header block is prefixed with two integers.  The Required Insert Count is
 encoded as an integer with an 8-bit prefix after the encoding described in
-{{ric}}).  The Base is encoded as sign-and-modulus integer, using a single sign
-bit and a value with a 7-bit prefix (see {{base}}).
-
-These two values are followed by representations for compressed headers.
+{{ric}}).  The Base is encoded as a sign bit, and a Delta Base value with a

```suggestion
{{ric}}).  The Base is encoded as a sign bit and a Delta Base value with a
```
Comma makes it looks like Delta Base follows the sign-bit-encoded Base.

-- 
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-276111748