TWCC based congestion control - v0#3165
Conversation
| packetsLostFromRR += (1 << 32) | ||
| r.packetsLostFromRR = r.packetsLostFromRR&0xFFFF_FFFF_FF00_0000 + uint64(rr.TotalLost) | ||
| if ((rr.TotalLost-r.lastRR.TotalLost)&((1<<24)-1)) < (1<<23) && rr.TotalLost < r.lastRR.TotalLost { | ||
| r.packetsLostFromRR += (1 << 24) |
There was a problem hiding this comment.
drive-by clean up. There is a 24-bit field in TWCC report also. Was handling that and remembered this. So, doing the same handling here.
paulwe
left a comment
paulwe
left a comment
There was a problem hiding this comment.
only really have superficial feedback but lgtm 👍
| onCongestionStateChange func(congestionState CongestionState, estimatedAvailableChannelCapacity int64) | ||
| } | ||
|
|
||
| func NewCongestionDetector(params congestionDetectorParams) *congestionDetector { |
There was a problem hiding this comment.
this can't be called by other modules because the params are private. should the constructor also be private? this applies to a bunch of the new constructors
There was a problem hiding this comment.
Yeah true. Will change those. I am always confused by those and I usually end up changing it a few times developing a module :-) . I was thinking if all methods should be private as well, but going to leave those as public, but just change the contructors.
There was a problem hiding this comment.
i think using "public" members on private types can make things more readable it was just he constructors that seemed confusing
| c.lock.Lock() | ||
| defer c.lock.Unlock() |
There was a problem hiding this comment.
might only need to guard PushBack?
There was a problem hiding this comment.
have been bitten by "posting to a closed channel" panic enough times :-) . So, tend to lock that as well. But, that is not doing what it is intended here. Will add a check for stop fuse also.
Will select not attempt to post if channel is closed? Not able to find good behaviour documentation there.
There was a problem hiding this comment.
Will merge this for now. Will change it is posting to channel can be done outside the lock. I think there are other places that can change as well if the posting can happen outside.
There was a problem hiding this comment.
it might make more sense to use c.stop.Break() to stop the worker then you don't have to worry about writing to a closed channel. you would never want to close wake anyway because as long as c.stop isn't broken the worker will infinitely loop selecting the closed wake channel
There was a problem hiding this comment.
Sounds good. Will make a small PR to just address that.
| type packetTracker struct { | ||
| params packetTrackerParams | ||
|
|
||
| lock sync.Mutex |
There was a problem hiding this comment.
it looks like access to this type is always externally synchronized?
There was a problem hiding this comment.
Unfortunately no. I initially relied on the wrapping lock (from congestion_detector), but in the forward path, when sequence numbers are created, congestion_detector is not in the path.
Also, in the reverse path, did not want to rely on the wrapping lock as it would mean holding the lock while the report is processed (it could be fast, but it would still affect the forward path).
There was a problem hiding this comment.
oh, i missed that it's part of the public interface of congestion detector...
2 participants
Making a PR to kick off this development. It is missing some bits (see notes below), but wanted to get this going so that we can make incremental/iterative commits. This is already quite large a change.
There are comments in code, but some guidance on reviewing if anyone needs it.
pacer/no_queue.go)RecordPacketSendAndGetSequenceNumberinsendsidebwe/packet_tracker.gorecords the outgoing packet with the TWCC sequence number.HandleRTCPinsendsidebwe/congestion_detector.goqueues up TWCC feedback reports and processes it in a goroutine (processFeedbackReportin that file ☝️ ).sendsidebwe/packet_group.goaccumulates packets acknowledged in the TWCC feedback report into groups.sendsidebwe/congestion_detector.gohas congestion detection state machine and bandwidth estimation.StreamAllocatorwhich uses the congestion signal to adjust allocation.TODO: