Add BBR Flow-Control #294
Merged
Add BBR Flow-Control #294
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Better separation of concerns.
- All of the needed fields of state are present and documented. I think I have my head wrapped around the whole thing now. - onAck is "fully" implemented.
The rtprop filter's window is time based.
Will be useful for computing estimates; doing this in a loop is awkward because of the ringbuffer nature.
...to use queue, and in the case of rtProp to avoid unbounded space.
The zero value for nextSample gets treated as an actual sample. So strip things out of the queue after updating it, and make sure it's timestamp makes it immediately obsolete.
This is going to be the public interface for this package. I want to use `state` to refer to something else.
Running out of steam, putting this down for a bit.
This looks different than in the paper due to the need to actually block things. Needs testing, and bits need docs.
Needs testing. Then we need to use it for testing.
In particular, in doSend we divide by pacingGain * btlBw, so the latter musn't be zero.
lthibault
reviewed
Aug 21, 2022
...which is more idiomatic for this kind of thing.
When I wrote this, the value we were supplying was 0, which could take precedence over an actual sample if the clock was set to within 1 second of MinInt64. But now that the value is MaxInt64, it will be superseded even then.
Manager is a huge code smell that usually means "I couldn't figure out what to call it," and this corresponds to the FlowLimiter interface in the flowcontrol package.
This should make it easier to understand what's going on.
Specifically: move the undesired state to the end of the array, so we can just pick a random index before then, rather than having to check if we go the bad one. This also fixes a probably-inconsequential bug in that the probability of choosing the index after the low one was slightly higher than the others; it should be a properly uniform distribution now.
The way we were/weren't using appLimitedUntil was... wrong, I think mostly as a function of me having a shaky understanding of how it worked when I first wrote this. After fixing this the bbr limiter gets within a factor of 3 for the bandwidth on my local network.
The tests here never really made sense.
...so we can practically use go test with this here.
- The fine grained time measurement seems to be working fine, so let's leave it. - cwndGain is actually essential to how startup works at least, so we can't just kill it.
...so I can get this info outside the test suite, for debugging.
...which has been useful for debugging.
- SnapshotLimiter is now safe to call concurrently with other operations on the limiter (uses whilePaused internally). - There is now a SnapshottingLimiter that takes snapshots at each point of interest.
The intent here was to test if there was significant overhead for this that could be throwing off results. Answer: no.
...four packets, as discussed in the paper.
...and make `underlying` private.
...instead of just appending them to a slice. This allows for streaming collection of data.
...as flowcontrol/bbr
Also, bbr.NewLimiter now defaults to using the system clock if its argument is nil. flowcontrol.NewBBR() has been removed; just use bbr.NewLimiter(nil)
lthibault
approved these changes
Nov 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is still WIP; what's left:
Probably clean up the historywe're just going to squash-merge this.Marking it as a draft, but the basic structure is there.