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.

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

Refactoring mutexes and evet around CSndUList #2045

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Jun 21, 2021

A rework of PRs #1814 and #1844. Closes #1814, #1844.

This PR introduces three changes divided into three commits.

Moved mutexes from CSndQueue

As noted in #1814, the CSndQueue has two members: m_WindowLock mutex and m_WindowCond CV. Pointers to those are given to CSndUList to signal an event of the list transitioning from an empty to a non-empty state.

Here is how it looks:

  • CSndUList - list (heap) of CUDT sockets to send data packets.

    • m_ListLock - locks CSndUList modifications. update, pop, remove, getNextProcTime.
    • m_pWindowLock - pointer to mutex owned by CSndQueue, only used together with the CV to signal there was a new entry added if it was empty.
    • m_pWindowCond - a pointer to a CV owned by CSndQueue.
  • CSndQueue - a class that owns CSndUList.

    • m_WindowLock
    • m_WindowCond - CV used to wait for CSndUList to become non-empty. Also signalled in the destructor to unblock the worker thread.

This PR moves m_WindowLock and m_WindowCond to CSndUList, and combines m_ListLockwithm_WindowLock. CSndUList` gains two new functions:

/// Wait for the list to become non empty.
void waitNonEmpty() const;

/// Signal to stop waiting in waitNonEmpty().
void signalInterrupt() const;

CSndUList::pop(..) logic simplified

The CSndUList::pop(..) function does several things:

  1. Pops the next socket from the heap.
  2. Asks the socket to prepare a data packet to send (CUDT::packData(..)).
  3. Re-inserts a socket back to the heap with the next processing time.

Performing these three steps overcomplicates the function and also holds a lock on the list while a data packet is prepared (which might take some time). Changes in PR #1844 were unlocking the m_ListLock before packData and locking back afterwards. But it also allows a list to be changed by some other thread, thus inserting back to the list must be done combined with checking the CUDT is not already in the list. That's what CSndUList::update(..) function does.

This PR leaves only no. 1 in the CSndUList::pop(..) function. No. 2 and 3 are moved to the CSndQueue::worker(..) function.

CSndUList::update(..) if earlier time

Now with a target processing time provided as an argument, reschedule only if the provided time is less (earlier) than the currently scheduled time. It allows to not touch the heap if already scheduled.

TODO

  • Test the changes. Tested sending over 5 simultaneous connections on the same UDP port. No issues found.
  • Maybe CSndUList::update(..)` should allow to take the smaller time of the next processing if a socket is already in the list.

Performance Considerations

Previously CSndUList::pop(..) was taking a socket from the heap, preparing a data packet to send, and rescheduling the sending. On one hand, having these three operations under the heap lock was ensuring that a socket was removed from the heap (list), and rescheduling does not need to check if the socket is in the heap.

On another hand, packing a data packet was blocking further work with the socket list (heap), e.g. adding another socket to the heap. However, there is only one sending worker thread, which can't take the next socket until data packet preparation is finished.

With the proposed changes a socket is extracted from the heap, a data packet is formed, and the socket is rescheduled if needed. Rescheduling is done with an assumption that socket might again exist in the heap, so an additional check is performed.
This check does not seem to provide any significant impact on the performance, however, it might be a point of further optimizations in the future. The updated logic of the CSndUList::update(..) function enables some further optimisations, required for #1910.

@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Jun 21, 2021
@maxsharabayko maxsharabayko added this to the v1.4.4 milestone Jun 21, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #2045 (e4dbba4) into master (86327e9) will increase coverage by 2.67%.
The diff coverage is 51.80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2045      +/-   ##
==========================================
+ Coverage   60.88%   63.56%   +2.67%     
==========================================
  Files          79       89      +10     
  Lines       17569    18972    +1403     
==========================================
+ Hits        10697    12059    +1362     
- Misses       6872     6913      +41     
Impacted Files Coverage Δ
srtcore/cache.h 80.00% <ø> (ø)
srtcore/channel.h 0.00% <0.00%> (ø)
srtcore/core.cpp 58.31% <ø> (+3.62%) ⬆️
srtcore/crypto.h 70.27% <ø> (+0.82%) ⬆️
srtcore/epoll.cpp 62.79% <0.00%> (-0.01%) ⬇️
srtcore/fec.h 64.00% <ø> (+12.00%) ⬆️
srtcore/group.cpp 36.96% <ø> (+0.53%) ⬆️
srtcore/group_backup.cpp 0.00% <0.00%> (ø)
srtcore/group_backup.h 0.00% <0.00%> (ø)
srtcore/handshake.h 81.25% <ø> (ø)
... and 112 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16eca6b...e4dbba4. Read the comment docs.

@maxsharabayko maxsharabayko marked this pull request as ready for review July 12, 2021 15:00
@maxsharabayko maxsharabayko merged commit 96a41db into Haivision:master Jul 13, 2021
@maxsharabayko maxsharabayko deleted the develop/sndulist-locks branch July 13, 2021 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants