-
Notifications
You must be signed in to change notification settings - Fork 204
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
Implicit acknowledgment via header block acknowledgment #1399
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change, the encoder simply kept an "Acknowledged" index that started at zero and incremented by the specified amount each time it received a TSS.
Now it needs to keep two things: "Explicitly Acknowledged" and "Acknowledged". When you receive a TSS, you increment Explicitly Acknowledged as before. The actual Acknowledged state is the largest-seen value of Explicitly Acknowledged or an acknowledged Largest Reference.
We probably need to explicitly say that these implicit acknowledgements don't affect that the number in TSS is still since the last TSS. (Unless we want the decoder to track what the encoder will have gathered from its acks, in which case it could.)
draft-ietf-quic-qpack.md
Outdated
An encoder MUST treat receipt of a Header Acknowledgment as also acknowledging | ||
any dynamic table entries that the header block referenced. That is, this | ||
instruction is also processed as a Table Size Synchronize instruction with a | ||
value matching the Largest Reference of the corresponding header block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, because TSS is a count of the inserts received since the last TSS, instead of an absolute index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, really? That's more efficient, I agree, but I completely missed that. (My fault, not the spec.) Since these are on the same stream, I think that we can make TSS incremental and avoid having two counters.
Now I have to fix my code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean TSS will be incremental from the largest acknowledged largest reference or most recent TSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. That way you maintain a single value for largest acknowledged entry and that is updated by TSS and header acknowledgment. It's simpler and ensures correctness more directly.
draft-ietf-quic-qpack.md
Outdated
A decoder MAY coalesce multiple synchronization updates into a single update. A | ||
decoder MAY rely on Header Acknowledgement instructions to indirectly | ||
acknowledge changes to the dynamic table. Relying on Header Acknowledgment | ||
instructions exclusively leads to blocking if the encoder waits for an entry to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it would lead to blocking, but it would lead to never using the dynamic table?
draft-ietf-quic-qpack.md
Outdated
An encoder MUST treat receipt of a Header Acknowledgment as also acknowledging | ||
any dynamic table entries that the header block referenced. That is, this | ||
instruction is also processed as a Table Size Synchronize instruction with a | ||
value matching the Largest Reference of the corresponding header block. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean TSS will be incremental from the largest acknowledged largest reference or most recent TSS?
draft-ietf-quic-qpack.md
Outdated
instructions exclusively leads to blocking if the encoder waits for an entry to | ||
be acknowledged before using it. This happens if the encoder wants to avoid the | ||
risk of blocking at the decoder, or the decoder sets the | ||
SETTINGS_QPACK_BLOCKED_STREAMS lower than the number of active streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to read this many times to figure out what you are saying - you are explaining why the encoder might be waiting for acknowledgement before using the headers in the dynamic table. I think it might be more clear if you change the second bit to something like
"... or the encoder has already reached the decoder's limit for blocked streams".
draft-ietf-quic-qpack.md
Outdated
A decoder MAY coalesce multiple synchronization updates into a single update. | ||
|
||
A decoder MAY coalesce multiple synchronization updates into a single update. A | ||
decoder MAY rely on Header Acknowledgement instructions to indirectly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the meaning of TSS is changing, then it's no longer a MAY for the decoder to let Header Acknowledgement instructions acknowledge changes. The decoder MUST track the encoder's perspective of what has been acknowledged, and SHOULD send TSS in addition if entries have been added that aren't referenced.
I think this discussion no longer belongs inside the TSS instruction. If the tracking of decoder state involves the encoder combining information from HA and TSS, and the decoder has to track the encoder's inferences from HA to generate TSS, this is a complex enough system to deserve its own subsection. Then TSS simply says that it advances the greatest acknowledged index by however many entries, and references that subsection. |
OK, that's not going to happen before I leave. Let's continue next week. |
draft-ietf-quic-qpack.md
Outdated
@@ -523,6 +532,16 @@ blocks within a stream have been fully processed. | |||
~~~~~~~~~~ | |||
{:#fig-header-ack title="Header Acknowledgement"} | |||
|
|||
An encoder MUST treat receipt of a Header Acknowledgment as also acknowledging | |||
any dynamic table entries that the header block referenced. That is, this | |||
instruction is also processed as a Table Size Synchronize instruction with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Table State Synchronize. It's wrong in the figure from that section also.
State Synchronize, encoded as a 7-bit prefix integer. The encoder uses this | ||
value to determine which table entries are vulnerable to head-of-line blocking. | ||
A decoder MAY coalesce multiple synchronization updates into a single update. | ||
State Synchronize or Header Acknowledgement that increased the largest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the section here makes me think perhaps having Header Acknowledgement described before TSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I want to do that later.
I'm sure that this could be better, but here you go.
Closes #1370.