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

make payload length 0 special #1301

Closed

Conversation

marten-seemann
Copy link
Contributor

0 is currently a valid value for the payload length, but sending a packet with no payload is not allowed. By making this value special, we can give it a clearly defined meaning, and save a byte in the common case (and potentially make implementations easier).

@mikkelfj
Copy link
Contributor

Would't it be even more efficient with a header flag, if there are any bits left? There are many more invalid values than just zero, e.g. all longer than UDP size.

@kazuho
Copy link
Member

kazuho commented Apr 18, 2018

I prefer not making 0 special for two reasons.

Firstly, I am not sure if it simplifies things. While I can see that it makes the encoding side slightly simpler for some implementations, I would argue that it makes the decoding side more complex since you need to special-case 0. Note that any value below AEAD tag length + 1 are invalid, so special-casing 0 just adds another if on the decoding side.

Secondly, I prefer just having one canonicalized form rather than having two ways to represent the same thing.

@mikkelfj

Would't it be even more efficient with a header flag, if there are any bits left?

FWIW, we discussed that in #1262 and it seemed that we preferred not having different packet types for those that have payload lengths fields. You might want to look at the comments.

@MikeBishop
Copy link
Contributor

Can I just say again how much I dislike sentinel values?

@larseggert
Copy link
Member

larseggert commented May 4, 2018

I'm with @marten-seemann - I think this would be useful. When encoding, either you need to make the varint fixed-length (so you can encode it after laying out the packet), which can waste a byte, or you may need to occasionally memmove, or you need to pre-calculate the length of your frames, which wastes cycles. Plus, even implementations that will never coalesce packets will always need to do this, which seems backward.

IMO the benefits outweigh the minimal added complexity on decode.

@janaiyengar
Copy link
Contributor

I agree with Lars's point that implementations that never coalesce will have to compute this. That is an unnecessary cost... and we expect the common case to be non-coalesced packets. In optimizing for the common case, I am in favor of this PR.

@kazuho
Copy link
Member

kazuho commented May 4, 2018

@larseggert:

I think this would be useful. When encoding, either you need to make the varint fixed-length (so you can encode it after laying out the packet), which can waste a byte, or you may need to occasionally memmove, or you need to pre-calculate the length of your frames, which wastes cycles. Plus, even implementations that will never coalesce packets will always need to do this, which seems backward.

While I can understand the argument, I do not think that the solution proposed by this PR is optimal.

In my view, the claim is that some implementations do not want to always spend 2 octets on expressing the payload length (note: you can express any payload size using a 2-octet varint expression).

If that is the case, why go for using 1 octet?

I would rather suggest having different packet types for packets that carry the payload length (e.g., Initial-with-Length, Handshake-with-Length) so that implementations not using coalesced packets wouldn't spend any extra octet, or keep the packet design as-is.

@larseggert
Copy link
Member

That’s fine too

@kazuho kazuho mentioned this pull request May 8, 2018
@janaiyengar
Copy link
Contributor

@marten-seemann : Can you raise this on the list and discuss? We should move this conversation forward, but I don't think this PR is the place to discuss.

@martinthomson
Copy link
Member

Closing based on list feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants