Support for cubic

[Aperence]

See #402 for the details.

This PR provide support for cubic, and support for selecting the maximum datagram size.

Exemple of usage :

from aioquic.quic.congestion.cubic import CubicCongestionControl
from aioquic.quic.congestion.reno import RenoCongestionControl
from aioquic.quic.congestion.slow_starts import HyStart

if (args.cca == "reno"):
    cca = RenoCongestionControl(max_datagram_size=K_MAX_DATAGRAM_SIZE, callback=callback, slow_start=HyStart(max_datagram_size=K_MAX_DATAGRAM_SIZE))
elif (args.cca == "cubic"):
    cca = CubicCongestionControl(max_datagram_size=K_MAX_DATAGRAM_SIZE, callback=callback, reno_friendly_activated=args.reno_friendly)
else:
    exit(1)

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

[jlaine]

This PR still seems to include unrelated code such as changes to slowstart, minmax.. can we cut it down to the bare minimum to support Cubic please?

[Aperence]

Hi,

First, thanks for your feedback.

I have normally removed all content that weren't related to cubic (as slow-start, minmax, get_dataclass_attr, ...).

Only the code for cubic should remain now.

Best,

[Aperence]

Hello,

I modified the content you pinpointed above, feel free to give me additional feedback if anything is still weird or should be changed :D

Have a nice day,

[jlaine]

OK we're getting close to a mergeable state! Would you mind rebasing on top of main and squashing down all your commits so we can get a CI run?

[Aperence]

Normally this should be done, I squashed all the commits into one (4cf275a)

[jlaine]

Normally this should be done, I squashed all the commits into one (4cf275a)

Yep it's squashed but not rebased on top of main so there is a conflict in src/aioquic/quic/configuration.py

[jlaine]

I've been reading more of the code and I'm not entirely comfortable with how it's architectured. Could you take a look at PR #432 which only moves code around and introduces a base class for congestion control algorithms?

I've had a look at whether the log callback stuff makes sense, but it seems there are no provisions for Cubic-specific metrics in the QLOG specs:

https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events.html#name-recovery-events

Do get in touch with @rmarx, he is usually open to suggestions on how to improve QLOG :)

[rmarx]

Hey both,

(just for the QLOG stuff ;))

qlog is actually intended to be extensible, so implementations can log whatever extra data they care about. We indeed don't want to over-specify for the different CC algorithms, but other implementations have certainly also logged their own fields for that. The one thing you can't count on of course is that general tools (like QVIS) will do anything more with your custom fields than just show them (which is what qvis does).

Looking at the specific fields here, they seem sensible. My suggestion would be to start the fields with cubic- though, and make them lowercase (that IS required by qlog ;)), so cubic-phase or cubic-wmax.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bbe4c4d) 100.00% compared to head (ca0fe88) 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##              main      #415    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           24        25     +1     
  Lines         4728      4840   +112     
==========================================
+ Hits          4728      4840   +112     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Aperence]

Ok, I've reverted normally every changed except for cubic, and logs, I've also run the lint checks locally (ruff + black + mypy, thank you btw because I didn't know about these tools at all) and I fixed normally everything that was listed locally

[Aperence]

My black version locally seem however to differ compared to the one used on CI, as it fails here, I can fix this (removing a blank line), if needed, but I'm waiting for you to confirm as I might have abused you CI instances with all these push, my apologies for this 😢

[jlaine]

It looks as though you still have one linter error left :)

The code is also going to need thorough tests to check everything is working in all scenarios. I would suggest you try writing a test_recovery_cubic.py based on test_recovery_reno.py. Full test coverage is a requirement to merge :)

[jlaine]

This doesn't look needed, we're already reporting ssthresh in QuicCongestionControl.get_log_data()

[Aperence]

Normally, I've fixed these last issues + added a test for cubic based on test_recovery_cubic.py. I'll try to check later the code that wasn't covered by thos tests

[jlaine]

Normally, I've fixed these last issues + added a test for cubic based on test_recovery_cubic.py. I'll try to check later the code that wasn't covered by thos tests

Sure! You can get coverage reports locally too:

pip install coverage
coverage run -m unittest tests/test_recovery_cubic.py
coverage html
firefox htmlcov/index.html

[jlaine]

Wow, amazing work, thanks so much for your patience! Is the test_cubic.py still useful or is it superseded by your test_recovery_cubic.py? If it's still useful maybe merge it into test_recovery_cubic.py?

Disclaimer: I am not able to determine whether the code works as intended, or follows specs. Would you mind taking the code in its current form for a spin and checking it still behaves as you want it to?

Once this is done I'd be happy to merge the code!

[Aperence]

Hi,

First, thank you for all your feedback during this PR, I learned quite a lot about CI tools to ensure some code quality !
Secondly, about the test_cubic.py, it is still useful, so I'll just merge it in test_recovery_cubic.py as you recommended, this should centralize all the tests for the cubic congestion controller.

The implementation seems to still work fine, here are some examples on how it performs:

With no delay on localhost:

With a delay of 100ms on localhost:

We can clearly still see the cubic growth when the delay is quite important (second plot), and the reno friendly region when we have a reasonable delay (first plot), thus I think we can affirm that it still work as intended

[jlaine]

Alright, thanks for the checks! Are these graphs produced from the QLOG output? I saw an article which mentioned you had set up a test bench as part of your University work?

If wouldn't mind squashing your commits down to a single commit with a clean commit message I'd be happy to hit the "merge" button.

[Aperence]

Yup, these graphs are generated based on the QLOG files, the script can be found here if you are interested with it (it support normally any congestion control, as it only check the presence of the cwnd field in the logs), but some features (W_max, ...) might not be up to date with the latest modifications for the logs (but the core for plotting losses and cwnd does). You can also select a particular field if you want to plot other things (for example ssthresh, bytes_in_flight, ...)

Normally, the commits should be squashed also !

[jlaine]

Merged, thanks a lot! 🎉

I hope we will have the pleasure of seing more of your work :)