Replace per-pair gain/pan mutex locking with snapshot by ann0see · Pull Request #3948 · jamulussoftware/jamulus

ann0see · 2026-09-13T09:50:06Z

Currently, we aquire a lock in GetPan, GetGain overly often in channel.cpp. We can use a one time snapshot and lock/unlock only once. Snapshotting removes constantly aquiring and releasing the Mutex which should give performance gains. Measurements suggest that it indeed gives a minor performance gain. I believe that this is safe (maybe even safer than the current code) from a concurrency viewpoint.

See AI findings/PR and more details here: ann0see#293

Short description of changes

CHANGELOG: Channel: Improve performance of getting gain and pan by using one time snapshot

Context: Fixes an issue?

Related to: #3916

Does this change need documentation? What needs to be documented and how?

No

Status of this Pull Request

Tested and measured by me and @mcfnord Needs review.

What is missing until this pull request can be merged?

Review

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

AUTOBUILD: Please build all targets

coderabbitai · 2026-09-13T09:50:27Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: QUIET

Plan: Advanced

Run ID: 479a544b-66ce-4d58-aab4-cb9063633185

📥 Commits

Reviewing files that changed from the base of the PR and between 57bb17c and 4616983.

📒 Files selected for processing (2)

src/channel.cpp
src/channel.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

CChannel replaces its single-channel gain and pan accessors with a bulk method. CServer::DecodeReceiveData uses the method to fill gain and panning arrays for connected channels. The existing fade-in gain processing remains.

Changes

Channel gain and panning retrieval

Layer / File(s)	Summary
Bulk retrieval API `src/channel.h`, `src/channel.cpp`	`CChannel` replaces `GetGain` and `GetPan` with `GetGainsAndPannings`. The method fills output vectors under `Mutex`. For out-of-range channel IDs, it writes gain `0.0` and panning `0.5`.
Server integration `src/server.cpp`	`CServer::DecodeReceiveData` uses the bulk method to fill gain and panning arrays. The existing fade-in gain multiplication remains unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: pljones, dingodoppelt

Merge Risk: ⚪ Minimal · up to 46169

The bulk snapshot preserves gain, panning, and fade-in behavior while reducing mutex acquisitions. No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 46169

The reviewed server path preserves existing network controls, channel ownership, and audio routing. No introduced security issue was identified. Compatibility with callers outside this repository and broader security coverage remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The changed operation affects gain/pan retrieval for connected-client audio mixes within one server process. The inspected path introduces no new network entrypoint or expansion of gain/pan control authority.

Security Findings and Attack Paths

inferred — Tracing network gain/pan messages through existing setters, the bulk snapshot, and audio mixing did not identify a PR-introduced control bypass, buffer-size violation, or cross-client routing error in the reviewed caller. This conclusion does not establish complete security coverage.

Trust Boundaries and Controls

observed — The server, not a newly exposed network argument, supplies the connected-ID list and count. Existing protocol-to-setter controls remain in place. The public declaration is a native method boundary, not a new remotely callable endpoint.

Resilience and Maintainability Implications

inferred — The inspected synchronization prevents a freed channel ID from being reassigned during the timer's snapshot-and-consumption cycle. Disconnect detection returns from the affected decode path, while subsequent list refresh and direct fade-in access retain baseline behavior. No new lifecycle or recovery transition was identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: replacing repeated gain and pan mutex locking with a snapshot.
Description check	✅ Passed	The description includes all required sections, explains the performance change, provides context, states documentation needs, identifies remaining review work, and completes the checklist.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

ann0see · 2026-09-13T09:51:05Z

    }
 }

-float CChannel::GetGain ( const int iChanID )


No longer needed

ann0see · 2026-09-13T09:51:45Z

Related: #3945

mcfnord · 2026-09-19T03:44:07Z

🤖 AI: The gain is measurable, and it grows with client count. df07df67 against its own merge-base 292506eb, both rebuilt clean in one session on a 4-core aarch64, headless serveronly, server under -T. Three reps per cell, arms interleaved inside each cell so drift cannot land on one of them. Per-client server cost, mean ±1 sample sd:

N	cycles base	cycles #3948	Δ cycles	Δ instructions
12	1.652 bn ±5.4 M	1.646 bn ±17.7 M	−0.40%	−1.71%
24	1.785 bn ±14.6 M	1.711 bn ±25.6 M	−4.12%	−3.33%
36	1.874 bn ±7.3 M	1.816 bn ±22.0 M	−3.06%	−4.88%
48	1.968 bn ±7.8 M	1.894 bn ±14.4 M	−3.77%	−6.68%

At N=12 the cycle difference sits inside ±1 sd and is not resolvable; from N=24 up it separates at every size, and GetGainsAndPannings is never the slower arm. Instructions separate at all four sizes, so the work saved shows everywhere while the time saved needs clients to accumulate — the shape of a per-client locking win, given the single call site now runs once per channel instead of once per pair.

The diff adds no buffer. vecvecfGains[iChanCnt] and vecvecfPannings[iChanCnt] are the rows the old loop already filled, and server.h is not among the three changed files. What goes away is lock traffic: GetGain and GetPan each took the mutex once per call from inside the per-client loop, so two acquisitions per client per receiving channel per frame become one for the whole channel, and a channel's gain and panning now leave the same critical section together.

