Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[qpack] Overhaul the pseudocode #3577

Merged
merged 3 commits into from
May 19, 2020
Merged

[qpack] Overhaul the pseudocode #3577

merged 3 commits into from
May 19, 2020

Conversation

afrind
Copy link
Contributor

@afrind afrind commented Apr 13, 2020

  • 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

* 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
@ianswett ianswett requested a review from bencebeky April 13, 2020 23:00
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thanks for doing the cleanup.

staticIdx = staticTable.getIndex(header)
if staticIdx:
encodeIndexReference(streamBuffer, staticIdx)
staticIndex = staticTable.getIndex(header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
staticIndex = staticTable.getIndex(header)
staticIndex = staticTable.findIndex(header)

This is a search, right?

continue

dynamicIdx = dynamicTable.getIndex(header)
if !dynamicIdx:
dynamicIndex = dynamicTable.getIndex(header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dynamicIndex = dynamicTable.getIndex(header)
dynamicIndex = dynamicTable.findIndex(header)

# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(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.

Comment on lines 1329 to 1330
dynamicTable.add(header)
dynamicIdx = dynamicTable.baseIndex
dynamicIndex = dynamicTable.getInsertCount()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kazu-yamamoto
Copy link
Contributor

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

@martinthomson martinthomson added -qpack editorial An issue that does not affect the design of the protocol; does not require consensus. labels Apr 15, 2020
Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 1322 to 1323
# getNameIndex attempts to find an index for this header's name.
# If one exists, it returns the name's (absolute) index and a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# getNameIndex attempts to find an index for this header's name.
# If one exists, it returns the name's (absolute) index and a
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
encodeInsert(encoderBuffer, nameIndex, isStaticName, header)
encodeInsert(
encoderBuffer, nameIndex, isStaticName, header
)

Comment on lines 1353 to 1354
wireRIC =
(requiredInsertCount % (2 * getMaxEntries(maxTableCapacity))) + 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@afrind afrind requested a review from MikeBishop May 19, 2020 22:38
@afrind afrind merged commit 402cd86 into master May 19, 2020
@afrind afrind deleted the qpack-pseudocode branch May 19, 2020 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QPACK Appendix B: nameIndex
4 participants