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

Plot a simulation of the congestion controller #468

Merged
merged 16 commits into from
Jan 28, 2021

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Jan 27, 2021

In light of #458, which could have been easily detected with a visual representation of the congestion window, this change introduces a plot of congestion window over time for a particular simulation of the congestion controller. For now, there is just one simple simulation where in each round the full congestion window is sent and acked, but we can add more simulations in the future. In addition, I've added a snapshot test to verify nothing has changed with the output of the congestion controller.

As can be seen in the plot, the congestion window is growing linearly, when it should be growing exponentially while in slow start (see #458)
image

Here is another simulation, this time with Loss experienced at 3MB:
image

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 January 27, 2021 01:16
@camshaft
Copy link
Contributor

camshaft commented Jan 27, 2021

Are all of the functions available in https://crates.io/crates/libm? We might be able to include that when we're in no_std.

Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

I don't see the actual snap file anywhere. Did you forget to commit that?

@alexw91
Copy link

alexw91 commented Jan 27, 2021

Codecov Report

Merging #468 (26863d8) into main (ad15171) will increase coverage by 0.03%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
+ Coverage   62.98%   63.02%   +0.03%     
==========================================
  Files         205      206       +1     
  Lines        8291     8327      +36     
  Branches     2130     2134       +4     
==========================================
+ Hits         5222     5248      +26     
- Misses       1676     1683       +7     
- Partials     1393     1396       +3     
Flag Coverage Δ
unit-tests 63.02% <55.55%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...2n-quic-core/src/recovery/congestion_controller.rs 31.25% <0.00%> (-4.47%) ⬇️
quic/s2n-quic-transport/src/connection/mod.rs 0.00% <ø> (ø)
quic/s2n-quic-transport/src/recovery/mod.rs 0.00% <ø> (ø)
...uic/s2n-quic/src/provider/congestion_controller.rs 0.00% <ø> (ø)
quic/s2n-quic-core/src/recovery/cubic.rs 94.54% <100.00%> (ø)
...ic/s2n-quic-core/src/recovery/hybrid_slow_start.rs 93.10% <100.00%> (ø)
quic/s2n-quic-core/src/packet/encoding.rs 34.48% <0.00%> (-0.61%) ⬇️
quic/s2n-quic-transport/src/space/mod.rs 80.95% <0.00%> (ø)
quic/s2n-quic-core/src/crypto/key.rs 22.22% <0.00%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad15171...26863d8. Read the comment docs.

camshaft
camshaft previously approved these changes Jan 28, 2021
@WesleyRosenblum WesleyRosenblum marked this pull request as ready for review January 28, 2021 06:18
@WesleyRosenblum WesleyRosenblum merged commit 2a469b6 into main Jan 28, 2021
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/congestionplot branch January 28, 2021 17:29
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.

3 participants