Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[core] Slightly optimize the RCV drop by message number. #2686

Merged
merged 2 commits into from
Aug 9, 2023
Merged

[core] Slightly optimize the RCV drop by message number. #2686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f4c8973
[core] some minor improvements on logs and comments
gou4shi1 Mar 11, 2023
2645b43
Added some comments
maxsharabayko Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions srtcore/buffer_rcv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,8 +257,9 @@ int CRcvBuffer::dropAll()
int CRcvBuffer::dropMessage(int32_t seqnolo, int32_t seqnohi, int32_t msgno, DropActionIfExists actionOnExisting)
{
IF_RCVBUF_DEBUG(ScopedLog scoped_log);
IF_RCVBUF_DEBUG(scoped_log.ss << "CRcvBuffer::dropMessage: seqnolo " << seqnolo << " seqnohi " << seqnohi
<< ", msgno " << msgno << " m_iStartSeqNo " << m_iStartSeqNo);
IF_RCVBUF_DEBUG(scoped_log.ss << "CRcvBuffer::dropMessage(): %(" << seqnolo << " - " << seqnohi << ")"
<< " #" << msgno << " actionOnExisting=" << actionOnExisting << " m_iStartSeqNo=%"
<< m_iStartSeqNo);

// Drop by packet seqno range to also wipe those packets that do not exist in the buffer.
const int offset_a = CSeqNo::seqoff(m_iStartSeqNo, seqnolo);
Expand Down Expand Up @@ -293,7 +294,9 @@ int CRcvBuffer::dropMessage(int32_t seqnolo, int32_t seqnohi, int32_t msgno, Dro
if (bKeepExisting && bnd == PB_SOLO)
{
bDropByMsgNo = false; // Solo packet, don't search for the rest of the message.
LOGC(rbuflog.Debug, log << "CRcvBuffer.dropMessage(): Skipped dropping an exising SOLO packet %" << packetAt(i).getSeqNo() << ".");
Copy link
Contributor Author

@gou4shi1 gou4shi1 Mar 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed typo: exising

LOGC(rbuflog.Debug,
log << "CRcvBuffer::dropMessage(): Skipped dropping an existing SOLO packet %"
<< packetAt(i).getSeqNo() << ".");
continue;
}

Expand All @@ -319,13 +322,15 @@ int CRcvBuffer::dropMessage(int32_t seqnolo, int32_t seqnohi, int32_t msgno, Dro

if (bDropByMsgNo)
{
// First try to drop by message number in case the message starts earlier thtan @a seqnolo.
// The sender should have the last packet of the message it is requesting to be dropped,
// therefore we don't search forward.
// If msgno is specified, potentially not the whole message was dropped using seqno range.
// The sender might have removed the first packets of the message, and thus @a seqnolo may point to a packet in the middle.
// The sender should have the last packet of the message it is requesting to be dropped.
// Therefore we don't search forward, but need to check earlier packets in the RCV buffer.
// Try to drop by the message number in case the message starts earlier than @a seqnolo.
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.
// Can't drop if message number is not known.
if (!m_entries[i].pUnit) // also dropped earlier.
continue;

Expand All @@ -336,21 +341,19 @@ int CRcvBuffer::dropMessage(int32_t seqnolo, int32_t seqnohi, int32_t msgno, Dro

if (bKeepExisting && bnd == PB_SOLO)
{
LOGC(rbuflog.Debug, log << "CRcvBuffer.dropMessage(): Skipped dropping an exising SOLO message packet %"
<< packetAt(i).getSeqNo() << ".");
LOGC(rbuflog.Debug,
log << "CRcvBuffer::dropMessage(): Skipped dropping an existing SOLO message packet %"
<< packetAt(i).getSeqNo() << ".");
break;
}

++iDropCnt;
dropUnitInPos(i);
m_entries[i].status = EntryState_Drop;
// As the search goes backward, i is always earlier than minDroppedOffset.
minDroppedOffset = offPos(m_iStartPos, i);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said in #2661 (comment)

gou4shi1 marked this conversation as resolved.
Show resolved Hide resolved

if (minDroppedOffset == -1)
minDroppedOffset = offPos(m_iStartPos, i);
else
minDroppedOffset = min(offPos(m_iStartPos, i), minDroppedOffset);

// Break the loop if the start of message has been found. No need to search further.
// Break the loop if the start of the message has been found. No need to search further.
if (bnd == PB_FIRST)
break;
}
Expand Down
8 changes: 4 additions & 4 deletions srtcore/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9754,7 +9754,7 @@ void srt::CUDT::sendLossReport(const std::vector<std::pair<int32_t, int32_t> > &
bool srt::CUDT::overrideSndSeqNo(int32_t seq)
{
// This function is intended to be called from the socket
// group managmenet functions to synchronize the sequnece in
// group management functions to synchronize the sequnece in
// all sockes in the bonding group. THIS sequence given
// here is the sequence TO BE STAMPED AT THE EXACTLY NEXT
// sent payload. Therefore, screw up the ISN to exactly this
Expand Down Expand Up @@ -9796,9 +9796,9 @@ bool srt::CUDT::overrideSndSeqNo(int32_t seq)
// the latter is ahead with the number of packets already scheduled, but
// not yet sent.

HLOGC(gslog.Debug, log << CONID() << "overrideSndSeqNo: sched-seq=" << m_iSndNextSeqNo << " send-seq=" << m_iSndCurrSeqNo
<< " (unchanged)"
gou4shi1 marked this conversation as resolved.
Show resolved Hide resolved
);
HLOGC(gslog.Debug,
log << CONID() << "overrideSndSeqNo: sched-seq=" << m_iSndNextSeqNo << " send-seq=" << m_iSndCurrSeqNo
<< " (unchanged)");
return true;
}

Expand Down
10 changes: 4 additions & 6 deletions srtcore/group.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,12 +1223,10 @@ int CUDTGroup::sendBroadcast(const char* buf, int len, SRT_MSGCTRL& w_mc)
// and therefore take over the leading role in setting the ISN. If the
// second one fails, too, then the only remaining idle link will simply
// go with its own original sequence.
//
// On the opposite side the reader should know that the link is inactive
// so the first received payload activates it. Activation of an idle link
// means that the very first packet arriving is TAKEN AS A GOOD DEAL, that is,
// no LOSSREPORT is sent even if the sequence looks like a "jumped over".
gou4shi1 marked this conversation as resolved.
Show resolved Hide resolved
// Only for activated links is the LOSSREPORT sent upon seqhole detection.

// On the opposite side, if the first packet arriving looks like a jump over,
// the corresponding LOSSREPORT is sent. For packets that are truly lost,
// the sender retransmits them, for packets that before ISN, DROPREQ is sent.

// Now we can go to the idle links and attempt to send the payload
// also over them.
Expand Down