Replace per-pair gain/pan mutex locking with single snapshot - #293
Replace per-pair gain/pan mutex locking with single snapshot#293ann0see wants to merge 1 commit into
Conversation
Review: correctness, safety, and measurementWhat changed
WhyEvery server frame the audio mix runs an O(N²) nested loop over the connected channels. Each Review findings
MeasurementSetup: headless server pinned to one core (
Result: −4.44 pp (−10.9%) at 50 clients, −3.94 pp (−6.5%) at 100 clients, zero overlap between the groups. The saving is worth it exactly on the large servers that hit the quadratic path; it is within measurement noise on typical small (<15 client) servers. Open questions for reviewers
|
There was a problem hiding this comment.
🤖 AI: The inline comments below are a 7-suggestion set that turns the per-channel snapshot into a compacted fill: GetGainsAndPannings() takes the connected-ID list and writes vecvecfGains[iChanCnt]/vecvecfPannings[iChanCnt] directly, in connected-channel order, under the same single lock. Same lock count as this PR (one per channel per frame), and additionally: iNumClients floats copied per array instead of 150 — the copy no longer grows with MAX_NUM_CHANNELS on small servers; the two snapshot members, their four Init lines and the re-index pass are deleted; the range guard GetGain()/GetPan() had (0 for an ID outside [0,150)) is kept, where the unchecked [vecChanIDsCurConChan[j]] read dropped it; and open question 2 disappears — there is no snapshot buffer left to size. The suggestions only work as a batch (apply all 7 in one commit).
Verified on the full set applied to c4b162d8: headless build, gcc 11.4 -Wall -Wextra, zero warnings, server boots; an A/B harness comparing the new getter against the still-present GetGain()/GetPan() returns byte-identical floats for 37 scrambled in-range IDs and 0 for out-of-range IDs; clang-format-14 -style=file is byte-clean on all four files (control: the pristine head files are also byte-clean, so the check is discriminating).
On open question 1: git grep at c4b162d8 finds zero callers of GetGain/GetPan — removable, and with the compacted fill nothing needs them. No suggestion attached because their lines are outside this PR's diff.
Separate observation, also outside the diff: the remaining per-pair accessor traffic is fade-in. GetFadeInGain() runs twice per channel pair in DecodeReceiveData, and its two inputs are constant for the frame — every writer (PutAudioData, OnProtocolMessageReceived) and OnTimer's decode phase hold CServer::Mutex. A per-frame fade-in vector (one read per connected channel per frame, taken where vecChanIDsCurConChan is built) would make that N reads instead of 2N²−N, and is a natural follow-up to this PR's pattern — now jamulussoftware/jamulus#3945.
One number in the benchmark worth a look before merge: the removed lock count is 2N² per frame — quadrupling from 50 to 100 clients — yet the measured saving stays ~4 pp at both. On a separate rig (12-core x86-64, isolated microbenchmark of only this refresh term: N=150, 64-sample ticks, 18 replicates) the full recompute is 575.9 µs/tick against 41.2 µs for identical arithmetic without the mutex — the mutex is ~93% of the term and scales as the lock count. A per-region timer around the fill-plus-mix block at N=50 and N=100 would show where the missing ~3× went; the per-channel 2×150-float copy, which the compacted fill removes, is one candidate to test. Related measurements on this term: #3895.
| void CChannel::GetGainsAndPannings ( CVector<float>& vecGains, CVector<float>& vecPannings ) | ||
| { | ||
| QMutexLocker locker ( &Mutex ); | ||
|
|
||
| // copy the gain and pan values under a single lock instead of acquiring the | ||
| // mutex once per value | ||
| vecGains = vecfGains; | ||
| vecPannings = vecfPannings; | ||
| } |
There was a problem hiding this comment.
🤖 AI: [2/7] Filling the caller's arrays compacted under the single lock copies iNumClients values per array instead of 150, and keeps the range guard the replaced GetGain()/GetPan() had — the caller's unchecked [vecChanIDsCurConChan[j]] indexing (suggestion 6/7 removes it) otherwise turns a future out-of-range ID into an out-of-bounds read on the mix path where the old code degraded to silence.
| void CChannel::GetGainsAndPannings ( CVector<float>& vecGains, CVector<float>& vecPannings ) | |
| { | |
| QMutexLocker locker ( &Mutex ); | |
| // copy the gain and pan values under a single lock instead of acquiring the | |
| // mutex once per value | |
| vecGains = vecfGains; | |
| vecPannings = vecfPannings; | |
| } | |
| void CChannel::GetGainsAndPannings ( const CVector<int>& vecChanIDs, const int iNumClients, CVector<float>& vecGains, CVector<float>& vecPannings ) | |
| { | |
| QMutexLocker locker ( &Mutex ); | |
| // copy the gain and pan values of the connected channels under a | |
| // single lock, compacted to the caller's channel order; out-of-range | |
| // channel IDs get zero gain and pan | |
| for ( int j = 0; j < iNumClients; j++ ) | |
| { | |
| const int iChanID = vecChanIDs[j]; | |
| if ( ( iChanID >= 0 ) && ( iChanID < MAX_NUM_CHANNELS ) ) | |
| { | |
| vecGains[j] = vecfGains[iChanID]; | |
| vecPannings[j] = vecfPannings[iChanID]; | |
| } | |
| else | |
| { | |
| vecGains[j] = 0; | |
| vecPannings[j] = 0; | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
The comment would probably need to be changed anyway as we don't care about what happened before the change (it's all visible in git). From performance standpoint, it would probably be better to first set vecGains/vecPannings[0..MAX_NUM_CHANNELS] = 0 with some memset and then only fill the ones where the if would fire - but the access pattern is potentially non contiguous anyway.
| CVector<CVector<float>> vecvecfGainsSnapshot; | ||
| CVector<CVector<float>> vecvecfPanningsSnapshot; |
There was a problem hiding this comment.
🤖 AI: [3/7] With the compacted fill the getter writes vecvecfGains[iChanCnt]/vecvecfPannings[iChanCnt] directly, so these two members (2 × 150 × 150 floats ≈ 180 KB of permanent state for a value that lives inside one DecodeReceiveData call) are no longer needed.
| CVector<CVector<float>> vecvecfGainsSnapshot; | |
| CVector<CVector<float>> vecvecfPanningsSnapshot; |
There was a problem hiding this comment.
Probably true.. Will need to go through the code again.
| vecvecfGainsSnapshot.Init ( iMaxNumChannels ); | ||
| vecvecfPanningsSnapshot.Init ( iMaxNumChannels ); |
There was a problem hiding this comment.
🤖 AI: [4/7] Snapshot members deleted (3/7).
| vecvecfGainsSnapshot.Init ( iMaxNumChannels ); | |
| vecvecfPanningsSnapshot.Init ( iMaxNumChannels ); |
We can probably remove the dead code. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Seems like I can't fully apply the suggestions. Anyway. Maybe open a. Ew PR to my repo such that I can see the full changes? |
Please open an upstream issue. |
|
Further todo: check the memory layout in how far we may want to interlieve the layout for pan and gain to avoid cache misses. CC: @coderabbitai find out if the layout of vecsPan/gain should be interlieved as struct GainPan { float gain, pan; }; |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline src/channel.h --items all --match 'GetGainsAndPannings|GetGain|GetPan'
ast-grep outline src/channel.cpp --items all --match 'GetGainsAndPannings|GetGain|GetPan|SetGain|SetPan'
ast-grep outline src/server.h --items all --match 'vecvecfGainsSnapshot|vecvecfPanningsSnapshot|vecvecfGains|vecvecfPannings'
ast-grep outline src/server.cpp --items all --match 'DecodeReceiveData'
printf '\n--- relevant declarations and implementations ---\n'
rg -n -C 8 'GetGainsAndPannings|GetGain\s*\(|GetPan\s*\(|SetGain\s*\(|SetPan\s*\(|vecvecfGainsSnapshot|vecvecfPanningsSnapshot|vecvecfGains|vecvecfPannings' \
src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '\n--- CVector declaration ---\n'
fd -t f -a '.*\.(h|hpp)$' src | xargs -r rg -n -C 4 'class CVector|template\s*<.*>\s*class CVector'Length of output: 18877 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- CChannel snapshot implementation ---'
sed -n '355,385p' src/channel.cpp
printf '%s\n' '--- CServer initialization ---'
sed -n '155,205p' src/server.cpp
printf '%s\n' '--- CServer snapshot and compaction path ---'
sed -n '870,930p' src/server.cpp
printf '%s\n' '--- CServer mono and stereo reads ---'
sed -n '1028,1122p' src/server.cpp
printf '%s\n' '--- CVector methods ---'
sed -n '145,205p' src/util.h
printf '%s\n' '--- PR diff ---'
git diff --find-renames -- src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '%s\n' '--- Snapshot identifiers and declarations ---'
rg -n -C 5 'vecvecfGainsSnapshot|vecvecfPanningsSnapshot' srcLength of output: 14421
The mixer loads gain and pan once per source channel before it enters the per-sample loops. Therefore, this access is not a cache-sensitive inner-sample operation. A The current structure-of-arrays layout is therefore appropriate: CVector<float> vecfGains;
CVector<float> vecfPannings;Use One separate issue needs correction before build verification. The inspected checkout declares the four-argument compacting You are interacting with an AI system. |
880823f to
e9d13dd
Compare
| { | ||
| QMutexLocker locker ( &Mutex ); | ||
|
|
||
| // copy the gain and pan values under a single lock instead of acquiring the |
|
@coderabbitai re-review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed files ---'
git diff --name-status HEAD^ HEAD
printf '%s\n' '--- pull request diff ---'
git diff --find-renames HEAD^ HEAD -- src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '%s\n' '--- relevant symbol outlines ---'
ast-grep outline src/channel.h --items all --match 'GetGainsAndPannings|GetGain|GetPan'
ast-grep outline src/channel.cpp --items all --match 'GetGainsAndPannings|GetGain|GetPan|SetGain|SetPan'
ast-grep outline src/server.h --items all --match 'vecvecfGains|vecvecfPannings|Snapshot'
ast-grep outline src/server.cpp --items all --match 'DecodeReceiveData'
printf '%s\n' '--- declarations, implementations, and call sites ---'
rg -n -C 6 '\b(GetGainsAndPannings|GetGain|GetPan|SetGain|SetPan)\s*\(|vecvecf(Gains|Pannings)(Snapshot)?\b' \
src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '%s\n' '--- direct calls across source tree ---'
rg -n -C 3 '\.(GetGainsAndPannings|GetGain|GetPan)\s*\(' src
printf '%s\n' '--- relevant server paths ---'
sed -n '150,215p' src/server.cpp
sed -n '850,950p' src/server.cpp
sed -n '1000,1140p' src/server.cppLength of output: 34221
The prior mismatch is fixed. The snapshot size is valid. Channel gain and pan vectors use The locking change is safe. The indexed reads are valid for channel IDs. Each snapshot row is private to
I did not execute a build or runtime test in this review environment. You are interacting with an AI system. |
|
Todo: check call sites manually |
1e29f99 to
af33621
Compare
ann0see
left a comment
ann0see
left a comment
There was a problem hiding this comment.
I believe that applying the comments somehow went wrong?
Acquire the channel mutex once per channel and copy the gain/pan values of all connected channels (compacted into the caller's channel order, with bounds guard) instead of acquiring it twice per channel pair (O(N^2) lock/unlock operations per server frame). The values are written directly into vecvecfGains/vecvecfPannings, no intermediate snapshot buffers are needed. Drop the now unused CChannel::GetGain()/GetPan().
af33621 to
68e86fb
Compare
ann0see
left a comment
ann0see
left a comment
There was a problem hiding this comment.
Seems more in line with what I expected now. @coderabbitai review
|
🤖 Completed: Generate docstrings for PR #293 — View PR #321 |
|
@mcfnord I think it's worth benchmarking this rigorously. On my machine I couldn't reproduce the gains from the start - however I believe that my PC is too noisy anyway. So follow the same methodology as the initial one - then refine it with fixed cpu frequency, warm up phase etc. I believe that we should definitely see improvements |
|
@coderabbitai it's more important that you do a thorough code review! |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR commit and changed files ---'
git status --short
git diff --name-status HEAD^ HEAD
git diff --check HEAD^ HEAD
git diff --find-renames --unified=80 HEAD^ HEAD -- \
src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '%s\n' '--- symbol outlines ---'
ast-grep outline src/channel.h --items all --match 'CChannel|GetGainsAndPannings|SetGain|SetPan|GetFadeInGain'
ast-grep outline src/channel.cpp --items all --match 'CChannel|GetGainsAndPannings|SetGain|SetPan'
ast-grep outline src/server.h --items all --match 'CServer|DecodeReceiveData|vecvecfGains|vecvecfPannings'
ast-grep outline src/server.cpp --items all --match 'CServer|DecodeReceiveData'
printf '%s\n' '--- exact API declarations, definitions, calls, and member use ---'
rg -n -C 8 -P '\b(GetGainsAndPannings|GetGain|GetPan|SetGain|SetPan|GetFadeInGain)\s*\(|\bvecvecf(Gains|Pannings)(Snapshot)?\b' src
printf '%s\n' '--- channel constants, vectors, mutex, and relevant implementations ---'
rg -n -C 10 -P '\b(MAX_NUM_CHANNELS|vecfGains|vecfPannings|Mutex)\b' src/channel.h src/channel.cpp
printf '%s\n' '--- DecodeReceiveData and adjacent mixing implementation ---'
rg -n -A 330 -B 30 -P '^\s*void\s+CServer::DecodeReceiveData\s*\(' src/server.cpp
printf '%s\n' '--- CVector declaration and resize/copy/index semantics ---'
fd -t f -a 'util.h' . -x sh -c '
for f do
printf "\n--- %s ---\n" "$f"
rg -n -C 12 -P "class\s+CVector\b|struct\s+CVector\b|CVector\s*<|operator=|Init\s*\(|Resize\s*\(|SetSize\s*\(|operator\[\]" "$f"
done
' sh {} +
printf '%s\n' '--- related server synchronization and parallel-dispatch context ---'
rg -n -C 10 -P '\b(DecodeReceiveData|QMutex|Mutex|Thread|Process|Parallel|vecChanIDsCurConChan|iNumClients)\b' src/server.h src/server.cppLength of output: 94711 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository state and change separation ---'
git status --short
printf '%s\n' 'Committed diff (HEAD^..HEAD):'
git diff --check HEAD^ HEAD
git diff --stat HEAD^ HEAD
printf '%s\n' 'Working-tree diff:'
git diff --check
git diff --stat
git diff -- src/channel.h src/channel.cpp src/server.h src/server.cpp
printf '%s\n' '--- all repository references to old and new accessors ---'
rg -n -C 5 -P '\b(GetGainsAndPannings|GetGain|GetPan)\s*\(' . \
-g '!**/.git/**' -g '!**/build/**' -g '!**/dist/**'
printf '%s\n' '--- constructor, timer, and processing-phase dispatch ---'
sed -n '45,225p' src/server.cpp
sed -n '620,815p' src/server.cpp
printf '%s\n' '--- server state and processing declarations ---'
sed -n '100,325p' src/server.h
printf '%s\n' '--- connected-channel list construction and lifecycle ---'
sed -n '1400,1545p' src/server.cpp
rg -n -C 12 -P '\b(vecChanIDsCurConChan|iNumClients|iMaxNumChannels)\b' src/server.cpp src/server.h
printf '%s\n' '--- thread-pool and block-processing definitions and use ---'
rg -n -C 16 -P '\b(DecodeReceiveDataBlocks|MixEncodeTransmitDataBlocks|ProcessData|ThreadPool|QtConcurrent|run\s*\(|waitForDone|iStartChanCnt|iStopChanCnt)\b' src
printf '%s\n' '--- fade-in field declaration and every read/write site ---'
rg -n -C 8 -P '\b(iFadeInCnt|iFadeInCntMax|GetFadeInGain)\b' src/channel.h src/channel.cpp src/server.cpp
printf '%s\n' '--- channel count constraints and MAX_NUM_CHANNELS definition ---'
rg -n -C 6 -P '^\s*#\s*define\s+MAX_NUM_CHANNELS\b|\bMAX_NUM_CHANNELS\b|\biMaxNumChannels\b' \
src/global.h src/server.h src/server.cpp src/channel.h src/channel.cppLength of output: 134038
I verified these items:
I did not run a build or runtime test in this environment. You are interacting with an AI system. |
|
🤖 AI: Rebenchmarked on three machines, and the flat saving is the thing to chase: this term is quadratic in N, so −4.44 pp at 50 clients and −3.94 pp at 100 cannot both be it. A quadratic term puts the 50-client row at a quarter of the 100-client one; his sits 4.51× above that. Isolated first. A harness replaying the locked getter against one lock per channel, same matrices byte-for-byte, gives a constant 41.06 instructions per removed call on a Xeon 8280 (68.5 on a Pi 4), unchanged from N=50 to N=150. Then end-to-end, one core, no multithreading, 20 s warm-up discarded, 60 s measured, runs interleaved, every point verified to be delivering ≥99% of expected frames to every client:
×2.39 from 50 to 75 against ×2.25 predicted, no overlap between groups. At N=25 the term is 0.69 pp and sits under this rig's noise floor, so that row is unresolvable rather than small. Harnesses and raw output: supporting gist. The capacity question answers itself on the way: ramping until the server stops delivering one mixed packet per client per frame, baseline holds 90 clients and misses at 95; patched holds 95 and misses at 100 — both knees landing identically on a repeat run an hour later. So a single core of this Xeon carries 100 clients on neither build, and any single-core figure at that count comes from a server already behind its frame deadline. Meanwhile CodeRabbit's re-review certified that |
|
After I've manually added the dockstrings I assume it's ready for an upstream PR |
2 participants
Short description of changes
Replace the per-pair
GetGain()/GetPan()calls in the server audio mixing loop with a single mutex-guarded snapshot per channel per frame (CChannel::GetGainsAndPannings()). The old code acquired the channel mutex 2N times per frame for every connected channel (O(N²) lock/unlock operations per server tick); the new code acquires it once per channel and copies both arrays.CHANGELOG: SKIP
Context: Fixes an issue?
No issue; performance optimization. The mutex cost is quadratic in the number of connected channels, so it is only measurable on large servers (measured with 50 and 100 clients).
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Working implementation; benchmarked with 50 and 100 real client processes.
What is missing until this pull request can be merged?
Review. Two open points for reviewers:
CChannel::GetGain()/GetPan()now have no remaining callers (dead code) — should they be removed or kept as public API?MAX_NUM_CHANNELSwhile the siblingvecvecfGainsusesiMaxNumChannels; functionally harmless (the copy assignment resizes) but asymmetric.Benchmark
Method: headless server pinned to a single core,
jackd -d dummy(48 kHz / 128 samples), real client processes streaming silence over UDP, server CPU read from/proc/<pid>/stat(utime+stime) over 40 s, 3 interleaved runs per build, zero overlap between the two groups. The absolute saving is ~4 pp of one core at both 50 and 100 clients; the server baseline itself grows with N because the audio mix is also O(N²).Checklist
Summary by CodeRabbit
Performance
Reliability