Server: per-frame deep copy of all client audio buffers on the realtime audio thread

[mcfnord]

Summary

CServer::CreateLevelsForAllConChannels() takes its vecvecsData argument by value:

jamulus/src/server.h

Lines 243 to 246 in 6b7a0ad

	bool CreateLevelsForAllConChannels ( const int iNumClients,
	const CVector<int>& vecNumAudioChannels,
	const CVector<CVector<int16_t>> vecvecsData,
	CVector<uint16_t>& vecLevelsOut );

bool CreateLevelsForAllConChannels ( const int                       iNumClients,
                                     const CVector<int>&             vecNumAudioChannels,
                                     const CVector<CVector<int16_t>> vecvecsData,   // <-- by value
                                     CVector<uint16_t>&              vecLevelsOut );

It is called from CServer::OnTimer() — the per-frame realtime audio processing routine — on every frame that has connected clients:

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

Since CVector<T> derives from std::vector<T>, the by-value parameter deep-copies the entire pre-allocated member vector, every audio frame. Note that vecvecsData is initialized in the CServer constructor to iMaxNumChannels (the -u setting) inner buffers of worst-case stereo size and is never shrunk — so the copy always covers the full configured channel limit, regardless of how many clients are actually connected. A server run with a high -u pays the full cost even when nearly empty.

This allocates in the realtime audio processing routine — which the server otherwise deliberately avoids (see the "no memory must be allocated" worst-case pre-allocation in the CServer constructor, directly above the vecvecsData.Init call).

Data flow and threading

(Context requested in the comments: where the source vector is populated, where the data is processed, and the subsequent flow.)

1. Where vecvecsData is populated — thread and trigger

vecvecsData is a pre-allocated CServer member. The constructor sizes it for the worst case (iMaxNumChannels inner buffers of 2 * DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES samples each), under the explicit invariant that the realtime routine must not allocate:

jamulus/src/server.cpp

