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

Martin Thomson <> Tue, 14 April 2020 01:32 UTC

Return-Path: <>
Received: from localhost (localhost []) by (Postfix) with ESMTP id 3AA723A22C4 for <>; Mon, 13 Apr 2020 18:32:47 -0700 (PDT)
X-Virus-Scanned: amavisd-new at
X-Spam-Flag: NO
X-Spam-Score: -3.267
X-Spam-Status: No, score=-3.267 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.168, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: (amavisd-new); dkim=pass (1024-bit key)
Received: from ([]) by localhost ( []) (amavisd-new, port 10024) with ESMTP id 50TuTp89bMtj for <>; Mon, 13 Apr 2020 18:32:45 -0700 (PDT)
Received: from ( []) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by (Postfix) with ESMTPS id 8FA113A22C2 for <>; Mon, 13 Apr 2020 18:32:45 -0700 (PDT)
Received: from ( []) by (Postfix) with ESMTP id A4126520357 for <>; Mon, 13 Apr 2020 18:32:44 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;; s=pf2014; t=1586827964; bh=ppO0mQ7KLrxiH48emjp8TCIBrv3VYNWLT+qIXBKCLhA=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=wGKbjJg8fjlWJeaUOEp5tOtmGRk9Oi1PS9nDZH3Cn25wsNb7wjpjCqTw41qjmUdkA Fgxk1fTwReiMPSfj3zJhsjrUWKRwq/tkbchFmn3lOVgEPFPY57lbxVbW59DxjnpoiZ gMJ38j7qORX5ON1LcxHiLOKjg3owzYma0VpgZw5I=
Date: Mon, 13 Apr 2020 18:32:44 -0700
From: Martin Thomson <>
Reply-To: quicwg/base-drafts <>
To: quicwg/base-drafts <>
Cc: Subscribed <>
Message-ID: <quicwg/base-drafts/pull/3577/review/>
In-Reply-To: <quicwg/base-drafts/pull/>
References: <quicwg/base-drafts/pull/>
Subject: Re: [quicwg/base-drafts] [qpack] Overhaul the pseudocode (#3577)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5e9512bc94814_68f13fbdd22cd95c1710b2"; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: martinthomson
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
Archived-At: <>
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <>
List-Unsubscribe: <>, <>
List-Archive: <>
List-Post: <>
List-Help: <>
List-Subscribe: <>, <>
X-List-Received-Date: Tue, 14 Apr 2020 01:32:47 -0000

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

  staticIndex = staticTable.findIndex(header)

This is a search, right?

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

  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(

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.

# An alternative might be to do the name lookup like this:
staticNameIndex = staticTable.findName(
if staticNameIndex is None:
    dynamicNameIndex = dynamicTable.findName(
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
        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:

        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: