Defer packet ordering until building RTCP packet #202
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #202 +/- ##
==========================================
- Coverage 79.51% 79.16% -0.36%
==========================================
Files 67 67
Lines 3369 3365 -4
==========================================
- Hits 2679 2664 -15
- Misses 572 580 +8
- Partials 118 121 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
pkg/twcc/twcc.go
Outdated
| @@ -19,7 +20,7 @@ type pktInfo struct { | |||
| // transport wide congestion control feedback reports as specified in | |||
| // https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01 | |||
| type Recorder struct { | |||
| receivedPackets []pktInfo | |||
| receivedPackets map[uint32]pktInfo | |||
There was a problem hiding this comment.
Based on very limited golang knowledge, maps seem to incur more memory (expected, but making the comment in the sense I have been surprised some times maps growing unexpectedly). Is there a need to use map here?
Also, this is constructing the slice from map in building feedback packet. That can be avoided I think if we just append and sort that slice directly.
There was a problem hiding this comment.
@boks1971 thanks for the comment! I used a map here to handle the need to de-dupe packets with the same sequence number (see the previous insertSorted method). I could use a slice and de-dupe during iteration when building the feedback packet, but this would require a map/set anyway
There was a problem hiding this comment.
Thank you @treyhakanson . As these maps should be small, it should be fine. On that, one comment, I think re-allocating the map after building report would be needed to ensure that map does not grow. Did not see it. Maybe, I missed it in the diff.
There was a problem hiding this comment.
You can do the deduplication when building the feedback packet without a map. When iterating over the sorted slice, you remember the previous value and compare it to the next because in the sorted slice, all duplicates will be neighbors. It could be somewhat fiddly, depending on whether you want to insert the first or last occurrence. Currently, we take the last (which would be more complicated), but I think we may want to use the first instead (ordered by arrival time, though, not by the time it was added). That could be added as a criterion to the sort function, then it would be easy again (just check if the current sequence number was already added in the last iteration). However, all this is probably very hypothetical since there will rarely be duplicates anyway.
There was a problem hiding this comment.
Ah you're totally right, not sure why I didn't think of that; thanks for calling out!
Updated to implement this approach and take the packet with the earliest arrival time. I also updated TestDuplicatePackets to test that we take the packet with the earliest arrival time even if it isn't the first one we record. Fwiw, I've been testing on LTE/5G networks recently and have noticed a fair bit of duplication so its seems worth handling
There was a problem hiding this comment.
Oh, that's good to know, thanks!
And thanks for implementing this change!
pkg/twcc/twcc.go
Outdated
| feedback = newFeedback(r.senderSSRC, r.mediaSSRC, r.fbPktCnt) | ||
| r.fbPktCnt++ | ||
| feedback.addReceived(uint16(pkt.sequenceNumber&0xffff), pkt.arrivalTime) | ||
| } | ||
| } | ||
| r.receivedPackets = []pktInfo{} | ||
| pkts = append(pkts, feedback.getRTCP()) | ||
| r.receivedPackets = make(map[uint32]pktInfo) |
There was a problem hiding this comment.
Ignore my comment above. See it now. Sorry, my bad.
|
For more context on the motivation behind this change, see this discussion on the pion slack channel: https://gophers.slack.com/archives/CAK2124AG/p1692822880632899 |
pkg/twcc/twcc.go
Outdated
| @@ -19,7 +20,7 @@ type pktInfo struct { | |||
| // transport wide congestion control feedback reports as specified in | |||
| // https://datatracker.ietf.org/doc/html/draft-holmer-rmcat-transport-wide-cc-extensions-01 | |||
| type Recorder struct { | |||
| receivedPackets []pktInfo | |||
| receivedPackets map[uint32]pktInfo | |||
There was a problem hiding this comment.
You can do the deduplication when building the feedback packet without a map. When iterating over the sorted slice, you remember the previous value and compare it to the next because in the sorted slice, all duplicates will be neighbors. It could be somewhat fiddly, depending on whether you want to insert the first or last occurrence. Currently, we take the last (which would be more complicated), but I think we may want to use the first instead (ordered by arrival time, though, not by the time it was added). That could be added as a criterion to the sort function, then it would be easy again (just check if the current sequence number was already added in the last iteration). However, all this is probably very hypothetical since there will rarely be duplicates anyway.
The previous implementation did a sorted insert (O(m)) on every call to `Record`. If sorting is deferred until a feedback packet is built, we can record in constant time and build a packet in O(nlogn). Ordering isn't required until building the packet anyway, and deferring nets a minor performance gain (I say minor since its unlikely there are a large number of received packets in the buffer prior to building a feedback packet). If ordering is really needed on record, we could use something like a B-tree to get O(logn) sorted inserts.
treyhakanson
force-pushed
the
twcc_sort
branch
from
0526e11 to
a646d66
Compare
Description
The previous implementation did a sorted insert (O(m)) on every call to
Record. If sorting is deferred until a feedback packet is built, we can record in constant time and build a packet in O(nlogn). Ordering isn't required until building the packet anyway, and deferring nets a minor performance gain (I say minor since its unlikely there are a large number of received packets in the buffer prior to building a feedback packet). If ordering is really needed on record, we could use something like a B-tree to get O(logn) sorted inserts.Reference issue
N/A