Skip to content

Improve channel lookup and logging #2048

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

Merged
softins merged 2 commits into from
Sep 29, 2021
Merged

Improve channel lookup and logging #2048

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
fc77b94
Improve comparison of IPv4 addresses.
softins Sep 27, 2021
6287974
Improve logging of active channel count
softins Sep 27, 2021
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
4 changes: 2 additions & 2 deletions src/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ void CServer::SendProtMessage ( int iChID, CVector<uint8_t> vecMessage )
Socket.SendPacket ( vecMessage, vecChannels[iChID].GetAddress() );
}

void CServer::OnNewConnection ( int iChID, CHostAddress RecHostAddr )
void CServer::OnNewConnection ( int iChID, int iTotChans, CHostAddress RecHostAddr )
{
QMutexLocker locker ( &Mutex );

Expand Down Expand Up @@ -637,7 +637,7 @@ void CServer::OnNewConnection ( int iChID, CHostAddress RecHostAddr )
DoubleFrameSizeConvBufOut[iChID].Reset();

// logging of new connected channel
Logging.AddNewConnection ( RecHostAddr.InetAddr, GetNumberOfConnectedClients() );
Logging.AddNewConnection ( RecHostAddr.InetAddr, iTotChans );
}

void CServer::OnServerFull ( CHostAddress RecHostAddr )
Expand Down
5 changes: 3 additions & 2 deletions src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,8 @@ class CServer : public QObject, public CServerSlots<MAX_NUM_CHANNELS>

bool PutAudioData ( const CVector<uint8_t>& vecbyRecBuf, const int iNumBytesRead, const CHostAddress& HostAdr, int& iCurChanID );

int GetNumberOfConnectedClients();

