feat(cca): BBR by FrankSpitulski · Pull Request #1151 · quinn-rs/quinn

FrankSpitulski · 2021-06-24T18:20:02Z

implements BBR congestion control based on google's quiche. There are some differences in implementation due to chromium's CCA having a different API, especially around sending vs acking packets.

It's a pretty rough first implementation but from my testing it seems to work well. It is very resistant to packet loss especially in high BDP situations.

Please critique and suggest changes as needed.

I've added additional fields and traits for debugging using stats which can be removed if needed.

Closes #693

Matthias247

Very cool to see BBR is integrateable and to hear it performs good! Definitely interested to see this landing.

I did a short skim on this, but comments are mostly stylistic since I don't have any background on BBR.

Since this is rather complex, I would be interested to see some tests as part of these changes. Ideally also in the form that show the benefits compared ot the other congestion controllers.

Matthias247 · 2021-07-02T04:21:26Z

+        // Compute how many bytes are expected to be delivered, assuming max
+        // bandwidth is correct.
+        let expected_bytes_acked = max_bandwidth
+            * (now - self.bbr_aggregation_epoch_start_time.unwrap_or(now)).as_micros() as u64


you probably want now.saturating_duration_since(...) for safety reasons and conciseness

fixed, but it doesn't look any more concise to me. can you make sure it's what you intended?

Ralith

Thanks, this looks pretty cool! Reviewed mostly for style and API impact for now, since I don't presently have time to dig deep into correctness, and it's not clear what the best reference would be to derive such a review from either.

FrankSpitulski · 2021-08-10T23:59:27Z

@Ralith is this still in the pipeline?

Ralith · 2021-08-12T18:08:30Z

Yeah, sorry, just on limited bandwidth at the moment. I am looking forward to getting this in.

djc · 2021-09-30T07:49:32Z

@FrankSpitulski this is ready for review, right? Mind marking it as such?

FrankSpitulski · 2021-10-20T20:48:51Z

@Ralith I'm not sure what to do about this audit failure after rebasing. It's complaining about a vulnerability in the chrono package, but I haven't touched the deps.

djc · 2021-10-25T08:05:25Z

@FrankSpitulski feel free to ignore that for now, it is irrelevant to this PR.

Ralith

Thanks for your patience, and for keeping this rebased! I don't feel able to review the internal logic here in depth at present, but I'm happy to merge it as non-default with the experimental disclaimer. Reviewed mainly for style/clarity; I think we should be able to merge this shortly.

djc

Also added some style notes. Thanks for all of your work on this!

Ralith

Thanks!

squashed messages: fixup for docs and removing extra braces add downcasting hatch pretty print should be Display, not Debug un-needed export fmt and clippy re-expose CCAs to public API reorder constants inline bbr_state remove extra param refactor bbr into its own module refactor bbr into its own module no threadlocal rng, use genrange rename fields for review Update quinn/src/connection.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> Update quinn-proto/src/connection/mod.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> Update quinn-proto/src/connection/mod.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> fill() now supported clippy in cubic was covered by the rebase add links to BBR info refactor to hide internal PathData clarify BBR usage reduce struct scope collapse update_last_sent into on_sent collapse on_loss into on_congestion_event remove dead code Update quinn/src/connection.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> Update quinn-proto/src/congestion/bbr.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> Update quinn-proto/src/congestion.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> Update quinn-proto/src/congestion/cubic.rs Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com> clippy safer duration comparison backwards compatibility fill add docs, rename stats cca to congestion remove PacketNumber and bbr_ prefixes rename constants to rust style fix packet number typing enable drain to target bdp mode during probe bw add ack aggregation for better target growth fix not entering growth mode remove extra debug printing enable rtt probe and track min rtt track max bandwidth with minmax filter use minmax filter for max bandwidth estimation switch minmax filter to use round counts instead of times port minmax filter to rust fix: count lost packet size instead of their id fix bw estimation not updating current bw expose congestion configs split bytes acked from end ack method simplify full bandwidth check remove some unused variables add 125% of target full bandwidth condition as per docs fix loss resetting too early, fix bandwidth target units of measurement check if full bw reached calculate recovery window calculate cwnd calculate pacing rate maybe_enter_or_exit_probe_rtt update gain cycle phase refactor bandwidth estimation add cca into stats for debugging add target window add recovery state transitions loss recording bandwidth estimation

