-
Notifications
You must be signed in to change notification settings - Fork 851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Reworked the CRcvBuffer::dropMessage(..) function. #2661
[core] Reworked the CRcvBuffer::dropMessage(..) function. #2661
Conversation
Drop by seqno offset first, then refine using msgno.
What if |
This can only happen in case the sender reports an invalid range. In general, the sender should have at least the last packet of the message it is requesting to be dropped. |
Agreed, but would be nice to have it documented or commented. |
const int stop_pos = decPos(m_iStartPos); | ||
for (int i = start_pos; i != stop_pos; i = decPos(i)) | ||
{ | ||
// Can't drop is message number is not known. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is -> if?
if (minDroppedOffset == -1) | ||
minDroppedOffset = offPos(m_iStartPos, i); | ||
else | ||
minDroppedOffset = min(offPos(m_iStartPos, i), minDroppedOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since it''s searching backward, min() is useless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm.. Why? We need to find the minimum offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new offset should always be the smaller one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, minDroppedOffset
is later used to check if the next reading position should be updated.
const bool needUpdateNonreadPos = (minDroppedOffset != -1 && minDroppedOffset <= getRcvDataSize());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the new offPos(m_iStartPos, i)
should always smaller than minDroppedOffset
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, right. Given the search stays always backward it should be enough to just set minDroppedOffset = offPos(m_iStartPos, i)
.
To improve the
CRcvBuffer::dropMessage(..)
performance the packets are dropped based on pkt seqno range first.Then, if the start of the message has not been dropped, and additional search step from the
seqnolo
position downwards is applied to look for a packet withPB_FIRST
boundary flag.Proceeding after #2657.
@gou4shi1 please have a look.