Lines 168 to 195 in 6b7a0ad

	// To avoid audio clitches, in the entire realtime timer audio processing
	// routine including the ProcessData no memory must be allocated. Since we
	// do not know the required sizes for the vectors, we allocate memory for
	// the worst case here:
	// allocate worst case memory for the temporary vectors
	vecChanIDsCurConChan.Init ( iMaxNumChannels );
	vecvecfGains.Init ( iMaxNumChannels );
	vecvecfPannings.Init ( iMaxNumChannels );
	vecvecsData.Init ( iMaxNumChannels );
	vecvecsData2.Init ( iMaxNumChannels );
	vecvecsSendData.Init ( iMaxNumChannels );
	vecvecfIntermediateProcBuf.Init ( iMaxNumChannels );
	vecvecbyCodedData.Init ( iMaxNumChannels );
	vecNumAudioChannels.Init ( iMaxNumChannels );
	vecNumFrameSizeConvBlocks.Init ( iMaxNumChannels );
	vecUseDoubleSysFraSizeConvBuf.Init ( iMaxNumChannels );
	vecAudioComprType.Init ( iMaxNumChannels );
	for ( i = 0; i < iMaxNumChannels; i++ )
	{
	// init vectors storing information of all channels
	vecvecfGains[i].Init ( iMaxNumChannels );
	vecvecfPannings[i].Init ( iMaxNumChannels );
	// we always use stereo audio buffers (which is the worst case)
	vecvecsData[i].Init ( 2 /* stereo */ * DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES /* worst case buffer size */ );
	vecvecsData2[i].Init ( 2 /* stereo */ * DOUBLE_SYSTEM_FRAME_SIZE_SAMPLES /* worst case buffer size */ );

Incoming network audio does not touch it directly: packets arrive on the socket thread and are placed into each channel's jitter buffer (CChannel). The socket thread never writes vecvecsData.

The trigger that populates it is the periodic frame tick. CHighPrecisionTimer (non-Windows: a dedicated QThread started at TimeCriticalPriority, clock_nanosleep absolute-deadline loop) emits timeout once per server frame — 64 or 128 samples at 48 kHz, i.e. every ~1.33 ms / ~2.67 ms:

jamulus/src/util.cpp

Lines 345 to 377 in 6b7a0ad

	void CHighPrecisionTimer::run()
	{
	// loop until the thread shall be terminated
	while ( bRun )
	{
	// call processing routine by fireing signal
	//### TODO: BEGIN ###//
	// by emit a signal we leave the high priority thread -> maybe use some
	// other connection type to have something like a true callback, e.g.
	// "Qt::DirectConnection" -> Can this work?
	emit timeout();
	//### TODO: END ###//
	// now wait until the next buffer shall be processed (we
	// use the "increment method" to make sure we do not introduce
	// a timing drift)
	# if defined( __APPLE__ ) || defined( __MACOSX )
	mach_wait_until ( NextEnd );
	NextEnd += Delay;
	# else
	clock_nanosleep ( CLOCK_MONOTONIC, TIMER_ABSTIME, &NextEnd, NULL );
	NextEnd.tv_nsec += Delay;
	if ( NextEnd.tv_nsec >= 1000000000L )
	{
	NextEnd.tv_sec++;
	NextEnd.tv_nsec -= 1000000000L;
	}
	# endif
	}
	}

Because the signal crosses threads, the default (queued) connection at server.cpp:268 delivers CServer::OnTimer() on the thread CServer lives in (the Qt main/event thread of the server process). OnTimer() is the entire per-frame pipeline and must complete within one frame period.

Inside OnTimer(), under QMutexLocker ( &Mutex ), DecodeReceiveData() pulls the coded frame for each connected channel from its jitter buffer (vecChannels[id].GetData(...)) and writes decoded PCM into vecvecsData[iChanCnt] — opus_custom_decode for OPUS, memcpy for raw audio, memset silence for lost packets:

jamulus/src/server.cpp

Lines 981 to 1002 in 6b7a0ad

	if ( !bIsRawAudio )
	{
	// OPUS decode received data stream
	if ( CurOpusDecoder != nullptr )
	{
	iUnused = opus_custom_decode ( CurOpusDecoder,
	pCurCodedData,
	iCeltNumCodedBytes,
	&vecvecsData[iChanCnt][iOffset],
	iClientFrameSizeSamples );
	}
	}
	else if ( pCurCodedData != nullptr )
	{
	// copy received raw data stream
	memcpy ( &vecvecsData[iChanCnt][iOffset], pCurCodedData, iCeltNumCodedBytes );
	}
	else
	{
	// lost packet - fill with silence
	memset ( &vecvecsData[iChanCnt][iOffset], 0, iCeltNumCodedBytes );
	}

With multithreading enabled the decode fans out over the thread pool (DecodeReceiveDataBlocks), but OnTimer() waits on all futures before leaving that block (server.cpp:713-718). So by the time execution reaches the call in question, population is complete and no other thread is writing to vecvecsData.

2. Where the data is processed — thread

The call at issue happens in the same OnTimer() invocation, on the same thread, immediately after the decode phase:

jamulus/src/server.cpp

Lines 732 to 735 in 6b7a0ad

	if ( iNumClients > 0 )
	{
	// calculate levels for all connected clients
	const bool bSendChannelLevels = CreateLevelsForAllConChannels ( iNumClients, vecNumAudioChannels, vecvecsData, vecChannelLevels );

CreateLevelsForAllConChannels() runs inline (never on the pool) and only reads the data: once every CHANNEL_LEVEL_UPDATE_INTERVAL (200) frames it passes vecvecsData[j] — as const CVector<short>& — to CChannel::UpdateAndGetLevelForMeterdB() to compute each client's meter level; on the other 199 frames the body does nothing but iFrameCount++:

jamulus/src/server.cpp

Lines 1676 to 1711 in 6b7a0ad

	bool CServer::CreateLevelsForAllConChannels ( const int iNumClients,
	const CVector<int>& vecNumAudioChannels,
	const CVector<CVector<int16_t>> vecvecsData,
	CVector<uint16_t>& vecLevelsOut )
	{
	bool bLevelsWereUpdated = false;
	// low frequency updates
	if ( iFrameCount > CHANNEL_LEVEL_UPDATE_INTERVAL )
	{
	iFrameCount = 0;
	bLevelsWereUpdated = true;
	for ( int j = 0; j < iNumClients; j++ )
	{
	// update and get signal level for meter in dB for each channel
	const double dCurSigLevelForMeterdB = vecChannels[vecChanIDsCurConChan[j]].UpdateAndGetLevelForMeterdB ( vecvecsData[j],
	iServerFrameSizeSamples,
	vecNumAudioChannels[j] > 1 );
	// map value to integer for transmission via the protocol (4 bit available)
	vecLevelsOut[j] = static_cast<uint16_t> ( std::ceil ( dCurSigLevelForMeterdB ) );
	}
	}
	// increment the frame counter needed for low frequency update trigger
	iFrameCount++;
	if ( bUseDoubleSystemFrameSize )
	{
	// additional increment needed for double frame size to get to the same time interval
	iFrameCount++;
	}
	return bLevelsWereUpdated;
	}

The by-value parameter, however, deep-copies all iMaxNumChannels inner buffers on every call — including the 199-in-200 frames where the copy is never even read.

Since the decode futures have already been joined and the socket thread only writes to the jitter buffers, there is no concurrent writer at the call site — the copy cannot be serving as a thread-safety snapshot. The existing implementation gains nothing from it.

3. Subsequent flow

• Levels out: when the interval fires, vecLevelsOut (= vecChannelLevels) is sent to every connected client via ConnLessProtocol.CreateCLChannelLevelListMes() (server.cpp:746-749) — the channel-level list protocol message that drives the client-side input meters.
• vecvecsData afterwards, same tick: the very same member vector is then consumed by the rest of the frame pipeline:
  • recording, if enabled: emit AudioFrame ( ..., vecvecsData[iChanCnt] ) (server.cpp:754-758) — here the queued signal makes its own copy, which is exactly the one place a cross-thread copy is genuinely needed;
  • mixing/encode/transmit: MixEncodeTransmitData() reads it via const CVector<int16_t>& vecsData = vecvecsData[j]; (server.cpp:1037, 1099-1100) — i.e. the mixer accesses the identical data in the identical tick by const reference, which is the established access pattern for this vector;
  • with --delaypan, the frame is copied element-wise into vecvecsData2 for the next frame (server.cpp:793-802).
• Next tick overwrites the buffers in place; nothing outside OnTimer()'s frame pipeline ever reads vecvecsData.

Impact / measurement

A standalone allocation-counting reproduction of the exact copy (150 channels, i.e. -u 150 = MAX_NUM_CHANNELS; 2×128 int16_t per buffer; ~375 frames/s at 128-sample double frame size) measured:

• ~56,600 heap allocations per second
• ~30 MB per second copied

purely from the by-value parameter. The copy is not elided at -O2. At the default -u 10 the same mechanism still costs ~4,100 allocations/s and ~2 MB/s. On any server with at least one client connected this is continuous, avoidable allocation and memory-bandwidth pressure on the most timing-critical code path in the server.

Fix

Pass vecvecsData by const reference (the function only reads it), matching the adjacent vecNumAudioChannels parameter and the const-reference access the mixer already uses on the same data in the same frame. Proposed in #3803.

The neighbouring vecNumAudioChannels is already const&, so this is just an oversight on the one parameter.

[pljones]

This should explain

• where the source vector is populated: which thread, in response to what trigger
• where the data gets processed, including thread
• what the subsequent flow is

The existing issue basically asserts there is a problem without supporting context to say why the existing implementation isn't the only solution.

[mcfnord]

Updated the issue with a "Data flow and threading" section covering the three points: where vecvecsData is populated (decode phase of the same OnTimer() tick, after the thread-pool futures are joined), where it is processed (same invocation, same thread, read-only), and the subsequent flow (level list message out; then the mixer reads the same vector by const reference in the same tick). Key point: there is no concurrent writer at the call site, so the by-value copy is not acting as a snapshot — the existing implementation gains nothing from it.

[pljones]

Ah, right... My guess is the intention was the levels would not be calculated inside the mutex - indeed not on that thread. They're a low frequency UI nicety, after all, not essential processing. That would have needed a copy, though...

I'm not sure whether to go with this for now or ask for a more in depth analysis of the problem.

[softins]

I've just commented on #3803, and agree with the conclusions above. Although as I mentioned in my comments, the passing of references is not even required, as the called method has direct access to the class members.

[softins]

@mcfnord I would be interested in which AI tool(s) you used to produce the very good analysis above?

[ann0see]

Probably https://www.anthropic.com/claude/fable Extremely expensive...

[mcfnord]

Claude Code with Fable on High effort, or X-High in this case. There's a deep discount for a few more days so I'm ripping through whatever I can find. This experiment with our repo has been dizzying and I rely on your feedback about whether it's useful. It navigates impressively among existing github Issues and PRs. But I am thinking now that it's easy to create a PR, hard to get one merged. "The human is the bottleneck" looks more and more apparent. I agree with @pljones that we should just ask for deeper analysis when we feel it would help. so let's do that now...

[softins]

Claude Code with Fable on High effort, or X-High in this case. There's a deep discount for a few more days so I'm ripping through whatever I can find. This experiment with our repo has been dizzying and I rely on your feedback about whether it's useful. It navigates impressively among existing github Issues and PRs.

It's useful stuff, certainly, and gives a lot of good info to work through.

But I am thinking now that it's easy to create a PR, hard to get one merged. "The human is the bottleneck" looks more and more apparent.

Not hard, necessarily, but it all takes time to review, understand and test. All the team also have Real Lives™. Most have jobs, and even I, who am retired, have family and other demands!

I agree with @pljones that we should just ask for deeper analysis when we feel it would help. so let's do that now...

[mcfnord]

Following up on the request for a deeper analysis. Three questions were left open in the two threads: why is this parameter a copy at all (@ann0see traced the function to #42 and CC'd @pljones), whether the copy is protecting anything under -T (@softins's point 9), and what shape the fix should take (const ref as in #3803, direct member access as @softins proposes, or off-thread computation as @pljones sketched). Below: the git archaeology, a memory-model review, and a live A/B measurement on running servers. Each section stands alone.

1. Why it's a copy: the history

The per-frame copy was never a decision anyone made. It is the interaction of four commits over six months, each individually reasonable:

date	commit	change	how often the copy fires
2020‑04‑04	a97ac9aa (#42)	function introduced — called after Mutex.unlock() (L1005 vs L1026), behind two call-site gates: the 200-frame interval and ≥1 client having opted into meters	≤ ~2 calls/s; zero if no client enabled the setting
2020‑06‑07	efc24ffd	interval gate moved inside the function (to support the new "only send if changed" bool return); the function is now called every frame	every frame, if a client opted in
2020‑06‑25	70cfdfc9	level math delegated to CChannel::UpdateAndGetLevelForMeterdB ( const CVector<short>&, ... ) — the copy's contents are only ever read through a const reference	—
2020‑10‑05	31de2401	multithreaded decode added; futures joined before the level call	—
2020‑10‑06	079f9751 (#638)	"Display Channel Levels" client setting removed → levels always requested → the last gate is gone	every frame with ≥1 client connected — the situation on main today

In #42 the signature was already const CVector<CVector<int16_t>> by value, directly under const CVector<int>& — but behind two gates at ≤2 calls/s the copy cost effectively nothing, so there was no reason for it to draw review attention. The later commits multiplied the call rate by ~200 (and removed the opt-in) without touching the signature, so no individual diff ever showed a copy getting hot.

On @pljones's recollection — "my guess is the intention was the levels would not be calculated inside the mutex — indeed not on that thread" — the archaeology half-confirms it: the call has been outside the mutex from day one (that intent is visible in the code), but the off-thread part was never built — the function has always run inline on the timer thread. A copy would genuinely have been required for the off-thread version. So the by-value parameter reads as insurance for a design that never materialized, which silently became expensive when the calling pattern changed underneath it.

2. The copy is not thread-safety — under any era's threading model

Writers of vecvecsData, current main (6b7a0adf):

• The only writer is the decode phase (DecodeReceiveData): the timer thread itself without -T, or pool threads with -T. Pool threads write disjoint index ranges (iStartChanCnt..iStopChanCnt), and each vecvecsData[i] is a distinct std::vector with its own heap block — no shared bytes, so @ann0see's disjoint-write condition holds at object granularity.
• The "final synchronization after all threads have finished" that @ann0see asked about exists: OnTimer() waits on every future inside the QMutexLocker block (server.cpp#L711-L718). CThreadPool::enqueue returns a real std::future from a std::packaged_task (threadpool.h#L122-L128), and std::future::wait() synchronizes-with task completion per the C++ memory model — every pool write happens-before everything after the wait loop. CreateLevelsForAllConChannels() is called after that, on the same thread; the pool is quiescent until the encode fan-out, which starts only after the function returns.
• The socket thread never touches vecvecsData — it writes into each channel's jitter buffer under that channel's own lock (channel.cpp#L553-L616).
• The recorder (the one true cross-thread consumer) already gets its own copy: emit AudioFrame(...) is forwarded into CJamRecorder, which lives in its own QThread (jamcontroller.cpp#L135-L138, #L166), so the queued connection marshals a per-argument copy. The parameter copy contributes nothing to it.
• On @softins's point 4 (timer thread): the tick source is its own thread on non-Windows (CHighPrecisionTimer : public QThread, util.h#L1339-L1341) and a plain QTimer on Windows (#L1313-L1315) — but in both cases the slot runs on the thread CServer lives in (cross-thread signal ⇒ queued delivery). OnTimer() is serialized; it never runs concurrently with itself.

And the principle that settles point 9 ("the copying … probably mitigates this, but I couldn't be sure"): a by-value copy can never substitute for synchronization, because copying is reading. If a concurrent writer existed, the copy constructor itself would read the racing bytes — same data race, same UB, just moved into the parameter. Whatever makes a const-ref read safe (here: the futures join plus program order) is exactly what makes the copy safe, and it is already present. The copy adds zero safety in the multithreaded era, and in the 2020 single-threaded era plain program order already sufficed. It follows that new mutexes or atomics (points 10–12) aren't needed either — there is no race left to guard, and std::atomic<CVector<…>> is ill-formed anyway, as you noted.

3. What the function reads vs. what the parameter copies

On 199 of 200 frames the entire body is iFrameCount++; return false; (server.cpp#L1676-L1711) — yet the parameter deep-copies all iMaxNumChannels worst-case buffers first. At -u 150, 128-sample double frames (375 ticks/s), 2 connected clients:

• data the function actually reads: 2 × 512 B × 1.875/s ≈ 2 KB/s
• data the parameter copies: 151 vectors × 375/s ≈ 28.8 MB/s, plus ~56,600 heap alloc/free pairs/s

A ~15,000:1 ratio of copied to consumed. The copy volume depends only on -u, not on occupancy — one client on a -u 150 server pays the same 28.8 MB/s. (A fully empty server pays nothing: the timer is stopped until a client connects.)

4. Measured live, not just in the standalone repro

Two identical trees of current main (6b7a0adf), one with only this PR's two-line & applied. Same host, same flags, back to back: CONFIG+=headless, GCC 13.3, Qt 5.15.13, glibc, single x86-64 vCPU, Linux 6.8. Server run as -s -n -u <N> -p 22150; two protocol-complete bot clients (gojam/gjstress) streaming a 440 Hz tone over loopback for the whole run. Allocations counted process-wide by an LD_PRELOAD interposer on malloc/calloc/realloc/free; CPU from /proc/<pid>/stat; 60 s steady-state window starting 15 s after connect.

config	binary	allocs/s (whole process)	frees/s	server CPU
-u 150, 2 clients	main	57,785	57,785	15.8 %
-u 150, 2 clients	main + &	1,107	1,107	12.1 %
-u 10, 2 clients	main	5,214	5,214	11.4 %
-u 10, 2 clients	main + &	1,159	1,159	11.4 %

• The measured deltas (56,678/s and 4,055/s) match the predicted (iMaxNumChannels + 1) × 375/s (56,625 and 4,125) within 2 % — the mechanism is exactly the one described, nothing else moved.
• At -u 150, this one parameter was 98 % of all allocator traffic in the server process, and removing it cut server CPU by ~23 % relative at this workload.
• At the default -u 10 the CPU delta is below measurement noise — the honest statement is that small servers waste allocations but not measurable CPU. The problem scales with -u, which is precisely the knob large public servers turn up.
• allocs = frees and RSS was flat in all four runs: pure churn, no leak, no elision by the optimizer.

5. Fix shape

option	verdict
(a) const& (this PR)	Minimal, zero behavioral change, matches the adjacent vecNumAudioChannels parameter and the const-ref access MixEncodeTransmitData() already uses on the same member in the same tick (L1037).
(b) drop the vec params, direct member access (@softins)	The consistent endpoint — the sibling per-frame function MixEncodeTransmitData ( iChanCnt, iNumClients ) already works exactly this way, and the function reads vecChannels/iFrameCount members regardless, so parameter purity isn't real today. Happy to rework #3803 to this if that's the preference; it's a maintainer style call, and (a) is a strict subset of it.
(c) compute levels off-thread (@pljones's original intent)	Not worth building now: the computation itself runs 1.875×/s and costs microseconds (it reads ~1 KB/s). Moving it would require an interval snapshot of connected channels, a queued hand-off, and cross-thread access rules for the per-channel meter state — more machinery than the work it offloads. The expensive thing was never the level calculation; it was the accidental every-frame copy. (If levels ever need to leave this thread, the snapshot should copy only the iNumClients connected buffers, once per interval — ~1 KB per 533 ms, not 28.8 MB/s.)
(d) add locking/atomics	Unnecessary per §2 — there is no concurrent access to protect against, and additions would put new cost on the hot path.

One more datum for confidence: I audited the server for siblings — vecvecsData here is the only by-value CVector audio-buffer parameter anywhere on the per-frame path. The other by-value CVector parameters in server.h/channel.h are protocol slots (SendProtMessage etc.), where Qt queued-connection marshalling makes the copy intentional. So this is the last one of its kind; merging (a) or (b) closes the class.

[mcfnord]

I might be a simple man, but that's astonishing.

[ann0see]

"The human is the bottleneck" looks more and more apparent. I agree with @pljones that we should just ask for deeper analysis when we feel it would help. so let's do that now...

Yes. But it's needed (for now). It should be used for now on the hardest issues and bugs to get deeper understanding - and best comment on those. This helps us triaging and deciding on the final fixes. IMO, there's no need for PRs even (unless the code is to some extent hard) but analysis and root cause findings for any bugs.

For example, it would also be good for multiple non trivial bugs such as:

#3779 , #3776, #3430, #3406, #3383, #3315 (you tried that but probably hard if you do not have any macOS/iOS access), #2993, #2806, #2519, #838 ...

Root cause analysis only, no PRs.

[mcfnord]

I have been puckish about PRs but you're right. Carefully limiting scope and progressing incrementally helps control cost, avoid unnecessary e-toil, and keep things tidy. I bet we can "point the wand" at all of those bugs for an initial review by Fable on Medium effort, and then just ramp up the effort until we feel confident in any proposed fix.

While perhaps not comparable, I am letting AI make decisions about https://github.com/mcfnord/jamulus-ping-join

Every indication is to get the human bottleneck out of the process. I wonder what a fork of this repo would produce if it had broad latitude to resolve bugs and add features with testing by telemetry in my fleet. My own programming approach is much more reckless than practiced here, but I suspect that doesn't matter like it once did.

[ann0see]

We're not - at least currently - and I don't think we ever will - AI only, full vibe coding. It's a very difficult topic and I have been thinking about this the past few months intensively (as I used it for something unrelated privately and am torn between multiple approaches). For Jamulus we should use it definitely - but we need a policy and this policy should say that the human will stay in the loop.

[softins]

I might be a simple man, but that's astonishing.

I agree (with the second part). @mcfnord, could you ask it to rework #3803 with direct member access instead of the vec parameters, as it offered to? It will need to rename vecLevelsOut to vecChannelLevels inside the function, but I'm sure it already knows that!