void GetConCliParam ( CVector<CHostAddress>& vecHostAddresses,
CVector<QString>& vecsName,
CVector<int>& veciJitBufNumFrames,
Expand Down Expand Up @@ -261,7 +263,6 @@ class CServer : public QObject, public CServerSlots<MAX_NUM_CHANNELS>
void InitChannel ( const int iNewChanID, const CHostAddress& InetAddr );
void FreeChannel ( const int iCurChanID );
void DumpChannels ( const QString& title );
int GetNumberOfConnectedClients();
CVector<CChannelInfo> CreateChannelList();

virtual void CreateAndSendChanListForAllConChannels();
Expand Down Expand Up @@ -413,7 +414,7 @@ class CServer : public QObject, public CServerSlots<MAX_NUM_CHANNELS>
public slots:
void OnTimer();

void OnNewConnection ( int iChID, CHostAddress RecHostAddr );
void OnNewConnection ( int iChID, int iTotChans, CHostAddress RecHostAddr );

void OnServerFull ( CHostAddress RecHostAddr );

Expand Down
7 changes: 5 additions & 2 deletions src/socket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ CSocket::CSocket ( CServer* pNServP, const quint16 iPortNumber, const quint16 iQ

QObject::connect ( this, &CSocket::ProtcolCLMessageReceived, pServer, &CServer::OnProtcolCLMessageReceived );

QObject::connect ( this, static_cast<void ( CSocket::* ) ( int, CHostAddress )> ( &CSocket::NewConnection ), pServer, &CServer::OnNewConnection );
QObject::connect ( this,
static_cast<void ( CSocket::* ) ( int, int, CHostAddress )> ( &CSocket::NewConnection ),
pServer,
&CServer::OnNewConnection );

QObject::connect ( this, &CSocket::ServerFull, pServer, &CServer::OnServerFull );
}
Expand Down Expand Up @@ -463,7 +466,7 @@ void CSocket::OnDataReceived()
if ( pServer->PutAudioData ( vecbyRecBuf, iNumBytesRead, RecHostAddr, iCurChanID ) )
{
// we have a new connection, emit a signal
emit NewConnection ( iCurChanID, RecHostAddr );
emit NewConnection ( iCurChanID, pServer->GetNumberOfConnectedClients(), RecHostAddr );

// this was an audio packet, start server if it is in sleep mode
if ( !pServer->IsRunning() )
Expand Down
2 changes: 1 addition & 1 deletion src/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class CSocket : public QObject
signals:
void NewConnection(); // for the client

void NewConnection ( int iChID,
void NewConnection ( int iChID, int iTotChans,
CHostAddress RecHostAddr ); // for the server

void ServerFull ( CHostAddress RecHostAddr );
Expand Down
2 changes: 1 addition & 1 deletion src/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,7 @@ int CHostAddress::Compare ( const CHostAddress& other ) const
quint32 thisAddr = InetAddr.toIPv4Address();
quint32 otherAddr = other.InetAddr.toIPv4Address();

return (int) thisAddr - (int) otherAddr;
return thisAddr < otherAddr ? -1 : thisAddr > otherAddr ? 1 : 0;
Copy link
Copy Markdown
Contributor

@dtinth dtinth Sep 27, 2021

Choose a reason for hiding this comment

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

I saw that in IPv6 branch uses memcmp, I suppose that can be used as well for consistency?

Copy link
Copy Markdown
Collaborator

@pljones pljones Sep 27, 2021

Choose a reason for hiding this comment

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

That would probably make more logical sense.

Copy link
Copy Markdown
Member Author

@softins softins Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

IPv6 uses memcmp() because the addresses are 16 bytes long. IPv4 addresses can be compared whole as they fit a 32-bit integer, so that seems more efficient than memcmp() which would be comparing a byte at a time (I assume).

Copy link
Copy Markdown
Collaborator

@pljones pljones Sep 28, 2021

Choose a reason for hiding this comment

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

I'd expect memcmp to be optimised for the platform its running on and the data sizes being compared.

Copy link
Copy Markdown
Contributor

@dtinth dtinth Sep 28, 2021

Choose a reason for hiding this comment

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

Here’s some quick and dirty benchmark. I am using Google Benchmark library (apt install libbenchmark-dev):

#include <benchmark/benchmark.h>
#include <string.h>
#include <stdlib.h>

static void BM_ManualComparison(benchmark::State &state)
{
    srand(1);
    for (auto _ : state)
    {
        state.PauseTiming();
        unsigned int a = rand();
        unsigned int b = rand();
        state.ResumeTiming();
        benchmark::DoNotOptimize(a < b ? -1 : a > b ? 1 : 0);
    }
}
BENCHMARK(BM_ManualComparison);

static void BM_Memcmp(benchmark::State &state)
{
    srand(1);
    for (auto _ : state)
    {
        state.PauseTiming();
        unsigned int a = rand();
        unsigned int b = rand();
        state.ResumeTiming();
        benchmark::DoNotOptimize(memcmp(&a, &b, sizeof(unsigned int)));
    }
}
BENCHMARK(BM_Memcmp);

BENCHMARK_MAIN();

Running benchmark with -O3 resulted in roughly the same amount of CPU time per iteration. Your results may vary:

$ g++ -lbenchmark bench.cpp -O3 && ./a.out

Running ./a.out
Run on (1 X 2900 MHz CPU )
CPU Caches:
  L1 Data 64K (x1)
  L1 Instruction 64K (x1)
  L2 Unified 512K (x1)
  L3 Unified 16384K (x1)
***WARNING*** Library was built as DEBUG. Timings may be affected.
-----------------------------------------------------------
Benchmark                    Time           CPU Iterations
-----------------------------------------------------------
BM_ManualComparison        329 ns        325 ns    2138302
BM_Memcmp                  326 ns        325 ns    2130022

Copy link
Copy Markdown
Member Author

@softins softins Sep 28, 2021

Choose a reason for hiding this comment

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

That's certainly an interesting result! :)

On a little-endian architecture such as Intel, the comparison by memcmp() must necessarily give different results than a direct comparison of a pair of int or unsigned int variables. In this particular application that doesn't actually matter, as the order is immaterial, provided it is consistent.

Personally, in this instance, I prefer the comparison of unsigned int directly (as in the current PR) rather than memcmp(), but not strongly enough to argue about it. If the consensus is to prefer memcmp(), then fair enough.

I repeated the test on my RPi4:

tony@pi:~/benchmark $ g++ -lbenchmark bench.cpp -O3 && ./a.out
2021-09-28 17:54:48
Running ./a.out
Run on (4 X 1500 MHz CPU s)
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
***WARNING*** Library was built as DEBUG. Timings may be affected.
-----------------------------------------------------------
Benchmark                    Time           CPU Iterations
-----------------------------------------------------------
BM_ManualComparison       2458 ns       2432 ns     285846
BM_Memcmp                 2443 ns       2425 ns     289761
tony@pi:~/benchmark $ ./a.out
2021-09-28 17:55:26
Running ./a.out
Run on (4 X 1500 MHz CPU s)
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
***WARNING*** Library was built as DEBUG. Timings may be affected.
-----------------------------------------------------------
Benchmark                    Time           CPU Iterations
-----------------------------------------------------------
BM_ManualComparison       2464 ns       2434 ns     288050
BM_Memcmp                 2487 ns       2463 ns     286765

So again there is not much to choose, and the two runs were different in the opposite way!

Copy link
Copy Markdown
Contributor

@dtinth dtinth Sep 28, 2021

Choose a reason for hiding this comment

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

Personally, I am fine with either way; I don't have a strong opinion — just pointing out that using memcmp will make both IPv4 and IPv6 code more consistent with negligible performance penalty, although consistency in this specific case may not be so important to warrant a change.

(By the way I am already deploying the code with patches from this PR, as it is right now, to my servers for testing tomorrow.)

}

// Instrument picture data base ------------------------------------------------
Expand Down