Refactor CongestionControl and Reno #420
Conversation
|
TBH this seems to make things quite a lot more complicated and hard to follow (due to all the macros and different traits). I think this can be simplified by using raw vtables (e.g. like Linux does) for the congestion control implementations. If done right this also solves the problem of having to redefine many of the common CC fields and methods for each different CC algorithm. I took a stab at this in #421 (though note that it's still WIP). It seems to work well, and also reduces the amount of code required for each CC algorithm, though I guess the main test would be adding more algorithms to see if the API is really flexible enough. One thing that is missing from that PR is a way for CC implementations to define internal fields without having to add them to Recovery (e.g. CUBIC has a bunch of these), but it's not really required for now and can be added later. |
- Split CongestionControl trait into multiple traits
- Redefine Reno using macro for future reuse
In this way, other congestion control can be simplified
without repeating same function again. mod.rs includes
Reno implementation using macros, so reno.rs is now
very simple.
If other cc algorithm want to use same hook, use one
of impl_cc_*!() for reusing Reno implementation.
It also enables to add a common feature for all CC
algorithms easily.
e.g. if CC Foo is added and want to reuse on_packet_sent() and
congestion_event() but want to define its own on_packet_acked():
pub struct Foo {
...
}
impl cc::CongestionControl for Foo {
...
}
// Reuse Reno hooks
impl_cc_common!(Foo);
impl_cc_on_packet_sent_reno!(Foo);
impl_cc_congestion_event!(Foo);
impl cc::CCOnPacketAcked for Foo {
...
}
Thanks for another solution in #421. Using function pointer table was 1st thing I thought when I started to work on congestion control implementation but tried to avoid it because I still don't think it's a good idea -- there is no clear code/class separation from Recovery and don't want to be C-like code as linux/bsd kernel does. Also, while #421 solves some problems such as accessing Recovery's internal variables from CC algorithm but so far it seems like thing needed from Recovery module is limited, so current approach (adding only when necessary) doesn't make a big problem for me.
Right. When CC algorithm need internal variables (most of them will do), you will need a separate struct for storing internal variables and one more hook for initialization of such internal variables as well. In current approach you simply need to create a one struct per congestion control algorithm with internal variables and its methods together, much simpler I think. One problem of this PR is use of macro_rules. Those are needed mainly due to the Rust limitation which doesn't allow trait to access struct variables. Looks like there is people feeling the same pain such as rust-lang/rfcs#1546, hopefully this will be solved in the future and we can remove macros and implement Reno in a trait's default method and all other CCs can implement their own only. |
Well, that's kind of the whole point. There is no separation because I don't think there can be separation since the CC code needs to access many recovery values (see e.g. the
This is actually a pretty common pattern in Rust too, see e.g.: even the Rust standard library exposes some APIs like that as well, e.g.: But more generally I don't think "being C-like" is a bad thing, if it actually improves code clarity, which I think it does in this case. I do agree with you that if a clean trait API could be implemented we should avoid doing this, but having to split a trait in 5 and have as many macros to define common parts just seems to make things much more complicated than they need to be.
Yeah, but that's just one of the things that it solves, as a side-effect. The main point is that the code is clearer and easier to understand, at least in my opinion.
What's wrong with that? It would still be simpler I think. In any case you could also just add them to
They are not together, they are split in 5 different traits, and also need to use macros extensively to implement common parts of the code (or just straight-up duplicate it). It might be simpler to implement if you understand how the API works, but reviewing these changes, or trying to understand what the code does in 1 year from now, is actually pretty hard.
That was created in 2016, and nobody is working on that. I wouldn't hold my breath waiting for that to be done TBH. But even if that was available now, I'm not really sure it would improve things here as the added macros do much more than simply accessing common fields? Like, I think you'd still have multiple traits. |
I think we start to have a different opinion around here, so I won't argue strongly.
The draft pseudo code implements Reno only... it's up to implementers how to do with additional CCs.
As I commented above, we can remove all separate traits and macros altogether if trait fields is supported. (define a CC trait which has default Reno methods and all other CC will reimplement their own methods as needed. that's all) However it doesn't exist so no need to discuss here. I'll close this PR soon unless further opinion exists from others. |
In this way, other congestion control can be simplified
without repeating same function again. mod.rs includes
Reno implementation using macros, so reno.rs is now
very simple.
If other cc algorithm want to use same hook, use one
of impl_cc_*!() for reusing Reno implementation.
It also enables to add a common feature for all CC
algorithms easily.
e.g. if CC Foo is added and want to reuse on_packet_sent() and
congestion_event() but want to define its own on_packet_acked():