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

Pluggable congestion control #776

Open
isaachess opened this issue Jul 31, 2017 · 4 comments
Open

Pluggable congestion control #776

isaachess opened this issue Jul 31, 2017 · 4 comments

Comments

@isaachess
Copy link

Right now this package uses an implementation of TCP-cubic for congestion control. It would be nice if the code easily allowed different congestion control algos to be used.

For example, if I wanted to use a LEDBAT congestion control algorithm, if I were to write my own implementation of the SendAlgorithm interface I still don't see a way to plug that in when I create a new session.

So it would be nice, even if we do not yet have officially supported congestion control other than cubic, to have an easy way to write one and use it instead of cubic.

@isaachess
Copy link
Author

The reason I'm asking this is specifically for LEDBAT. We need the ability to use cubic cc for some connections, but LEDBAT for others. So if there is already a simple way to do that with this QUIC implementation I'd be interested in that.

@lucas-clemente
Copy link
Member

I think the options are either having an enum for the congestion controllers we support (which wouldn't allow it to be different on a connection basis), or adding a func (connectionID int64) SendAlgorithm to the quic.Config.

We also have plans to support BBR (#341), and for that we'll need to change the SendAlgorithm interface – so there will be breaking changes at some point.

If you plan to do benchmarks please be aware that some performance-relevant parts of our implementation are still lacking (e.g. loss recovery).

@isaachess
Copy link
Author

I think having different congestion-control per-connection is not really what I need (sorry for being imprecise), but rather being able to change it per-process.

And yes, I agree having an enum for officially supported congestion controllers is a fantastic idea. I just think even now, before you get around to officially supporting a bunch of different congestion controllers, the code could be rewritten a little to make it simpler to write your own congestion controller and plug it into the session.

@lucas-clemente
Copy link
Member

SGTM :) If forking is an acceptable solution for now, the relevant line you need to change is https://github.com/lucas-clemente/quic-go/blob/master/ackhandler/sent_packet_handler.go#L69.

Feel free to send PRs to simplify things wherever you think it makes sense – what specifically did you have in mind?

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

No branches or pull requests

2 participants