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

Allow congestion window to grow even when the window is not completely full and determine window utilization in on_packet_sent #474

Merged
merged 11 commits into from
Feb 3, 2021

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Feb 1, 2021

Issue: #458

To address the congestion window growing too slowly (as detailed in #458), this change introduces two improvements:

  1. The definition of "under-utilized" is more lenient in the slow start phase, where as long as half the congestion window is being used, the window is allowed to grow. In the congestion avoidance state, as long there is less than 3 * MTU of window available, the window is allowed to grow.
  2. The determination of "under-utilized" is made in on_packet_sent instead of on_packet_ack, and is stored in the congestion controller itself. This allows for a burst of acks to grow the window, even if the application is not able to send data during the processing of those acks.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@WesleyRosenblum WesleyRosenblum marked this pull request as draft February 1, 2021 20:06
@WesleyRosenblum WesleyRosenblum marked this pull request as ready for review February 2, 2021 00:20
@WesleyRosenblum
Copy link
Contributor Author

WesleyRosenblum commented Feb 2, 2021

Thought of another approach: Check is_congestion_window_under_utilized() in the on_packet_sent method and store the result on the CubicCongestionController. This value would then be checked in on_packet_ack. Since the change from under_utilized to utilized occurs before the congestion controller prevents sending, I believe this would work, since as long as the application is able to send data, under_utilized should flip to false and on_packet_ack would be allowed to grow the window.

Here is a Gist containing this approach: https://gist.github.com/WesleyRosenblum/6e74c9944e3e27405d047bdf669e62c2

Edit: I've gone ahead and committed this approach. The difference between this approach and the previous RTT based approach can be seen here

@WesleyRosenblum WesleyRosenblum changed the title Allow congestion window to increase for 1 RTT past when it was last considered utilized Allow congestion window to grow even when the window is not completely full and determine window utilization in on_packet_sent Feb 3, 2021
camshaft
camshaft previously approved these changes Feb 3, 2021
/// without further evidence of the stability of the current window.
fn is_congestion_window_under_utilized(&self) -> bool {
// This value is based on kMaxBurstBytes from Chromium
// https://source.chromium.org/chromium/chromium/src/+/master:net/third_party/quiche/src/quic/core/congestion_control/tcp_cubic_sender_bytes.cc;l=23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while cwnd >= MINIMUM_MTU as u32 {
congestion_controller.on_packet_sent(timestamp, bytes);

let ack_receive_time = timestamp + rtt_estimator.min_rtt() / 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this actually affects the results but wouldn't it just be the RTT (without / 2)? Like if i send a packet and get an ack back that would be 1 RTT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep

Comment on lines +11 to +21
1: cwnd: 25200,
2: cwnd: 51600,
3: cwnd: 104400,
4: cwnd: 210000,
5: cwnd: 421200,
6: cwnd: 843600,
7: cwnd: 1688400,
8: cwnd: 3378000,
9: cwnd: 6757200,
10: cwnd: 13515600,
11: cwnd: 27032400,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better!

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.

None yet

2 participants