Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #327 +/- ##
==========================================
+ Coverage 71.59% 72.24% +0.64%
==========================================
Files 79 79
Lines 4746 4756 +10
==========================================
+ Hits 3398 3436 +38
+ Misses 1207 1183 -24
+ Partials 141 137 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JoTurk
left a comment
There was a problem hiding this comment.
The PR makes sense to me, but after looking at the code I found those two concerns.
Thank you so much.
| for i := s.lastConsecutive + 1; i != until+1; i++ { | ||
| if !s.getReceived(i) { | ||
| missingPacketSeqNums = append(missingPacketSeqNums, i) | ||
| missingPacketSeqNums[c] = i |
There was a problem hiding this comment.
I think we need protection against overflow here, if size < c?
There was a problem hiding this comment.
I believe I’ve already added a test case for this scenario, and the code behaves as expected.
Assume the receiverLog size is 128.
Initially, packet 101 is received. At this point, the lastConsecutive is 101.
Next, packet 250 is received. While adding the packet, Since 250 is beyond the window size of 128, it falls under the logic at:
interceptor/pkg/nack/receive_log.go
Line 71 in 96a23bb
interceptor/pkg/nack/receive_log.go
Line 72 in 96a23bb
These lines reset lastConsecutive, which is guaranteed to be less than 128.
Based on this, I don’t believe an overflow occurs.
Please feel free to correct me if I’m mistaken. Thank you.
JoTurk
left a comment
There was a problem hiding this comment.
I've tested it manually, and it seems to re-produce all the old behavior exactly. The only trade-off is that we now allocate two fixed n.size slices each interval, which in a perfect conditions does introduce a small constant overhead? (It'll be nothing after all). please correct me if I'm wrong.
As a side note: we need tests for a few of the more corner-case path. There are a few more mutations more than I'm comfortable with (Not your PR, the behavior of the code itself before your PR) This is why I run manual tests :) I'll make future PRs for tests.
Thanks for the great work on this! This is great!
40240ba to
ddeff29
Compare
Refactored packet processing to avoid multiple make calls and eliminate append usage. This reduces garbage collection overhead and enhances overall memory management efficiency.
ddeff29 to
540f6a9
Compare
Refactored packet processing to avoid multiple make calls and eliminate the use of append. This reduces garbage collection overhead and improves overall memory management efficiency.
Description
Reference issue
Fixes #...