-
Notifications
You must be signed in to change notification settings - Fork 205
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
Maximal change for HTTP integer encoding #888
Conversation
This didn't turn out to be that disruptive. I'm somewhat worried that I missed something actually. Given that the changes are small, I think that I would prefer this over #887 for #877. The most interesting change here is that extension frame types that mention Stream IDs will have to change. I am not aware of any proposed thus far, so maybe it's not too much of a cost to bear. It further confirms my view that we shouldn't be using Stream IDs at this layer anyway. Based on this, I don't see any need for a middle-ground change. The biggest cost here is in taking the larger Stream ID space, not in taking the integer encoding.
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.
Some small nits, but looks reasonable to me.
@@ -569,8 +578,9 @@ that is being cancelled (see {{frame-push-promise}}). | |||
If the client receives a CANCEL_PUSH frame, that frame might identify a Push ID |
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.
There's a reference to a 32 bit Push ID three lines above this one. I think that needs to be updated as well?
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.
Actually, there were two, thanks.
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.
Or three; I missed your example, but fixed two others...
draft-ietf-quic-http.md
Outdated
HTTP/QUIC permits use of a larger number of streams (2^62-1) then HTTP/2. The | ||
considerations about exhaustion of stream identifier space apply, though the | ||
space is significantly larger such that it is likely that other limits in QUIC | ||
are reached first, such asthe limit on the connection flow control window. |
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.
asthe -> as the
: A 32-bit identifier for the server push request. A push ID is used in push | ||
stream header ({{server-push}}), CANCEL_PUSH frames ({{frame-cancel-push}}), | ||
and PRIORITY frames ({{frame-priority}}). | ||
: A variable-length integer that identifies the server push request. A push ID |
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.
It'd be nice if we didn't have to call it a variable length integer everywhere. The encoding is variable length, but I think of the integer being encoded as having a fixed length of 62 bits. I'm not sure it's better to say a 62 bit integer or not.
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 think that this is OK. XXX-bit and variable-length occupy the same mental space and the later is more correct. Substitute 62-bit if that works for you, but I don't want to have to explain that 62-bit means the variable-length encoding.
This didn't turn out to be that disruptive. I'm somewhat worried that I missed
something actually. Given that the changes are small, I think that I would
prefer this over #887 for #877.
The most interesting change here is that extension frame types that mention
Stream IDs will have to change. I am not aware of any proposed thus far, so
maybe it's not too much of a cost to bear. It further confirms my view that we
shouldn't be using Stream IDs at this layer anyway.
Based on this, I don't see any need for a middle-ground change. The biggest
cost here is in taking the larger Stream ID space, not in taking the integer
encoding. Worse, I just noticed that PRIORITY would need a lot more work to
split if we only took the change for Stream ID, so the difference would be tiny.