-
Notifications
You must be signed in to change notification settings - Fork 842
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
Refax: extract receiver buffer insertion handling to a separate function #2500
Refax: extract receiver buffer insertion handling to a separate function #2500
Conversation
2246e1b
to
a56c84d
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
/// @return 0 The call was successful (regardless if the packet was accepted or not). | ||
/// @return -1 The call has failed: no space left in the buffer. | ||
/// @return -2 The incoming packet exceeds the expected sequence by more than a length of the buffer (irrepairable discrepancy). | ||
int handleSocketPacketReception(const std::vector<CUnit*>& incoming, bool& w_new_inserted, bool& w_was_sent_in_order, CUDT::loss_seqs_t& w_srt_loss_seqs); |
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 name sounds confusing. Especially "socket" and "reception" parts seem unrelated or redundand.
processDataPackets
? 🤔
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.
In the group development branch, there's a second function named handleGroupPacketReception
.
// This is executed only when bonding is enabled and only | ||
// with the new buffer (in which case the buffer is in the group). | ||
|
||
const int avail_bufsize = (int) getAvailRcvBufferSizeNoLock(); |
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.
This function doesn't match the codes removed from processData()
below, several PRs were lost.
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.
What exactly? I can see only changes in logs and slight improvement for the offset counting. Some parts are left up to the calling function (emergency closing is handled through returning -2). You can cut off since this line:
UniqueLock recvbuf_acklock(m_RcvBufferLock);
and except the last two branches (which remain at the original location and are unchanged) you can see no significant changes in the handleSocketPacketReception
.
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.
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 can see only changes in logs and slight improvement for the offset counting.
Your refactor is great, but I don't think we can silently revert PRs.
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.
Hey, one moment. @maxsharabayko please join.
Since when is m_iRcvLastAck
the base sequence number for the receiver buffer?
This was so in the old UDT, and it was sharing the number that was latest received as ACK as well as the sequence number of the first cell in the receiver buffer. This has changed since the first versions of SRT when the TLPKTDROP was introduced and so a separate field m_iRcvLastSkipAck
, which has become the true beginning of the receiver buffer. Here is an excerpt from the CUDT::tsbpd
function taken from 1.4.4 version, which executes the drop functionality in order to reach the after-drop packets:
/* Packet ready to play according to time stamp but... */
int seqlen = CSeqNo::seqoff(self->m_iRcvLastSkipAck, skiptoseqno);
if (skiptoseqno != SRT_SEQNO_NONE && seqlen > 0)
{
/*
* skiptoseqno != SRT_SEQNO_NONE,
* packet ready to play but preceeded by missing packets (hole).
*/
self->updateForgotten(seqlen, self->m_iRcvLastSkipAck, skiptoseqno);
self->m_pRcvBuffer->skipData(seqlen);
self->m_iRcvLastSkipAck = skiptoseqno;
Here you can see that the call to skipData
is getting only the number of cells to drop as the old receiver buffer had no idea about the sequence numbers at all. The code is almost the same in version 1.2.0, the very first line added to Github. The base sequence number to calculate seqlen
is taken from m_iRcvLastSkipAck
and this is the real base sequence number for the receiver buffer.
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.
Actually from my production logs, I think m_iRcvLastSkipAck
behaved better than m_iRcvLastAck
as the base seq of recv buffer.
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.
So the conclusion is using m_iRcvLastSkipAck
as the recv buffer base seq? Then I would suggest to change getAvailRcvBufferSizeNoLock()
to return m_pRcvBuffer->getAvailSize(m_iRcvLastSkipAck);
to keep offset
consistent with avail_bufsize
.
This doesn't change the result of ACKD_BUFFERLEFT
, because it happens after ackDataUpTo()
, as #2467 (comment) said .
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.
And m_pRcvBuffer->strFullnessState(qrlog.Warn.CheckEnabled(), m_iRcvLastAck, steady_clock::now())
should be fixed to m_iRcvLastSkipAck
also
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 difference is that at the moment when empty cells are being removed from the buffer - per TLPKTDROP - then m_iRcvLastSkipAck
is updated, but m_iRcvLastAck
is not. The latter contains the ACK number that was last received, and when it happens, the cells are also being removed from the receiver buffer up to this number, and both fields are then updated. Hence, m_iRcvLastSkipAck
contains the sequence number of the cell 0 of the receiver buffer, while m_iRcvLastAck
is either equal to m_iRcvLastSkipAck
, or points to a sequence in the past (already cut off from the buffer).
In any case when there's a need to get some value to operate with the buffer, m_iRcvLastAck
shall never be used.
@@ -9665,20 +9665,10 @@ bool srt::CUDT::overrideSndSeqNo(int32_t seq) | |||
return true; | |||
} | |||
|
|||
int srt::CUDT::processData(CUnit* in_unit) | |||
int srt::CUDT::checkLazySpawnLatencyThread() |
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.
int srt::CUDT::checkLazySpawnLatencyThread() | |
int srt::CUDT::checkLazySpawnTsbpdThread() |
const int pktrexmitflag = m_bPeerRexmitFlag ? (rpkt.getRexmitFlag() ? 1 : 0) : 2; | ||
const bool retransmitted = pktrexmitflag == 1; | ||
|
||
int buffer_add_result; |
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.
buffer_add_result
defined without initial value, why not define in line 9790, i.e.
int buffer_add_result = m_pRcvBuffer->insert(u);
continue; | ||
} | ||
|
||
// This is executed only when bonding is enabled and only |
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.
What does This
stands for?
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 below call. Although this comment is false and comes from a version with changes that are not brought here.
buffer_add_result = m_pRcvBuffer->insert(u); | ||
if (buffer_add_result < 0) | ||
{ | ||
// addData returns -1 if at the m_iLastAckPos+offset position there already is a packet. |
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.
// addData returns -1 if at the m_iLastAckPos+offset position there already is a packet. | |
// insert() returns -1 if at the m_iLastAckPos+offset position there already is a packet. |
// addData returns -1 if at the m_iLastAckPos+offset position there already is a packet. | ||
// So this packet is "redundant". | ||
IF_HEAVY_LOGGING(exc_type = "UNACKED"); | ||
adding_successful = false; |
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.
Also need to update w_new_inserted
?
adding_successful = false; | |
adding_successful = false; | |
w_new_inserted = false; |
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.
This is initialized to false before passing. Although you are partially right - the style for reference passing forbids relying on the initial values set before passing (this is only allowed in case of in-out protocol), so it should be at least initialized in the beginning of the function.
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.
it should be at least initialized in the beginning of the function.
Agreed.
@@ -9853,208 +10077,54 @@ int srt::CUDT::processData(CUnit* in_unit) | |||
} | |||
#endif | |||
|
|||
// NULL time by default | |||
time_point next_tsbpd_avail; |
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.
next_tsbpd_avail
is not used
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 this used in your following PRs?
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. See #2527
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.
OK, it's a big change, I would find some time to study it.
Sure, will go. |
Extracted the part handling the insertion of the newly coming in packet into the buffer in
CUDT::processData
to a separate function.This fragment will be, in perspective, added an alternative handling for the implementation of the group-common receiver buffer.