Run-to-run spread at N=48 is six times tighter on this branch than on base, 2.9 M against 18.5 M instructions per client. No mechanism claimed for that; three reps is not an investigation.

All 24 cells cleared the saturation gate — delivered packet ratio 1.010–1.011 against a 0.99 floor, none discarded, no channel ever unmixed.

ann0see · 2026-09-20T21:11:40Z

@mcfnord I think you can open a follow up with the other change proposed by you.

Also we're missing a 2nd review here.

ann0see · 2026-09-26T13:23:03Z

@softins @dingodoppelt could someone of you please have a look at this?

dingodoppelt · 2026-09-26T19:30:06Z

-        return 0;
+        const int iChanID = vecChanIDs[j];
+
+        if ( ( iChanID >= 0 ) && ( iChanID < MAX_NUM_CHANNELS ) )


MathUtils::InRange could be used here.

Yes. Agree. Will apply this in the next batch.

dingodoppelt · 2026-09-26T19:31:47Z

+        {
+            // should not happen
+            vecGains[j]    = 0;
+            vecPannings[j] = 0;


I'd reset the panning to 0.5 because 0 is hard left.

softins · 2026-09-29T15:09:41Z

@softins @dingodoppelt could someone of you please have a look at this?

Looks ok to me. Agree with @dingodoppelt's couple of comments, but otherwise fine.

ann0see · 2026-09-29T18:47:23Z

Hmm. Pushed the changes but it doesn't show up here...

ann0see · 2026-09-29T18:52:35Z

Anyway. Seems to be done now.

softins · 2026-10-01T12:11:47Z

+            vecGains[j]    = 0.5;
+            vecPannings[j] = 0.5;


Suggested change

vecGains[j] = 0.5;

vecPannings[j] = 0.5;

vecGains[j] = 0.0;

vecPannings[j] = 0.5;

Even though this situation should not occur, I think gain should still default to 0, with only pan defaulting to 0.5

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(). Co-authored-by: mcfnord <mcfnord@users.noreply.github.com>

ann0see · 2026-10-01T13:27:25Z

Done.

softins

Seems good to me now. Compiled and tested with gain and pan changes on a couple of channels.

ann0see added refactoring Non-behavioural changes, Code cleanup AI AI generated or potentially AI generated labels Sep 13, 2026

ann0see added this to Tracking Sep 13, 2026

github-project-automation Bot moved this to Triage in Tracking Sep 13, 2026

ann0see requested a review from softins September 13, 2026 09:50

ann0see commented Sep 13, 2026

View reviewed changes

Comment thread src/channel.cpp

}

}

float CChannel::GetGain ( const int iChanID )

ann0see Sep 13, 2026

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

No longer needed

ann0see moved this from Triage to Waiting on Team in Tracking Sep 13, 2026

pljones reviewed Sep 13, 2026

View reviewed changes

Comment thread src/server.cpp

pljones reviewed Sep 13, 2026

View reviewed changes

Comment thread src/server.cpp Outdated

pljones approved these changes Sep 14, 2026

View reviewed changes

ann0see added this to the Release 4.0.0 milestone Sep 14, 2026

pljones approved these changes Sep 15, 2026

View reviewed changes

mcfnord mentioned this pull request Sep 21, 2026

Server: read the fade-in gain once per channel per frame #3959

Open

5 tasks done

dingodoppelt reviewed Sep 26, 2026

View reviewed changes

ann0see force-pushed the perf/server-gain-pan-snapshot branch from df07df6 to 57bb17c Compare September 29, 2026 18:49

softins reviewed Oct 1, 2026

View reviewed changes

ann0see and others added 2 commits October 1, 2026 15:26

Fix typo

4616983

ann0see force-pushed the perf/server-gain-pan-snapshot branch from 57bb17c to 4616983 Compare October 1, 2026 13:27

softins approved these changes Oct 1, 2026

View reviewed changes

softins merged commit 7ebf8f8 into jamulussoftware:main Oct 1, 2026
16 checks passed

ann0see deleted the perf/server-gain-pan-snapshot branch October 1, 2026 15:26

Conversation

ann0see commented Sep 13, 2026 • edited by pljones Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

coderabbitai Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

ann0see Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

ann0see commented Sep 13, 2026

Uh oh!

Uh oh!

Uh oh!

mcfnord commented Sep 19, 2026

Uh oh!

ann0see commented Sep 20, 2026

Uh oh!

ann0see commented Sep 26, 2026

Uh oh!

dingodoppelt Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

ann0see Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingodoppelt Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

softins commented Sep 29, 2026

Uh oh!

ann0see commented Sep 29, 2026

Uh oh!

ann0see commented Sep 29, 2026

Uh oh!

softins Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

ann0see commented Oct 1, 2026

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ann0see commented Sep 13, 2026 •

edited by pljones

Loading

coderabbitai Bot commented Sep 13, 2026 •

edited

Loading

ann0see Sep 28, 2026 •

edited

Loading