statistics #130
statistics #130
Conversation
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
==========================================
+ Coverage 87.50% 88.85% +1.35%
==========================================
Files 43 52 +9
Lines 8181 8527 +346
==========================================
+ Hits 7159 7577 +418
+ Misses 1022 950 -72
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
It would be great to see statistics added to srt-transmit. For now it would be fine to get print the Debug implemenation
Also--remind me why we have both run_handler_loop and run_input_loop?
I'm a bit unsure about the new package structure of "export everything"--i'm much more generally in favor of not exporting private types (I noticed this with ReceiverContext, which seems clearly private, but it is exported from the crate...). I'm okay with it for now, at some point I'll make sure our exports are minimal.
Overall looks okay though, some of it I didn't fully look over because it's impossible to see what actually changed in these files...but we have a great testsuite which makes me much more confident in general.
| } | ||
| } | ||
|
|
||
| #[allow(clippy::trivially_copy_pass_by_ref)] |
There was a problem hiding this comment.
Why suppress this? Can't we just pass by self?
There was a problem hiding this comment.
This was copied from an original implementation, basically untouched, so this is not net new code. I am unsure why this was here, I imagine just to appease clippy at some point.
| @@ -158,6 +164,19 @@ impl MessagePacketCount { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Debug, Default)] | |||
| pub struct ReceiveBufferStatistics { | |||
There was a problem hiding this comment.
I don't see this used anywhere??
There was a problem hiding this comment.
This is dead code, left over from an early iteration on the design, I'll delete.
| Some(usize::try_from(s - self.seqno0).unwrap()) | ||
| let index = usize::try_from(seq_number - self.seqno0).unwrap(); | ||
| // assert!(index < self.buffer.len(), "{:?} !< {:?}", index, self.buffer.len()); | ||
| Some(index) |
There was a problem hiding this comment.
This can probably be rolled back--I assume you found that that assert was not true?
There was a problem hiding this comment.
Yes, and I revisited the code, in a type system as powerful as Rust's is, I find defensive runtime assertions in production code to be a code smell, often times a result of Primitive Obsession.
|
Also, I'm sure you saw but warning: field is never read: `adjustments`
--> srt-protocol/src/protocol/receiver/time.rs:16:5
|
16 | adjustments: u64,
| ^^^^^^^^^^^^^^^^
|
= note: `#[warn(dead_code)]` on by default
warning: field is never read: `drift`
--> srt-protocol/src/protocol/receiver/time.rs:17:5
|
17 | drift: (TimeSpan, TimeSpan),
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
warning: `srt-protocol` (lib) generated 2 warnings |
Yes, I was going to do this next.
I wasn't sure which API I liked best yet. We could also keep both, but extract the run_input_loop into a separate struct that wraps the DuplexConnection. This would reduce the confusion.
I agree, and all of the ReceiverContext and SenderContext are used to invoke/implement behavior (i.e. mutations) for the Receiver and Sender "objects", yet implementing these behaviors in a way that is intimately familiar and responsible with the data of the Receiver and Sender. The design of ReceiverContext and SenderContext is influenced by (or at least my best attempt at) the DCI design pattern. I chose this approach because dependency injection is problematic in general in most languages, but especially in Rust. Having used Go quite a bit at work the last few years, I also dislike passing dependencies into functions everywhere, it makes the call site messy. We already do this with "now", but ultimately I was seeing the need for multiple objects that would be participating in the interaction that implements these behaviors (packet output, metrics, timers, errors/events, logging, etc.). So it started to look like DCI could help.
I LOVE this test suite! |
@russelltg I added statistics to the |
|
Yeah...srt-transmit does a bunch of stuff and it's pretty complex. Let's add it when we feel more confident about the interface. |
|
Oops |
@russelltg this is mostly to address #70, but it also includes yet another major redesign of the srt_protocol module. The redesign made it easier for me to both test and implement statistics, but I also fixed a few bugs on the way as I added more to the unit test coverage. I also fixed #106 and #104. Sorry, I know it might be an overwhelming PR to review, but it would be great to get it merged so we can iteratively fill out all the desired statistics with much smaller commits. :)