Don't retain Receiver Report loss info when stats are disabled #613
Conversation
This prevents unbounded memory growth of the Vec that holds onto packet loss records derived from RTCP Receiver Reports, if the method that would drain that Vec will never get called.
There was a problem hiding this comment.
Thanks for tackling this!
I left an idea on how I think this could be solved in a more type-safe manner. Before you tackle it, let's hear what @algesten thinks of it :)
| @@ -153,7 +153,7 @@ pub(crate) struct StreamTxStats { | |||
| /// Can be null in case of missing or bad reports | |||
| rtt: Option<f32>, | |||
| /// losses collecter from RR (known packets, lost ratio) | |||
| losses: Vec<(u64, f32)>, | |||
| losses: Option<Vec<(u64, f32)>>, | |||
There was a problem hiding this comment.
What I would suggest is, to make a newtype called Losses that has two constructors:
newno_op/dummyetc
We then pass Losses as a parameter through all layers that don't yet know whether capturing Losses will be required. At the point where we have the config available, we can then decide which constructor to call.
This newtype can also encapsulate some of the functionality on how the data is actually processed and inserted, i.e. provide a more specific interface for the other components to interact with.
If you wanted to go full on type-safe, we could also define Losses as an enum such that in the no-op mode, there isn't even an underlying data-structure to operate on / fill.
| }; | ||
| if let Some(losses) = self.losses.as_mut() { | ||
| let ext_seq = { | ||
| let prev = losses.last().map(|s| s.0).unwrap_or(r.max_seq as u64); |
There was a problem hiding this comment.
All this logic could go onto the Losses newtype.
| } | ||
|
|
||
| pub(crate) fn fill(&mut self, snapshot: &mut StatsSnapshot, midrid: MidRid, now: Instant) { | ||
| if self.bytes == 0 { | ||
| return; | ||
| } | ||
|
|
||
| let loss = { | ||
| let loss = self.losses.as_mut().and_then(|losses| { |
There was a problem hiding this comment.
Similar here, this computation can be moved onto the Losses newtype.
Fixes #610
StreamTxStatscannot beOption'd out entirely if stats reports are disabled, because it's still used for other purposes, e.g. RTX ratio calculation. Trying to split it out into a stats-reports and non-stats-reports portion was also awkward, due to overlap.Instead, we give it a hint that
fill()will never be called, which will prevent it from creating alossesarray at all. Fortunately, bad/missing loss data was already accounted for, so even if it does end up being called, the behavior will still be sane (i.e. no panics, no bogus data).I also considered a solution that would just put an upper bound on the
lossesarray instead, but that had a few downsides: