Conversation
|
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 configurationConfiguration used: Repository UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe server now reads each connected channel’s fade-in gain once per frame in ChangesChannel gain processing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to The supplied evidence indicates cached gains are populated and applied consistently. No actionable merge-blocking risk is identified. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
@mcfnord please rebase this and resolve conflicts. |
DecodeReceiveData() called CChannel::GetFadeInGain() twice per channel pair, so every frame performed 2N^2-N int-to-float conversions and divisions on N values that do not change within the frame: the fade-in counters are only written under CServer::Mutex, which OnTimer() holds for the whole decode phase. Read each connected channel's fade-in gain into vecfFadeInGains in the loop that builds vecChanIDsCurConChan, in the same order and under the same lock, and multiply from that vector instead. The two factors are applied in the same order as before. Closes jamulussoftware#3945 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
100f2cc to
d01be45
Compare
2 participants
🤖 AI: Follow-up to #3948, opened at ann0see's request for the change described in #3945. Rebased onto
mainafter #3948 merged; the diff is the one commit.Short description of changes
The pair loop in
DecodeReceiveDatacallsGetFadeInGain()twice per channel pair, once for the source channel and once for the current one: 2N²−N int-to-float conversions and divisions per frame, on N values that do not change within the frame. Both counters are written only inPutAudioDataandOnNetTranspPropsReceived, each reached underCServer::Mutex(audio, protocol), andOnTimerholds that mutex from the connected-channel scan through both decode paths, single-threaded and thread pool alike.The commit reads each connected channel's fade-in gain into
vecfFadeInGainsin the loop that buildsvecChanIDsCurConChan, in the same order and inside the same locked block, and the pair loop multiplies from that vector. The same two factors are applied in the same order as before, so the gain row that reaches the mixer is unchanged.vecfFadeInGainsis sized toiMaxNumChannelswith the other per-frame vectors, so the timer path still allocates nothing.GetFadeInGain()keeps one call site.Two review notes from #3948 are folded in as pljones suggested: the getter's comment is reflowed wider on one block, and the two in-loop comments become one above the loop that names both fade-in factors and #628.
Per-client server cost on the rig behind the #3948 numbers: Raspberry Pi 4, headless
serveronly, server under-T, both arms rebuilt clean in one session (the #3948 head and the #3948 head plus this commit), three reps per cell with the arms interleaved inside each cell,perf statover 30 s of steady load:Instructions separate at every N and cycles at none: all four cycle deltas sit inside ±1 sd, so the time saved is below this rig's resolution at N ≤ 48, where the #3945 benchmark puts the removed term at 10.0 µs per frame at N=50. What is measured here is the work, fewer instructions per client at every size and most at N=48; the rig saturates at N=60, so the sizes where the term reaches percent of a frame are out of its reach.
All 24 cells cleared the saturation gate, delivered ratio 1.011 against a 0.99 floor, and every client was mixed in every cell.
Builds: x86-64 (g++ 13.3) and aarch64 (g++ 14.2),
CONFIG+=headless serveronly, zero warnings inserver.cpp/server.h, the server up after 6 s on both;clang-format-14 -style=filebyte-clean on both files.CHANGELOG: Server: Reduced the mixing cost per frame by computing each channel's fade-in gain once per frame instead of once per channel pair
Context: Fixes an issue?
Fixes: #3945, which was itself opened at ann0see's request on the fork PR behind #3948.
Does this change need documentation? What needs to be documented and how?
No.
Status of this Pull Request
Working implementation, rebased onto
mainafter #3948 merged.What is missing until this pull request can be merged?
Review.
Checklist
🤖 This message was written by AI and reviewed by @mcfnord.