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

Alternative Largest Reference Decoding algorithm. #2248

Closed
wants to merge 1 commit into from
Closed

Alternative Largest Reference Decoding algorithm. #2248

wants to merge 1 commit into from

Conversation

bencebeky
Copy link
Contributor

A Largest Reference Decoding algorithm with two fewer branches (but one more modulo operation).

@bencebeky
Copy link
Contributor Author

This algorithm should be safe from overflow and underflow when using signed arithmetic, as long as the same width integers are used for SETTINGS_TABLE_SIZE and Largest Reference on the wire and TotalInsertionCount, because MaxEntries is bounded from above by the maximum representable number divided by 32.

When using unsigned integers, one can add 3 * MaxEntries instead of 1 * Maxentries in the first line to avoid underflow in the second line, and can break up the last line into two, checking before doing the subtraction. A sane decoder would need to do some checking anyway to make sure that the resulting Largest Reference is at least 1.

@martinthomson martinthomson added editorial An issue that does not affect the design of the protocol; does not require consensus. -qpack labels Dec 24, 2018
@martinthomson
Copy link
Member

The problem with code like this is that it is indecipherable. It's short, but I don't think that it improves readability much. Not that this code is ever readable...

@bencebeky
Copy link
Contributor Author

The problem with code like this is that it is indecipherable. It's short, but I don't think that it improves readability much. Not that this code is ever readable...

I appreciate your concern. Indeed if one is focused on the mechanics of how the decoder or Largest Reference wraps around and what that does to different quantities, the current code is easier to read.

On the other hand, if you focus on the requirement that "Largest Reference is the unique integer that is congruent with wire Largest Reference - 1 modulo 2 * MaxEntries and falls in the interval [TotalNumberOfInserts - MaxEntries, TotalNumberOfInserts + MaxEntries)", then the proposed code is easier to read.

So I believe readability is largely dependent on the mental model that readers have in their heads.

@bencebeky
Copy link
Contributor Author

Since this PR is not clearly an improvement, and it did not gain much traction, I am abandoning it so that we can focus on more important issues.

@bencebeky bencebeky closed this Jan 23, 2019
@bencebeky bencebeky deleted the bnc-largestreferencedecoder-alternative branch January 23, 2019 02:20
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.

None yet

2 participants