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

Consider making long header payload length 16bits #1577

Closed
DavidSchinazi opened this issue Jul 17, 2018 · 17 comments
Closed

Consider making long header payload length 16bits #1577

DavidSchinazi opened this issue Jul 17, 2018 · 17 comments
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.

Comments

@DavidSchinazi
Copy link
Contributor

Currently using variable-length encoding for the long header payload length complicates implementations because the payload length is not necessarily known at the start of payload assembly. One solution is to have a separate buffer and/or copy the payload when complete. What several implementations end up doing is to always use the two-byte variant of variable length encoding for all values 0-16383. Making this a 16bit field would allow all values 0-65535 and simplify implementations.

@mikkelfj
Copy link
Contributor

I agree

@marten-seemann
Copy link
Contributor

You need to know the length you're about to write in order to not overflow the maximum packet size. I had no problem whatsoever using the smallest possible varint representation for the payload length.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -transport labels Jul 17, 2018
@martinthomson
Copy link
Member

Can't you just use the 16-bit encoding always? You can encode small values on 2, 4, or 8 octets if necessary.

@DavidSchinazi
Copy link
Contributor Author

You absolutely can do that, but it might make implementor's lives easier to use a fixed-width encoding here.

@larseggert
Copy link
Member

We discussed something similar in #1301

@janaiyengar
Copy link
Contributor

+1. I'm noticing also that everyone is using a 2-byte PN, I think it makes sense to just use that. Varlen is overkill.

@kazuho
Copy link
Member

kazuho commented Jul 18, 2018

I oppose.

We currently have a consistency is using varint type for expressing offsets, lengths. Using int16 just for this length type destroys that consistency, thereby makes the design unnecessary complex.

For the encoder side, most people not interested in saving one octet can always use a two-octet encoding.
For the decoder side, people can use their varint decoder, that they have been consistently using for decoding lengths.

@dtikhonov
Copy link
Member

I agree with @kazuho, for the same reasons.

@nibanks
Copy link
Member

nibanks commented Jul 18, 2018

I disagree that changing the length to a 2-byte field increases complexity. Maybe things aren't quite as consistent, but the code to read the field isn't going to drastically increase in complexity because of it. It would likely get a tad bit simpler too. Additionally, I don't think we need to worry about allowing for the possibility that someone really wants to save a single byte in a long header (only during the handshake).

@dtikhonov
Copy link
Member

@nibanks, I think that this sort of issue is "six one way, half dozen the other." I think we should avoid making changes of this sort lest there be a good reason. As it stands, it is trivial to both write and read the current varint encoding.

With regard to expressible range: For packet sizes larger than 16K, it is permissible to splurge and encode their length using 4 bytes.

@nibanks
Copy link
Member

nibanks commented Jul 18, 2018

Actually, @dtikhonov I think the expressible range maybe the biggest non-bikeshed reason to use a 2-byte field. Everyone is always using 2 bytes because they build up the contents first (because they don't know the final length upfront) and then go back and fill in those two bytes they preallocated essentially. But if a complete solution wants to support maximum 64K UDP payload sizes, that logic gets a lot more complicated. Either they always preallocate 4 bytes or they have to precalculate the size, which is hard enough that nearly all implementations don't do that already.

@marten-seemann
Copy link
Contributor

marten-seemann commented Jul 18, 2018

I agree with @kazuho and @dtikhonov here. There's value in consistency, and we changed the encoding of values just for the sake of consistency before (last time for the ODCIL field in the Retry packet).

@dtikhonov
Copy link
Member

@nibanks, you are making a good point. I withdraw my objection.

@kazuho
Copy link
Member

kazuho commented Jul 19, 2018

@nibanks That argument stands for many other fields. Initial.token_len, Stream.length, ... There are some fields that never becomes greater than 32-bits (i.e. ACK block size).

We can try to have different types for based on each of the requirements. However that causes pain, because we cannot reuse code. That was the pain we had, until we introduced variable integers in #877.

I do not think we need to revert that to reopen tons of issues.

@ianswett
Copy link
Contributor

I'd like to stick with the varint heavy design for QUIC v1. If it turns out to be a pain in practice, let's reconsider for v2.

@mcmanus
Copy link
Contributor

mcmanus commented Jul 19, 2018 via email

@janaiyengar
Copy link
Contributor

I don't have a strong opinion, but @kazuho's point is fair.

Based on the feedback on this issue, I'm going to close it. If you think we ought to continue discussing this point, please feel free to reopen.

@mnot mnot added the has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. label Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list.
Projects
None yet
Development

No branches or pull requests