feat: assert no allocations on the RT audio path#238
Merged
Conversation
Adds the `assert_no_alloc` crate (warn_debug feature) and per-stage, per-IR-cabinet, and full-engine test coverage that runs `Engine::process` inside an `assert_no_alloc` scope and asserts zero violations. Existing standalone integration tests are also wrapped to give end-to-end coverage. Three known offenders (peak_meter, pitch_shifter, recorder) allocate on the RT path; their call sites in `engine.rs` are wrapped with `permit_alloc` and FIXME comments pointing to the underlying modules to fix in a follow-up. Two real RT-thread `Box::new` sites in `Engine::handle_messages` (IR convolver swap and samplers swap) are wrapped with `permit_alloc` because the `RtDropHandle` channel takes `Box<dyn Send>` and there's no way to type-erase without boxing — proper fix is a separate refactor. Closes #237.
17 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds real-time allocation auditing around the audio engine by introducing assert_no_alloc-based tests in rustortion-core and rustortion-standalone, while annotating a few currently known RT-path allocators inside Engine::process() with permit_alloc. In the wider codebase, this is intended to codify the “no heap traffic on the audio thread” contract and keep future engine/stage changes from regressing it.
Changes:
- Added a new
rustortion-core/tests/no_alloc.rstest suite that exercises the engine hot path, per-stage processing, IR convolution, and standalone-style engine extras underassert_no_alloc. - Extended standalone engine integration tests to assert that repeated
Engine::process()calls stay allocation-free after setup. - Wrapped known RT-path allocation sites in
rustortion-core/src/audio/engine.rswithpermit_alloc, and added the requiredassert_no_allocdependency entries.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
rustortion-standalone/tests/engine.rs |
Adds allocation assertions to existing standalone engine integration tests. |
rustortion-standalone/Cargo.toml |
Adds assert_no_alloc as a standalone test dependency. |
rustortion-core/tests/no_alloc.rs |
New comprehensive RT no-allocation audit test suite for core engine/stage paths. |
rustortion-core/src/audio/engine.rs |
Marks known allocator call sites with permit_alloc inside the RT processing path. |
rustortion-core/Cargo.toml |
Adds assert_no_alloc to core crate dependencies. |
Cargo.lock |
Locks the new assert_no_alloc dependency and workspace dependency graph updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+394
to
+399
| fn full_engine_does_not_allocate() { | ||
| // Covers: Engine::new path with tuner (disabled), metronome | ||
| // (disabled), and peak meter (always-on once present). The peak meter | ||
| // allocates internally; engine.rs wraps that call in permit_alloc with | ||
| // a FIXME pointing to peak_meter.rs:62. | ||
| let (mut engine, _handle) = full_engine(1.0, None); |
| } | ||
|
|
||
| #[test] | ||
| fn pitch_shifter_does_not_allocate() { |
Comment on lines
+165
to
+166
| // FIXME(no_alloc): PitchShifter::process_block uses Vec scratch | ||
| // buffers internally — see audio/pitch_shifter.rs. |
Comment on lines
+65
to
+70
| /// Run `engine.process()` `iters` times inside an `assert_no_alloc` scope | ||
| /// after one warm-up call. Panics if any allocation was observed. | ||
| fn assert_engine_alloc_free(engine: &mut Engine, input: &[f32], output: &mut [f32], iters: usize) { | ||
| // Warm up once outside the assertion to amortise any first-call setup. | ||
| engine.process(input, output).unwrap(); | ||
|
|
Comment on lines
+444
to
+455
| // recorder.rs:47. Recorder::new opens a WAV file + spawns a thread, | ||
| // which is one-shot setup wrapped in permit_alloc here. | ||
| let (mut engine, handle) = full_engine(1.0, None); | ||
| let tmp = std::env::temp_dir().join("rustortion_no_alloc_rec"); | ||
| let recorder = | ||
| permit_alloc(|| Recorder::new(SAMPLE_RATE as u32, tmp.to_str().unwrap()).unwrap()); | ||
| handle | ||
| .start_recording(SAMPLE_RATE, tmp.to_str().unwrap()) | ||
| .unwrap(); | ||
| // Drop the locally-built recorder; the engine will use the one it | ||
| // created from the StartRecording message. | ||
| drop(recorder); |
Comment on lines
+365
to
+367
| let dir = std::env::temp_dir().join("rustortion_no_alloc_ir"); | ||
| let _ = write_test_ir(&dir, "tiny.wav", 1024); | ||
| let loader = IrLoader::new(&dir, SAMPLE_RATE).unwrap(); |
- Wrap PitchShifter::new in handle_pitch_shift with permit_alloc + FIXME. Caught by Copilot review: SetPitchShift drained inside Engine::process() constructs the FFT scratch buffers on the RT thread. - Restructure pitch_shifter_does_not_allocate to send SetPitchShift inside the assert scope so the construction path is actually audited (the warm-up call would otherwise hide it). - Drop the dead local Recorder + manual drop in the recorder test that also risked a filename-timestamp collision with the engine's recorder. - Use tempfile::tempdir() for the IR loader and recorder tests so parallel runs and stale files can't pollute each other. - Doc-clarify check_no_alloc and assert_engine_alloc_free contracts. - Make full_engine_does_not_allocate doc honest about the peak_meter gate.
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
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.
Closes #237.
Summary
assert_no_alloc(warn_debugfeature) and a newrustortion-core/tests/no_alloc.rs(20 tests) that runsEngine::processinside anassert_no_allocscope across all 10 stage types, both IR convolver implementations, and the full standalone-style engine.rustortion-standalone/tests/engine.rs) withassert_no_allocafter their original assertions for end-to-end coverage.peak_meter,pitch_shifter,recorder) withpermit_alloc+ FIXME comments at their call sites inengine.rs. The underlying modules are the proper fix targets.Box::new(...)sites inEngine::handle_messages(IR convolver retire, samplers retire) withpermit_alloc—RtDropHandletakesBox<dyn Send>so type erasure forces an allocation. Follow-up: hold the values asBox<T>on the engine side to remove the allocation entirely.Known offenders (permit_alloc'd, FIXME)
audio/peak_meter.rs:62—Arc::new(PeakMeterInfo)per block. Trivial fix: replaceArc<ArcSwap<PeakMeterInfo>>with three atomics.audio/pitch_shifter.rs— Vec scratch buffers inprocess_block.audio/recorder.rs:47—Vec::with_capacityper block.audio/engine.rs:296,333— boxing forRtDropHandle::retire(channel type erasure).Test plan
cargo test -p rustortion-core --test no_alloc— 20 passedcargo test -p rustortion-standalone --test engine— 7 passed (4 new alloc assertions added)make test— full suite greenmake lint— clean (clippy with-D warnings -D pedantic -D nursery)Out of scope (follow-up work)
rustortion-plugin) feature flag fornih_plug/assert_process_allocs.