perf(acars): shared multi-channel downconverter front end (per-channel DDC → channelizer)#178
Open
fredclausen wants to merge 7 commits into
Open
perf(acars): shared multi-channel downconverter front end (per-channel DDC → channelizer)#178fredclausen wants to merge 7 commits into
fredclausen wants to merge 7 commits into
Conversation
cpu.sh had no ACARS row. ACARS runs many narrowband channels from one wideband capture, so the per-channel downconverter front end dominates CPU and scales with channel count -- exactly the cost worth gating. Synthesize a multi-channel 2.4 MS/s capture from the crate's modulator (no vendored off-air fixture exists) and decode it across 8 VHF ACARS channels, the realistic acarsdec-replacement load.
A per-channel Ddc runs a full-rate anti-alias decimation for every channel, so N channels do N full-rate convolutions over the same wideband stream -- the dominant decode cost, linear in channel count. SharedDdc downconverts several channels that share one capture/output rate/passband with the expensive full-rate decimation run ONCE, then a cheap per-channel NCO mix + sharp finish at the low intermediate rate. Equivalent per-channel output to N independent Ddcs. General-purpose (xng-dsp): intended for ACARS first, then the other narrowband multi-channel modes (VDL2/AIS/Aero/STD-C) that use the same per-channel offset DDC today.
When a session has >=2 ACARS channels, collapse_shared_acars replaces the N independent per-channel AcarsChannelDecoders with one AcarsMultiChannelDecoder that shares a single downconverter front end (SharedDdc), then runs the usual per-channel MskDemod/Deframer. A single channel or any other mode is unchanged; a build failure of the shared front end falls back to per-channel DDCs (optimization, never required). Runtime: a ModeChannel::AcarsShared variant (spans many channels) is special-cased in decode_loop via process_shared, which emits per-channel freq/level/provenance-tagged messages. The publish pipeline is factored into publish_msg, shared by the single-channel and shared paths; shared stats are tracked per-frequency. A TODO marks generalizing the loop for the VDL2/AIS/Aero/STD-C rollout. A loopback test asserts the shared decoder decodes the same frames as the per-channel path.
SharedDdc's win shrinks as channels spread across the band: its coarse stage can only decimate as far as the widest channel allows, so a wide span keeps the per-channel finish expensive. ChannelizedDdc removes that dependence with a polyphase channelizer -- one shared FFT pass produces every channel, cost independent of channel count AND span. VHF airband channels are all on a 25 kHz raster, so the bin grid fs/M is chosen to land every requested channel on a bin center (no scalloping); a small residual NCO + a gentle resampler hit the exact 24 kHz channel rate. AcarsMultiChannelDecoder now prefers ChannelizedDdc via a switchable Front enum, falling back to SharedDdc (new_shared) when the channelizer cannot be built. A reverting commit drops the Channelized arm and leaves the decoder on SharedDdc -- the proven fallback, not dead code. A test asserts both front ends decode identically. Measured (bench/cpu.sh, 32-core x86): 16 wide channels 1.9x -> 18x, 8 tight channels 3.7x -> 27x vs the old per-channel DDC. Live RF on a 16-channel underpowered box: ~25% -> ~8% CPU, and SDR overrun errors gone (the front end now keeps up with the ring buffer). Both front ends are general-purpose xng-dsp components, intended next for VDL2/AIS/Aero/STD-C, which use the same per-channel offset DDC today.
|
Warning Review limit reached
More reviews will be available in 6 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded shared multi-channel ACARS decoding across DSP front ends, ACARS mode decoding, runtime session handling, tests, docs, and CPU benchmarking. The runtime can now collapse multi-channel ACARS sessions into a shared decoder path or keep the per-channel path when needed. ChangesShared multi-channel ACARS decoding
Sequence Diagram(s)sequenceDiagram
participant build_decoders
participant collapse_shared_acars
participant AcarsMultiChannelDecoder
participant decode_loop
participant publish_msg
participant ChannelizedDdc
participant SharedDdc
build_decoders->>collapse_shared_acars: collapse multi-channel ACARS decoders
collapse_shared_acars->>AcarsMultiChannelDecoder: new(input_rate, offsets)
AcarsMultiChannelDecoder->>ChannelizedDdc: try channelizer front end
alt channelizer construction fails
AcarsMultiChannelDecoder->>SharedDdc: new_shared(input_rate, offsets)
end
decode_loop->>AcarsMultiChannelDecoder: process_shared(input)
AcarsMultiChannelDecoder->>publish_msg: publish decoded frames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bench/cpu.sh (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reusing potentially stale
/tmpfixture data across benchmark runs.This guard can silently keep old capture content even after
gen_capturebehavior changes, which can invalidate benchmark comparisons. Prefer regenerating each run (or versioning the cache key by generator settings/commit).Suggested change
-if [ ! -f /tmp/bench_acars_8ch.cf32 ]; then - cargo run --release -q -p xng-mode-acars --example gen_capture -- /tmp/bench_acars_1x.cf32 >/dev/null 2>&1 - for i in $(seq 5); do cat /tmp/bench_acars_1x.cf32; done > /tmp/bench_acars_8ch.cf32 -fi +cargo run --release -q -p xng-mode-acars --example gen_capture -- /tmp/bench_acars_1x.cf32 >/dev/null 2>&1 +for _ in $(seq 5); do cat /tmp/bench_acars_1x.cf32; done > /tmp/bench_acars_8ch.cf32🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/cpu.sh` around lines 30 - 33, The benchmark fixture generation in the shell script reuses a cached /tmp file, which can leave stale capture data in place across runs. Update the logic around the gen_capture and bench_acars_8ch.cf32 setup to regenerate the fixture each run or key the cached file off generator settings/commit so changes to the capture generator are always reflected in benchmarks.crates/xng-mode-acars/src/lib.rs (1)
165-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider surfacing the channelizer build error on double failure.
When
ChannelizedDdc::newfails the error is dropped (Err(_)) and only theSharedDdcerror propagates via?. If both front ends reject the rate/offset set, the caller never sees why the (default) channelizer was rejected, which complicates debugging unusual capture configurations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/xng-mode-acars/src/lib.rs` around lines 165 - 172, The fallback logic in `Front::new` is dropping the `ChannelizedDdc::new` failure by matching `Err(_)`, so the caller only sees the `SharedDdc::new` error on double failure. Update the `Front::new` construction path to capture and retain the channelizer error and combine it with the shared DDC failure if both constructors reject the input, so the final `Result<Self, String>` reports why the default channelizer was rejected as well as why the fallback failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/xng-dsp/src/channelized_ddc.rs`:
- Around line 79-83: The validation in channelized_ddc::channelized path only
checks the center residual against the bin midpoint, so channels whose passband
extends past the PFB bin edge can still be accepted. Update the offset check in
the channel selection logic to also consider passband_hz, rejecting cases where
residual.abs() + passband_hz exceeds bin_rate / 2 so the code falls back to
SharedDdc instead of silently using an attenuated channelized path.
In `@src/runtime.rs`:
- Around line 781-809: The shared-channel path in process_shared only emits
active channels, so quiet channels never get their live level/count refreshed
and can stay stale compared with the single-channel path. Update the shared
decode flow so every channel in the shared decoder is recorded each chunk, even
when dec.process(iq) yields no frames for that index, by reusing the same
record_channel/live-metrics update logic used elsewhere in runtime.rs and
ensuring the caller handles all freqs from AcarsShared, not just the ones
returned in the out vector.
---
Nitpick comments:
In `@bench/cpu.sh`:
- Around line 30-33: The benchmark fixture generation in the shell script reuses
a cached /tmp file, which can leave stale capture data in place across runs.
Update the logic around the gen_capture and bench_acars_8ch.cf32 setup to
regenerate the fixture each run or key the cached file off generator
settings/commit so changes to the capture generator are always reflected in
benchmarks.
In `@crates/xng-mode-acars/src/lib.rs`:
- Around line 165-172: The fallback logic in `Front::new` is dropping the
`ChannelizedDdc::new` failure by matching `Err(_)`, so the caller only sees the
`SharedDdc::new` error on double failure. Update the `Front::new` construction
path to capture and retain the channelizer error and combine it with the shared
DDC failure if both constructors reject the input, so the final `Result<Self,
String>` reports why the default channelizer was rejected as well as why the
fallback failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2802f036-6af9-48d0-a615-453803973471
📒 Files selected for processing (9)
bench/cpu.shcrates/xng-dsp/src/channelized_ddc.rscrates/xng-dsp/src/lib.rscrates/xng-dsp/src/shared_ddc.rscrates/xng-mode-acars/src/lib.rscrates/xng-mode-acars/tests/end_to_end.rsdocs/notes/ACARS.mddocs/notes/BENCHMARKS.mdsrc/runtime.rs
The edge guard only checked the center residual against +/-1/2 bin, so a channel whose +/-passband spilled past the selected PFB bin edge was still accepted and silently attenuated instead of falling back to SharedDdc. Tighten the guard to bin_rate/2 - passband. choose_num_bins sizes bins at >= 2.4*passband so the real airband raster passes (verified: the 16-channel US plan's worst residual 12.5 kHz << 20 kHz threshold); this guards pathological rate/offset sets.
process_shared only returned channels that decoded a frame, so record_channel (live dashboard level/count) fired only for active channels -- quiet shared channels kept stale/missing metrics, unlike the single-channel path which records every channel every chunk. Return every channel each chunk (quiet ones with no messages, zero counts) so all are recorded.
- bench/cpu.sh: regenerate the synthetic ACARS capture every run (cheap, ~0.5 s) instead of caching in /tmp, so a gen_capture change can't leave a stale capture skewing the comparison. - AcarsMultiChannelDecoder::new: on double front-end failure, report the channelizer error alongside the shared-decim error, so an unusual capture config shows why the default channelizer was rejected too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
ACARS decodes many narrowband channels from one wideband capture, but each
channel ran its own digital downconverter — N independent full-rate
anti-alias decimations over the same stream. That front end (not the bit
demod, which is ~17× cheaper) dominated CPU and scaled linearly with channel
count. On an underpowered 16-channel box this was ~25% CPU and produced SDR
overrun errors (the decoder couldn't keep up with the ring buffer, so real
samples — and real frames — were being dropped).
This PR collapses the per-channel front ends into one shared downconverter
for all ACARS channels, with two interchangeable implementations behind the
same interface:
xng_dsp::ChannelizedDdc(default) — a polyphase channelizer: one sharedFFT pass produces every channel, so cost is independent of channel count
and of how far apart the channels sit.
xng_dsp::SharedDdc(fallback) — one shared full-rate decimation feedscheap per-channel finishes. Its win shrinks as channels spread across the
band (the coarse stage can only decimate as far as the widest channel
allows), so the channelizer is preferred;
SharedDdcis the fallback whenthe channelizer can't be built for a rate/offset set, and the clean revert
target for the channelizer commit.
Both produce byte-identical decodes to the old per-channel path (a
cargo testasserts it); the choice is purely CPU.
Results
Live RF, 16 channels, underpowered box: ~25% → ~8% CPU, and the SDR
overrun errors are gone (the front end now keeps up).
Synthetic bench (
bench/cpu.sh, 32-core x86 dev box), front end only:SharedDdc)ChannelizedDdc, default)Commits
bench(acars):add a multi-channel ACARS CPU benchmark row (bench/cpu.sh).perf(dsp):addSharedDdc(the fallback front end).perf(acars):drive multi-channel ACARS through one shared front end(
AcarsMultiChannelDecoder+ runtime wiring; defaults toSharedDdcatthis commit).
perf(dsp):addChannelizedDdcand make it the default; reverting justthis commit falls back to the proven
SharedDdcpath (verified: ACARStests stay green after the revert).
All workspace tests pass (1225, 0 failures); each commit builds + tests green
on its own.
Where this PR falls short
The channelizer FFT is now the dominant cost, and is the gap to the reference
decoder. On the same hardware, acarsdec sits at ~0.6% where this lands ~8%.
The live per-channel-count breakdown shows why:
There is a large fixed floor (~3.1%) that is the channelizer itself, and the
marginal per-channel cost is small (~0.2%/ch). The channelizer computes all M
bins (e.g. M=48 for the US ACARS plan) but only uses ~16 — that wasted-bin
FFT is the bulk of the floor. acarsdec wins by doing cheap short per-channel
mix+decimate on only the requested channels, with no wasted-bin FFT. Closing
this is out of scope here (see follow-up B) — this PR is the structural win
(per-channel front end → shared front end) that unblocks it.
Two known limitations carried as TODOs:
src/runtime.rsTODO(perf)(×2): the decode loop assumes one(freq, ModeChannel)per channel and tags every message from aModeChannelwith oneProvenance. The shared ACARS decoder spans manychannels, so it is special-cased in
decode_loopviaprocess_shared.This is deliberately contained; the loop should be generalized to natively
drive multi-output decoders when other modes adopt the shared front end.
is validated live; the channelizer's behavior on weak/edge frames vs the
old per-channel DDC is not. For the exact 16-channel plan tested, M=48 lands
the worst channel ~0.25 bin (12.5 kHz) off its bin center (the channels don't
all sit on the 50 kHz grid relative to the capture center), which is in the
bin's rolloff. A smarter capture center (more channels on bin centers) and/or
per-channel residual handling would help. Tracked in follow-up A.
Follow-up work (after this PR gets a green light)
Once this is approved, two follow-up PRs:
Follow-up A — roll the shared front end out to the other narrowband modes.
SharedDdcandChannelizedDdcare general-purposexng-dspcomponents. Themodes that use the same per-channel offset DDC today and would benefit
(narrowband, many channels clustered in one wideband capture):
This PR also resolves the
TODO(perf)insrc/runtime.rsas part of that work:generalize the decode loop so any mode can opt into a shared multi-output front
end, instead of the ACARS-only special case.
(Wideband / whole-capture modes — ADS-B, UAT, Iridium — and HF modes whose plans
span many MHz — HFDL, DSC, NAVTEX — do not benefit and are out of scope.)
Follow-up B — close the channelizer FFT gap toward acarsdec. Options to
explore: prune the channelizer to only the requested bins (sparse output / a
Goertzel-style or partial-FFT path), pick
Mand the capture center so everyrequested channel lands on a bin center (kills scalloping and shrinks the
grid), or a hybrid that falls back to cheap per-channel mix+decimate when only a
few channels are requested. This is the path from ~8% toward acarsdec's ~0.6%.
A design/reference doc for follow-up agents is in
docs/notes/ACARS-SHARED-FRONTEND.md.Summary by CodeRabbit
New Features
Bug Fixes
Documentation