From cf9e5d01564c0d69de6a5ab31384cc03868b9a88 Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Fri, 20 Aug 2021 12:44:04 +0100 Subject: [PATCH 1/5] Combine FindChannel() and GetFreeChan() This is preparatory for improving the performance of FindChannel() with high numbers of channels. Also call IsConnected() separately from GetAddress(). --- src/channel.cpp | 16 ------- src/channel.h | 1 - src/server.cpp | 108 +++++++++++++++++++----------------------------- src/server.h | 4 +- 4 files changed, 45 insertions(+), 84 deletions(-) diff --git a/src/channel.cpp b/src/channel.cpp index cb0afb0041..c5e247ae44 100644 --- a/src/channel.cpp +++ b/src/channel.cpp @@ -400,22 +400,6 @@ void CChannel::OnChangeChanPan ( int iChanID, float fNewPan ) { SetPan ( iChanID void CChannel::OnChangeChanInfo ( CChannelCoreInfo ChanInfo ) { SetChanInfo ( ChanInfo ); } -bool CChannel::GetAddress ( CHostAddress& RetAddr ) -{ - QMutexLocker locker ( &Mutex ); - - if ( IsConnected() ) - { - RetAddr = InetAddr; - return true; - } - else - { - RetAddr = CHostAddress(); - return false; - } -} - void CChannel::OnNetTranspPropsReceived ( CNetworkTransportProps NetworkTransportProps ) { // only the server shall act on network transport properties message diff --git a/src/channel.h b/src/channel.h index 5d36a4d4bc..34cb091948 100644 --- a/src/channel.h +++ b/src/channel.h @@ -84,7 +84,6 @@ class CChannel : public QObject bool IsEnabled() { return bIsEnabled; } void SetAddress ( const CHostAddress NAddr ) { InetAddr = NAddr; } - bool GetAddress ( CHostAddress& RetAddr ); const CHostAddress& GetAddress() const { return InetAddr; } void ResetInfo() diff --git a/src/server.cpp b/src/server.cpp index 6635a3e796..641c0d9866 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1461,21 +1461,6 @@ void CServer::CreateOtherMuteStateChanged ( const int iCurChanID, const int iOth } } -int CServer::GetFreeChan() -{ - // look for a free channel - for ( int i = 0; i < iMaxNumChannels; i++ ) - { - if ( !vecChannels[i].IsConnected() ) - { - return i; - } - } - - // no free channel found, return invalid ID - return INVALID_CHANNEL_ID; -} - int CServer::GetNumberOfConnectedClients() { int iNumConnClients = 0; @@ -1493,27 +1478,57 @@ int CServer::GetNumberOfConnectedClients() return iNumConnClients; } -int CServer::FindChannel ( const CHostAddress& CheckAddr ) +int CServer::FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew ) { - CHostAddress InetAddr; + int iNewChanID = INVALID_CHANNEL_ID; // check for all possible channels if IP is already in use for ( int i = 0; i < iMaxNumChannels; i++ ) { - // the "GetAddress" gives a valid address and returns true if the - // channel is connected - if ( vecChannels[i].GetAddress ( InetAddr ) ) + if ( vecChannels[i].IsConnected() ) { // IP found, return channel number - if ( InetAddr == CheckAddr ) + if ( vecChannels[i].GetAddress() == CheckAddr ) { return i; } } + else if ( bAllowNew && iNewChanID == INVALID_CHANNEL_ID ) + { + iNewChanID = i; + } } - // IP not found, return invalid ID - return INVALID_CHANNEL_ID; + // Initialise if it's a new channel + if ( iNewChanID != INVALID_CHANNEL_ID ) + { + InitChannel ( iNewChanID, CheckAddr ); + } + + // IP not found, return new channel or invalid ID + return iNewChanID; +} + +void CServer::InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ) +{ + // initialize new channel by storing the calling host address + vecChannels[iNewChanID].SetAddress ( InetAddr ); + + // reset channel info + vecChannels[iNewChanID].ResetInfo(); + + // reset the channel gains/pans of current channel, at the same + // time reset gains/pans of this channel ID for all other channels + for ( int i = 0; i < iMaxNumChannels; i++ ) + { + vecChannels[iNewChanID].SetGain ( i, 1.0 ); + vecChannels[iNewChanID].SetPan ( i, 0.5 ); + + // other channels (we do not distinguish the case if + // i == iCurChanID for simplicity) + vecChannels[i].SetGain ( iNewChanID, 1.0 ); + vecChannels[i].SetPan ( iNewChanID, 0.5 ); + } } void CServer::OnProtcolCLMessageReceived ( int iRecID, CVector vecbyMesBodyData, CHostAddress RecHostAddr ) @@ -1543,48 +1558,13 @@ bool CServer::PutAudioData ( const CVector& vecbyRecBuf, const int iNum QMutexLocker locker ( &Mutex ); bool bNewConnection = false; // init return value - bool bChanOK = true; // init with ok, might be overwritten // Get channel ID ------------------------------------------------------ // check address - iCurChanID = FindChannel ( HostAdr ); - - if ( iCurChanID == INVALID_CHANNEL_ID ) - { - // a new client is calling, look for free channel - iCurChanID = GetFreeChan(); - - if ( iCurChanID != INVALID_CHANNEL_ID ) - { - // initialize current channel by storing the calling host - // address - vecChannels[iCurChanID].SetAddress ( HostAdr ); - - // reset channel info - vecChannels[iCurChanID].ResetInfo(); - - // reset the channel gains/pans of current channel, at the same - // time reset gains/pans of this channel ID for all other channels - for ( int i = 0; i < iMaxNumChannels; i++ ) - { - vecChannels[iCurChanID].SetGain ( i, 1.0 ); - vecChannels[iCurChanID].SetPan ( i, 0.5 ); + iCurChanID = FindChannel ( HostAdr, true ); - // other channels (we do not distinguish the case if - // i == iCurChanID for simplicity) - vecChannels[i].SetGain ( iCurChanID, 1.0 ); - vecChannels[i].SetPan ( iCurChanID, 0.5 ); - } - } - else - { - // no free channel available - bChanOK = false; - } - } - - // Put received audio data in jitter buffer ---------------------------- - if ( bChanOK ) + // If channel is valid or new, put received audio data in jitter buffer ---------------------------- + if ( iCurChanID != INVALID_CHANNEL_ID ) { // put packet in socket buffer if ( vecChannels[iCurChanID].PutAudioData ( vecbyRecBuf, iNumBytesRead, HostAdr ) == PS_NEW_CONNECTION ) @@ -1603,8 +1583,6 @@ void CServer::GetConCliParam ( CVector& vecHostAddresses, CVector& veciJitBufNumFrames, CVector& veciNetwFrameSizeFact ) { - CHostAddress InetAddr; - // init return values vecHostAddresses.Init ( iMaxNumChannels ); vecsName.Init ( iMaxNumChannels ); @@ -1614,10 +1592,10 @@ void CServer::GetConCliParam ( CVector& vecHostAddresses, // check all possible channels for ( int i = 0; i < iMaxNumChannels; i++ ) { - if ( vecChannels[i].GetAddress ( InetAddr ) ) + if ( vecChannels[i].IsConnected() ) { // get requested data - vecHostAddresses[i] = InetAddr; + vecHostAddresses[i] = vecChannels[i].GetAddress(); vecsName[i] = vecChannels[i].GetName(); veciJitBufNumFrames[i] = vecChannels[i].GetSockBufNumFrames(); veciNetwFrameSizeFact[i] = vecChannels[i].GetNetwFrameSizeFact(); diff --git a/src/server.h b/src/server.h index f7ed776ee5..3ec1c00c70 100644 --- a/src/server.h +++ b/src/server.h @@ -257,8 +257,8 @@ class CServer : public QObject, public CServerSlots // access functions for actual channels bool IsConnected ( const int iChanNum ) { return vecChannels[iChanNum].IsConnected(); } - int GetFreeChan(); - int FindChannel ( const CHostAddress& CheckAddr ); + int FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew = false ); + void InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ); int GetNumberOfConnectedClients(); CVector CreateChannelList(); From 2cf8e104b7f220ed9764ea37ba6501bf37bdfb4c Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Fri, 20 Aug 2021 23:05:44 +0100 Subject: [PATCH 2/5] Add FreeChannel() for future use --- src/server.cpp | 11 +++++++++++ src/server.h | 1 + 2 files changed, 12 insertions(+) diff --git a/src/server.cpp b/src/server.cpp index 641c0d9866..51099ae8cf 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1054,10 +1054,15 @@ void CServer::DecodeReceiveData ( const int iChanCnt, const int iNumClients ) emit ClientDisconnected ( iCurChanID ); // TODO do this outside the mutex lock? } + FreeChannel ( iCurChanID ); // note that the channel is now not in use + // note that no mutex is needed for this shared resource since it is not a // read-modify-write operation but an atomic write and also each thread can // only set it to true and never to false bChannelIsNowDisconnected = true; + + // since the channel is no longer in use, we should return + return; } // get pointer to coded data @@ -1531,6 +1536,12 @@ void CServer::InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ) } } +void CServer::FreeChannel ( const int iCurChanID ) +{ + // for future use with improved channel search + Q_UNUSED ( iCurChanID ); +} + void CServer::OnProtcolCLMessageReceived ( int iRecID, CVector vecbyMesBodyData, CHostAddress RecHostAddr ) { QMutexLocker locker ( &Mutex ); diff --git a/src/server.h b/src/server.h index 3ec1c00c70..efa34ba5c8 100644 --- a/src/server.h +++ b/src/server.h @@ -259,6 +259,7 @@ class CServer : public QObject, public CServerSlots int FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew = false ); void InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ); + void FreeChannel ( const int iCurChanID ); int GetNumberOfConnectedClients(); CVector CreateChannelList(); From 5478caae2166ce20d65cd5ba0ff2c5bb33a7dade Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Sun, 22 Aug 2021 17:40:15 +0100 Subject: [PATCH 3/5] Improve search algorithm in CServer::FindChannel() The old algorithm used sequential search, which was very inefficient for large numbers of channels. This is now changed to use a binary search, by maintaining an array of channel IDs ordered by IP+port. The actual ordering is not important, so long as it is deterministic. --- src/server.cpp | 103 +++++++++++++++++++++++++++++++++++-------------- src/server.h | 9 ++++- src/util.cpp | 44 +++++++++++++++++++++ src/util.h | 2 + 4 files changed, 127 insertions(+), 31 deletions(-) diff --git a/src/server.cpp b/src/server.cpp index 51099ae8cf..84f199dd80 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -235,6 +235,7 @@ CServer::CServer ( const int iNewMaxNumChan, bUseDoubleSystemFrameSize ( bNUseDoubleSystemFrameSize ), bUseMultithreading ( bNUseMultithreading ), iMaxNumChannels ( iNewMaxNumChan ), + iCurNumChannels ( 0 ), Socket ( this, iPortNumber, iQosNumber, strServerBindIP, bNEnableIPv6 ), Logging(), iFrameCount ( 0 ), @@ -410,6 +411,7 @@ CServer::CServer ( const int iNewMaxNumChan, for ( i = 0; i < iMaxNumChannels; i++ ) { vecChannels[i].SetEnable ( true ); + vecChannelOrder[i] = i; } int iAvailableCores = QThread::idealThreadCount(); @@ -1468,49 +1470,69 @@ void CServer::CreateOtherMuteStateChanged ( const int iCurChanID, const int iOth int CServer::GetNumberOfConnectedClients() { - int iNumConnClients = 0; + QMutexLocker locker ( &MutexChanOrder ); - // check all possible channels for connection status - for ( int i = 0; i < iMaxNumChannels; i++ ) - { - if ( vecChannels[i].IsConnected() ) - { - // this channel is connected, increment counter - iNumConnClients++; - } - } - - return iNumConnClients; + return iCurNumChannels; } +// CServer::FindChannel() is called for every received audio packet or connected protocol +// packet, to find the channel ID associated with the source IP address and port. +// In order to search as efficiently as possible, a list of active channel IDs is stored +// in vecChannelOrder[], sorted by IP and port (according to CHostAddress::Compare()), +// and a binary search is used to find either the existing channel, or the position at +// which a new channel should be inserted. + int CServer::FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew ) { int iNewChanID = INVALID_CHANNEL_ID; - // check for all possible channels if IP is already in use - for ( int i = 0; i < iMaxNumChannels; i++ ) + QMutexLocker locker ( &MutexChanOrder ); + + int l = 0, r = iCurNumChannels, i; + + // use binary search to find the channel + while ( r > l ) { - if ( vecChannels[i].IsConnected() ) + int t = ( r + l ) / 2; + int cmp = CheckAddr.Compare ( vecChannels[vecChannelOrder[t]].GetAddress() ); + + if ( cmp == 0 ) { - // IP found, return channel number - if ( vecChannels[i].GetAddress() == CheckAddr ) - { - return i; - } + // address and port match + return vecChannelOrder[t]; } - else if ( bAllowNew && iNewChanID == INVALID_CHANNEL_ID ) + + if ( cmp < 0 ) { - iNewChanID = i; + l = t + 1; } + else + { + r = t; + } + } + + // existing channel not found - return if we cannot create a new channel + if ( !bAllowNew || iCurNumChannels >= iMaxNumChannels ) + { + return INVALID_CHANNEL_ID; } - // Initialise if it's a new channel - if ( iNewChanID != INVALID_CHANNEL_ID ) + // allocate a new channel + i = iCurNumChannels++; // save index of free channel and increment count + iNewChanID = vecChannelOrder[i]; + InitChannel ( iNewChanID, CheckAddr ); + + // now l == r == position in vecChannelOrder to insert iNewChanID + // move channel IDs up by one starting at the top and working down + while ( i > r ) { - InitChannel ( iNewChanID, CheckAddr ); + int j = i--; + vecChannelOrder[j] = vecChannelOrder[i]; } + // insert the new channel ID in the correct place + vecChannelOrder[i] = iNewChanID; - // IP not found, return new channel or invalid ID return iNewChanID; } @@ -1536,10 +1558,33 @@ void CServer::InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ) } } +// CServer::FreeChannel() is called to remove a channel from the list of active channels. +// The remaining ordered IDs are moved down by one space, and the freed ID is moved to the +// end, ready to be reused by the next new connection. + void CServer::FreeChannel ( const int iCurChanID ) { - // for future use with improved channel search - Q_UNUSED ( iCurChanID ); + QMutexLocker locker ( &MutexChanOrder ); + + for ( int i = 0; i < iCurNumChannels; i++ ) + { + if ( vecChannelOrder[i] == iCurChanID ) + { + --iCurNumChannels; + + // move channel IDs down by one starting at the bottom and working up + while ( i < iCurNumChannels ) + { + int j = i++; + vecChannelOrder[j] = vecChannelOrder[i]; + } + // put deleted channel at the end ready for re-use + vecChannelOrder[i] = iCurChanID; + return; + } + } + + qWarning() << "FreeChannel() called with invalid channel ID"; } void CServer::OnProtcolCLMessageReceived ( int iRecID, CVector vecbyMesBodyData, CHostAddress RecHostAddr ) @@ -1572,7 +1617,7 @@ bool CServer::PutAudioData ( const CVector& vecbyRecBuf, const int iNum // Get channel ID ------------------------------------------------------ // check address - iCurChanID = FindChannel ( HostAdr, true ); + iCurChanID = FindChannel ( HostAdr, true /* allow new */ ); // If channel is valid or new, put received audio data in jitter buffer ---------------------------- if ( iCurChanID != INVALID_CHANNEL_ID ) diff --git a/src/server.h b/src/server.h index efa34ba5c8..79c694a295 100644 --- a/src/server.h +++ b/src/server.h @@ -306,8 +306,13 @@ class CServer : public QObject, public CServerSlots // do not use the vector class since CChannel does not have appropriate // copy constructor/operator - CChannel vecChannels[MAX_NUM_CHANNELS]; - int iMaxNumChannels; + CChannel vecChannels[MAX_NUM_CHANNELS]; + int iMaxNumChannels; + + int iCurNumChannels; + int vecChannelOrder[MAX_NUM_CHANNELS]; + QMutex MutexChanOrder; + CProtocol ConnLessProtocol; QMutex Mutex; QMutex MutexWelcomeMessage; diff --git a/src/util.cpp b/src/util.cpp index 69b351047e..39674f007b 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -877,6 +877,50 @@ bool NetworkUtil::IsPrivateNetworkIP ( const QHostAddress& qhAddr ) return false; } +// CHostAddress methods +// Compare() - compare two CHostAddress objects, and return an ordering between them: +// 0 - they are equal +// <0 - this comes before other +// >0 - this comes after other +// The order is not important, so long as it is consistent, for use in a binary search. + +int CHostAddress::Compare ( const CHostAddress& other ) const +{ + // compare port first, as it is cheap, and clients will often use random ports + + if ( iPort != other.iPort ) + { + return (int) iPort - (int) other.iPort; + } + + // compare protocols before addresses + + QAbstractSocket::NetworkLayerProtocol thisProto = InetAddr.protocol(); + QAbstractSocket::NetworkLayerProtocol otherProto = other.InetAddr.protocol(); + + if ( thisProto != otherProto ) + { + return (int) thisProto - (int) otherProto; + } + + // now we know both addresses are the same protocol + + if ( thisProto == QAbstractSocket::IPv6Protocol ) + { + // compare IPv6 addresses + Q_IPV6ADDR thisAddr = InetAddr.toIPv6Address(); + Q_IPV6ADDR otherAddr = other.InetAddr.toIPv6Address(); + + return memcmp ( &thisAddr, &otherAddr, sizeof ( Q_IPV6ADDR ) ); + } + + // compare IPv4 addresses + quint32 thisAddr = InetAddr.toIPv4Address(); + quint32 otherAddr = other.InetAddr.toIPv4Address(); + + return (int) thisAddr - (int) otherAddr; +} + // Instrument picture data base ------------------------------------------------ CVector& CInstPictures::GetTable ( const bool bReGenerateTable ) { diff --git a/src/util.h b/src/util.h index 285ab1ab31..8ab704a163 100644 --- a/src/util.h +++ b/src/util.h @@ -720,6 +720,8 @@ class CHostAddress // compare operator bool operator== ( const CHostAddress& CompAddr ) const { return ( ( CompAddr.InetAddr == InetAddr ) && ( CompAddr.iPort == iPort ) ); } + int Compare ( const CHostAddress& other ) const; + QString toString ( const EStringMode eStringMode = SM_IP_PORT ) const { QString strReturn = InetAddr.toString(); From f41b5099c4e5157d6a932d6369c09c23eb1d080f Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Thu, 26 Aug 2021 22:43:55 +0100 Subject: [PATCH 4/5] Add DumpChannels() method to verify channel ordering. --- src/server.cpp | 22 ++++++++++++++++++++++ src/server.h | 1 + 2 files changed, 23 insertions(+) diff --git a/src/server.cpp b/src/server.cpp index 84f199dd80..07e4ce3fd5 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1533,6 +1533,8 @@ int CServer::FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew ) // insert the new channel ID in the correct place vecChannelOrder[i] = iNewChanID; + DumpChannels ( __FUNCTION__ ); + return iNewChanID; } @@ -1580,6 +1582,9 @@ void CServer::FreeChannel ( const int iCurChanID ) } // put deleted channel at the end ready for re-use vecChannelOrder[i] = iCurChanID; + + DumpChannels ( __FUNCTION__ ); + return; } } @@ -1587,6 +1592,23 @@ void CServer::FreeChannel ( const int iCurChanID ) qWarning() << "FreeChannel() called with invalid channel ID"; } +void CServer::DumpChannels ( const QString& title ) +{ + qDebug() << qUtf8Printable ( title ); + + for ( int i = 0; i < iMaxNumChannels; i++ ) + { + int iChanID = vecChannelOrder[i]; + + if ( i == iCurNumChannels ) + { + qDebug() << "----------"; + } + + qDebug() << qUtf8Printable ( QString ( "%1: [%2] %3" ).arg ( i, 3 ).arg ( iChanID ).arg ( vecChannels[iChanID].GetAddress().toString() ) ); + } +} + void CServer::OnProtcolCLMessageReceived ( int iRecID, CVector vecbyMesBodyData, CHostAddress RecHostAddr ) { QMutexLocker locker ( &Mutex ); diff --git a/src/server.h b/src/server.h index 79c694a295..1f10747b00 100644 --- a/src/server.h +++ b/src/server.h @@ -260,6 +260,7 @@ class CServer : public QObject, public CServerSlots int FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew = false ); void InitChannel ( const int iNewChanID, const CHostAddress& InetAddr ); void FreeChannel ( const int iCurChanID ); + void DumpChannels ( const QString& title ); int GetNumberOfConnectedClients(); CVector CreateChannelList(); From be326c048235f18f6619f36abe367688dae68c2e Mon Sep 17 00:00:00 2001 From: Tony Mountifield Date: Thu, 26 Aug 2021 22:45:25 +0100 Subject: [PATCH 5/5] Remove calls to DumpChannels() but keep method available. --- src/server.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server.cpp b/src/server.cpp index 07e4ce3fd5..504e736994 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -1533,7 +1533,7 @@ int CServer::FindChannel ( const CHostAddress& CheckAddr, const bool bAllowNew ) // insert the new channel ID in the correct place vecChannelOrder[i] = iNewChanID; - DumpChannels ( __FUNCTION__ ); + // DumpChannels ( __FUNCTION__ ); return iNewChanID; } @@ -1583,7 +1583,7 @@ void CServer::FreeChannel ( const int iCurChanID ) // put deleted channel at the end ready for re-use vecChannelOrder[i] = iCurChanID; - DumpChannels ( __FUNCTION__ ); + // DumpChannels ( __FUNCTION__ ); return; }