Subject: [quicwg/base-drafts] [qpack] Overhaul the pseudocode (#3577)

From: afrind <[email-masked]>
Date: Mon, 13 Apr 2020 15:58:17 -0700

* Use the current terminology (insert count, base, encoder stream)
* Update to current design (modulo encoding of RIC)
* Normalize naming: index vs. idx
* Comments

Fixes #3544
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/3577

-- Commit Summary --

  * [qpack] Overhaul the pseudocode

-- File Changes --

    M draft-ietf-quic-qpack.md (68)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/3577.patch
https://github.com/quicwg/base-drafts/pull/3577.diff

-- 
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/3577

---

From: Martin Thomson <[email-masked]>
Date: Mon, 13 Apr 2020 18:32:44 -0700

@martinthomson commented on this pull request.

I couldn't see any problems, but I have some suggestions.

Thanks for doing the cleanup.

>  for header in headers:
-  staticIdx = staticTable.getIndex(header)
-  if staticIdx:
-    encodeIndexReference(streamBuffer, staticIdx)
+  staticIndex = staticTable.getIndex(header)

```suggestion
  staticIndex = staticTable.findIndex(header)
```

This is a search, right?

>      continue
 
-  dynamicIdx = dynamicTable.getIndex(header)
-  if !dynamicIdx:
+  dynamicIndex = dynamicTable.getIndex(header)

```suggestion
  dynamicIndex = dynamicTable.findIndex(header)
```

>      # No matching entry.  Either insert+index or encode literal
-    nameIdx = getNameIndex(header)
+
+    # getNameIndex attempts to find an index for this header's name.
+    # If one exists, it returns the name's (absolute) index and a
+    # boolean indicating which table has the name.  If the name
+    # can't be found, nameIndex is None
+    nameIndex, isStaticName = getNameIndex(header.name)

This is a little strange, as you are relying on functions on the tables to find the full entry, but you jump to a different approach for looking up the name.  I realize that two lookups are worse than one, but it's an odd stylistic jump, that's all.

```python
# An alternative might be to do the name lookup like this:
staticNameIndex = staticTable.findName(header.name)
if staticNameIndex is None:
    dynamicNameIndex = dynamicTable.findName(header.name)
if header.canIndex() and dynamicTable.canAdd(header):
    encodeInsert(encoderBuffer, staticNameIndex, dynamicNameIndex, header)
    dynamicIndex = dynamicTable.add(header)

    # For a bonus, continuing from the above, forget about the
    # newly inserted dynamic table entry if we are at the limit of blocked streams.
    if numBlockedStreams >= maxBlockedStreams:
        dynamicIndex = None
    else:
        addedBlockingEntry = True
        # Later, you can increase numBlockedStreams, but you only do that once.
```

>        dynamicTable.add(header)
-      dynamicIdx = dynamicTable.baseIndex
+      dynamicIndex = dynamicTable.getInsertCount()

I can't suggest here, but I think that you just want this:

```python
        dynamicIndex = dynamicTable.add(header)
```

Otherwise you are breaking encapsulation in a small way: relying on the insert count for getting the index of the thing you just added.

-- 
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/3577#pullrequestreview-392540915

---

From: Kazu Yamamoto <[email-masked]>
Date: Tue, 14 Apr 2020 17:50:46 -0700

This pseudo code is much better! But I would like to support @martinthomson's comments, too.

-- 
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/3577#issuecomment-613753366

---

From: Mike Bishop <[email-masked]>
Date: Thu, 16 Apr 2020 13:42:56 -0700

@MikeBishop requested changes on this pull request.

Looks good.  Your build errors are due to line-length; I think this should fix them.

```suggestion
    # getNameIndex searches for this header's name in the table.
    # If an entry matches, it returns the (absolute) index and a
```

>      if shouldIndex(header) and dynamicTable.canIndex(header):
-      encodeLiteralWithIncrementalIndex(controlBuffer, nameIdx,
-                                        header)
+      encodeInsert(encoderBuffer, nameIndex, isStaticName, header)

```suggestion
    encodeInsert(
        encoderBuffer, nameIndex, isStaticName, header
    )
```

> +  wireRIC =
+    (requiredInsertCount % (2 * getMaxEntries(maxTableCapacity))) + 1;

```suggestion
  wireRIC = (
    requiredInsertCount
    % (2 * getMaxEntries(maxTableCapacity))
  ) + 1
```

-- 
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/3577#pullrequestreview-394978073