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

Added several congestion control algorithms #402

Closed
wants to merge 29 commits into from

Conversation

Aperence
Copy link
Contributor

@Aperence Aperence commented Sep 12, 2023

Hi,

I've added the following congestion control algorithms to aioquic : cubic, bbrv1, bbrv2.

Cubic was tested, while bbrv1/2 was only implemented, and verified graphically, but no formal verification was done.

I also ran an interop test with cubic to ensure that the implementation wasn't broken after my modifications, here are the results. Overall, an average gain of 67kbps was observed for crosstraffic, and the implementation seemed to work still fine (Note: quinn seems to have a problem with cubic, but it is surely due to the way they handle the acks that doesn't work well with cubic).

Also I added hystart++ as a choice for the slow start algorithm.

To choose a congestion control algorithm, just fill the parameter in the config with the congestion control you want to use. For example:

from aioquic.quic.congestion.cubic import CubicCongestionControl
from aioquic.quic.congestion.reno import RenoCongestionControl
from aioquic.quic.congestion.bbr import BBRCongestionControl
from aioquic.quic.congestion.bbr2 import BBR2CongestionControl
from aioquic.quic.congestion.slow_starts import HyStart
from aioquic.quic.configuration import QuicConfiguration
import argparse

args = argparse.ArgumentParser().parse_args()

if (args.cca == "reno"):
    cca = RenoCongestionControl(slow_start=HyStart())
elif (args.cca == "cubic"):
    cca = CubicCongestionControl(reno_friendly_activated=args.reno_friendly)
elif (args.cca == "bbr"):
    cca = BBRCongestionControl()
elif (args.cca == "bbr2"):
    cca = BBR2CongestionControl()
else:
    print("invalid cca")
    exit(1)

conf = QuicConfiguration(alpn_protocols=["quic-testing"], is_client=True, congestion_control=cca)

Here are example on how cubic and bbr perform for the growth of cwnd:

  • cubic : cubic
  • bbr : bbr

@jlaine
Copy link
Contributor

jlaine commented Nov 4, 2023

I really appreciate the work that has gone into this! However it's a HUGE pull request, and I think we should focus on adding one new CC method at a time, with extensive unit tests. From what you've observed what would be the best algorithm to add first?

@jlaine jlaine added the changes requested Some changes are required before the PR can be merged. label Nov 4, 2023
@Aperence
Copy link
Contributor Author

Aperence commented Nov 6, 2023

Hi,

From what I have seen, cubic should be the first to be included according to me.

BBR and BBRv2 are mostly experimental, and there isn’t really any tests that can be performed to check if it behaves as it should (at least for the moment).

Cubic seems thus to be the best choice, as it is much simpler, as well as being easier to test (actually, a test was already implemented to see if the growth of the cwnd correspond to the W function of the mathematical model). Apart from that, I didn’t find any test to perform, as this field of research (verification of congestion control) isn’t much developed from what I’ve heard.

@jlaine
Copy link
Contributor

jlaine commented Nov 6, 2023

Great, let's focus on cubic then. Maybe a good way to proceed would be to leave this PR as-is so we don't break it and start a fresh PR which only adds cubic so we can polish it. There is going to be some adjustment needed because the congestion controller now gets handed a max_datagram_size argument instead of relying on the old PACKET_MAX_SIZE.

@Aperence Aperence mentioned this pull request Nov 7, 2023
@Aperence
Copy link
Contributor Author

Aperence commented Nov 7, 2023

I’ve opened a new PR (#415) with the needed changes normally

@jlaine
Copy link
Contributor

jlaine commented Dec 14, 2023

Hi @Aperence, if it's OK with you I suggest we close this PR and if you want to add another algorithm you open a specific PR?

@Aperence
Copy link
Contributor Author

Yes, it seems fine to me 👍

@jlaine jlaine closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Some changes are required before the PR can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants