Server: avoid per-frame deep copy of audio buffers in CreateLevelsForAllConChannels

[mcfnord]

What

CServer::CreateLevelsForAllConChannels() declares its vecvecsData parameter as

const CVector<CVector<int16_t>> vecvecsData   // by value

This PR changes it to const CVector<CVector<int16_t>>& (by const reference), matching the neighbouring vecNumAudioChannels parameter.

Why

The function is called from CServer::OnTimer() — the realtime audio timer thread — on every frame in which at least one client is connected:

const bool bSendChannelLevels = CreateLevelsForAllConChannels ( iNumClients, vecNumAudioChannels, vecvecsData, vecChannelLevels );

Because CVector derives from std::vector, passing by value deep-copies the entire pre-allocated member vector every frame. vecvecsData is initialized in the CServer constructor to iMaxNumChannels (the -u setting) worst-case stereo buffers and is never shrunk, so the copy always covers the full configured channel limit — even a nearly empty server pays it in full. This both allocates on the audio thread (which the codebase otherwise takes care to avoid — see the "no memory must be allocated" comments in the CServer constructor) and burns memory bandwidth.

A small allocation-counting reproduction of the exact copy (150 channels = -u 150, 2×128 int16 per buffer, 375 frames/s) measured ~56,600 heap allocations/s and ~30 MB/s copied, purely from the by-value parameter; at the default -u 10 it is still ~4,100 allocations/s and ~2 MB/s. The copy is not elided at -O2.

Change

One parameter, declaration + definition — no behavioural change. The function only reads vecvecsData.

Testing

• Headless build (CONFIG+=headless, Qt 5.15) is clean.
• Server starts, runs, and shuts down cleanly on SIGTERM (OnHandledSignal: 15).
• clang-format-14 applied to the touched files.

See #3804 for the analysis.

[ann0see]

Seems sensible. We'd need to check if there's really no reason for a copy though.

[mcfnord]

Not sure how to parse the scope of const here, but it might be a clue that perhaps there's no reason to copy the bits.

[softins]

Just been looking at this, and studying the life-cycle of vecvecsData.

1. vecvecsData is a protected member of class CServer. There is also a vecvecsData2 which is specifically for delay panning, and is not relevant to this discussion.

2. The outer vector and inner vectors of vecvecsData are sized and allocated within the CServer constructor, and their sizes do not change after that.

3. vecvecsData[n] is written to only within CServer::DecodeReceiveData(), which is called by the static callback method CServer::DecodeReceiveDataBlocks(). That callback was in turn called from either:

   • CServer::OnTimer(), if multi-threading (-T) is not enabled, or
   • a thread in the threadpool, if multi-threading is enabled.

4. CServer::OnTimer() is called via a signal and slot from CServer::HighPrecisionTimer. A normal timer calls its callback from the main application thread, but I'm not sure whether a high precision timer uses its own thread instead?

5. The method CServer::CreateLevelsForAllConChannels(), which is the subject of this PR, is called within CServer::OnTimer(). It is called with the following arguments:

   • iNumClients, by value, which is a variable local to the caller
   • vecNumAudioChannels, by reference, which is a member of CServer
   • vecvecsData, by value, which is a member of CServer
   • vecChannelLevels, by reference to receive the calculated values, which is a member of CServer.

   But since CreateLevelsForAllConChannels() is also a member of CServer, it has direct access to the protected or private members already, meaning there was no need for it to receive them as parameters! Certainly this applies to the two arguments passed by reference, as it is therefore accessing them directly via the reference.

   The situation with vecvecsData is currently different, because it is passed by value. This results in a copy being made of the outer vector and all its inner vectors, all iMaxNumChannels of them (iMaxNumChannels is set on the command line with -u, defaulting to 10 if not specified). Each inner vector contains 128 bytes of data, whether used or not.

6. In CServer::CreateLevelsForAllConChannels(), the only use made of vecvecsData is to pass each contained vector by reference to CChannel::UpdateAndGetLevelForMeterdB(), which reads the data to calculate a dB value for the sound level.

7. So conceptually, vecvecsData could be passed to CServer::CreateLevelsForAllConChannels() by reference, and in fact the three vec parameters could be dispensed with and the method could access the class members directly. This latter seems to be most logical.

8. If multi-threading is not enabled, then the vectors within vecvecsData are both written to and read from within CServer::OnTimer(), so there should be no contention.

9. However, it multi-threading is enabled, the inner vectors of vecvecsData will be updated by a different thread, which could be on a different CPU or core. This could conceivably happen while CServer::CreateLevelsForAllConChannels() is processing it. The copying of vecvecsData when passing by value probably mitigates this, but I couldn't be sure it guards absolutely against another thread updating one of the inner vectors while it is being copied.

10. I think logically, we don't want to be making deep copies of vecvecsData, but we do need to guard against concurrent access to its constituent vectors. I initially wondered whether we could change vecvecsData from CVector<CVector<int16_t>> to CVector<std::atomic<CVector<int16_t>>>. But apparently, std::atomic<T> can only be used if the type T is trivially copyable, and does not contain dynamic elements such as std::vector.

11. Otherwise, we might need to introduce mutex protection of either vecvecsData or more finely-grained of its constituent vectors?

12. Instead of passing the whole vecvecsData by value, maybe it would be better in step 6 above to pass the inner vector by value, or make a temporary copy of it with mutex protection and then pass the copy by reference. This would make any locking much more finely grained, and copying to be only necessary for channels that are currently is use.

Comments?

[softins]

OK, I've now just read the analysis on #3804, which I'd overlooked before. It largely aligns with what I came up with above (without using AI!), but reassures me that the writing and reading are protected against concurrent access.

If that's the case, I would propose removing the vec parameters from CServer::CreateLevelsForAllConChannels() and replacing them with direct access to the relevant class members.