FrankSpitulski · 2021-10-29T18:44:15Z

had to rebase @Ralith

FrankSpitulski force-pushed the feat/bbr branch from e3915de to 8a1e23e Compare June 24, 2021 18:28

Matthias247 reviewed Jul 2, 2021

View reviewed changes

Ralith reviewed Jul 12, 2021

View reviewed changes

FrankSpitulski force-pushed the feat/bbr branch 3 times, most recently from 208cd2c to cdb93b8 Compare July 12, 2021 17:19

Ralith reviewed Jul 12, 2021

View reviewed changes

Comment thread quinn-proto/src/congestion/cubic.rs Outdated

Comment thread quinn-proto/src/congestion/cubic.rs Outdated

FrankSpitulski force-pushed the feat/bbr branch from ff52e7b to ea02f43 Compare July 19, 2021 22:26

FrankSpitulski force-pushed the feat/bbr branch from ea02f43 to 0ae17c2 Compare August 19, 2021 20:08

FrankSpitulski force-pushed the feat/bbr branch from 0ae17c2 to e2ebcea Compare September 1, 2021 19:16

FrankSpitulski force-pushed the feat/bbr branch 2 times, most recently from cd4b926 to 140e01a Compare September 22, 2021 19:27

FrankSpitulski force-pushed the feat/bbr branch from 140e01a to 70d4287 Compare September 29, 2021 20:59

FrankSpitulski marked this pull request as ready for review September 30, 2021 17:08

FrankSpitulski force-pushed the feat/bbr branch from 337d70c to 92ae0e4 Compare September 30, 2021 17:39

FrankSpitulski force-pushed the feat/bbr branch from 92ae0e4 to dbc844a Compare October 20, 2021 20:29

FrankSpitulski force-pushed the feat/bbr branch from dbc844a to d23e4a2 Compare October 25, 2021 18:35

Ralith requested changes Oct 26, 2021

View reviewed changes

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion.rs Outdated

djc reviewed Oct 26, 2021

View reviewed changes

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Comment thread quinn-proto/src/congestion/bbr.rs Outdated

Ralith requested changes Oct 27, 2021

View reviewed changes

Comment thread quinn-proto/src/connection/mod.rs Outdated

Comment thread quinn-proto/src/connection/mod.rs Outdated

Comment thread quinn/src/connection.rs Outdated

FrankSpitulski force-pushed the feat/bbr branch 2 times, most recently from 7c242a0 to 8425311 Compare October 29, 2021 00:20

Ralith reviewed Oct 29, 2021

View reviewed changes

Comment thread quinn-proto/src/congestion/bbr/bw_estimation.rs Outdated

Comment thread quinn-proto/src/congestion.rs Outdated

FrankSpitulski force-pushed the feat/bbr branch from 34a9ec4 to db1a797 Compare October 29, 2021 02:21

Ralith reviewed Oct 29, 2021

View reviewed changes

Comment thread quinn-proto/src/config.rs Outdated

Ralith reviewed Oct 29, 2021

View reviewed changes

Comment thread quinn-proto/src/congestion/bbr/mod.rs Outdated

Ralith previously approved these changes Oct 29, 2021

View reviewed changes

FrankSpitulski dismissed Ralith’s stale review via 616c305 October 29, 2021 18:36

FrankSpitulski force-pushed the feat/bbr branch from 69a73f0 to 616c305 Compare October 29, 2021 18:36

Ralith approved these changes Oct 29, 2021

View reviewed changes

djc merged commit 7c04a15 into quinn-rs:main Oct 30, 2021

Uh oh!

Uh oh!

Conversation

FrankSpitulski commented Jun 24, 2021

Uh oh!

Matthias247 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Matthias247 Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

FrankSpitulski Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ralith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FrankSpitulski commented Aug 10, 2021

Uh oh!

Ralith commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Sep 30, 2021

Uh oh!

FrankSpitulski commented Oct 20, 2021

Uh oh!

djc commented Oct 25, 2021

Uh oh!

Ralith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ralith left a comment

Choose a reason for hiding this comment

Uh oh!

FrankSpitulski commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Ralith commented Aug 12, 2021 •

edited

Loading