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

ACK delay & RTT estimation #40

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pietdevaere
Copy link
Contributor

Adding RTT estimation to the congestion controller.

I added both QUIC and TCP-style estimates. The TCP style estimates do not consider the ACK Delay field. They might be useful later to see what the influence is of moving the stamping of the ACK Delay and the recording of the Rx time of packets.

For this I had to populate the ACK Delay field in ACK frames. I currently use the IEEE 754 binary16 half precision format to encode the time. This is not exactly the format from draft-ietf-quic-transport-07, but as that format is under review, implementing it seems a waste of time. I borrowed the code for this from https://github.com/h2so5/half, which is CC0.

@ekr
Copy link
Owner

ekr commented Dec 4, 2017

How different is this from the existing format? I.e., is it going to produce crazy results when interpreted by other stacks?

@pietdevaere
Copy link
Contributor Author

pietdevaere commented Dec 4, 2017

As per -07

The bit format is loosely modeled after IEEE 754

I didn't look at it in detail, but I think the main difference is that it is an unsigned float instead of a signed one.

Considering the crazy results: draft-ietf-quic-recovery specifies a sanity check to make sure that ACK delay > RTT, so if this value is wrong, the worst that can (should?) happen is that the endpoint assumes that the link RTT is 0.

In -08 the format will change completely.

@ekr
Copy link
Owner

ekr commented Dec 19, 2017

This looks basically sound, but given that we are now at -08, can you update the float format?

@pietdevaere
Copy link
Contributor Author

The new format is a varint. Perhaps we should park this PR until varint support is added?

@ekr
Copy link
Owner

ekr commented Dec 22, 2017 via email

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.

2 participants