Repository navigation
⚡ Hoist 'pow' and array lookups out of inner DSP loops - #71
Conversation
What: The unisonDetunes and stringDetunes iterations generated multiple expensive 'pow' calls per-frame inside a DSP loop. We move the iteration and pow calls outside the audio loop (per-block). Why: pow(x) is an expensive math operation, especially inside an audio rendering loop running at the sample rate (e.g. 44100 times per sec per voice). Measured Improvement: Could not benchmark directly as swift wasn't in PATH. However, skipping 11 pow allocations and variable computations inside a 4096-frame chunk will measurably reduce DSP load significantly for the unison/string oscillators. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe DSP path now calculates and stores unison and string oscillator increments once per audio buffer, eliminating repeated array creation, detune math, and pow calls from the per-frame loop while retaining the existing oscillator behavior. Flow diagram for precomputed oscillator incrementsflowchart TD
A[Audio buffer begins] --> B[Calculate unisonIncrements]
A --> C[Calculate stringIncrements]
B --> D[Per-frame DSP loop]
C --> D
D --> E[Read cached increment]
E --> F[Generate unison or strings oscillator sample]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ebd2131b-cd89-46fd-97a2-a5f7054c92b6) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved: Cursor Bugbot was present but completed as skipped (usage limit), so the required automated-review signal did not succeed. Human review is needed; no reviewers were assigned because the only collaborator is the PR author.
Sent by Cursor Approval Agent: Pull Request Router and Approver
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Sources/XPadAudio/AudioEngine.swift" line_range="1171-1185" />
<code_context>
var reachedReleaseEnd = false
+ // Precalculate unison and string increments outside the frame loop
+ let unisonDetunes: [Double] = [-0.35, -0.22, -0.10, 0.0, 0.10, 0.22, 0.35]
+ var unisonIncrements = [Double](repeating: 0.0, count: 7)
+ for vi in 0..<7 {
+ let df = frequency * (pow(2.0, unisonDetunes[vi] / 12.0) - 1.0)
+ unisonIncrements[vi] = min((frequency + df) / sampleRate, 0.49)
+ }
+
+ let stringDetunes: [Double] = [-0.08, -0.025, 0.025, 0.08]
+ var stringIncrements = [Double](repeating: 0.0, count: 4)
</code_context>
<issue_to_address>
**issue (performance):** The audio callback now constructs both increment buffers and executes all 11 `pow` calls for every active audio block, even when `snap.oscillator1` is a normal oscillator rather than `.unison` or `.strings`. This adds array allocation/deallocation and expensive math to the common non-detuned path, where the previous code performed none of this work, increasing the chance of missing real-time audio deadlines.
**Triggers:** When an active voice uses any oscillator other than `.unison` or `.strings`.
**Suggested fix:** Only calculate these buffers when the selected oscillator requires them, or store reusable buffers/constants outside the audio callback.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: Sources/XPadAudio/AudioEngine.swift:1185
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Precalculate unison and string increments outside the frame loop | ||
| let unisonDetunes: [Double] = [-0.35, -0.22, -0.10, 0.0, 0.10, 0.22, 0.35] | ||
| var unisonIncrements = [Double](repeating: 0.0, count: 7) | ||
| for vi in 0..<7 { | ||
| let df = frequency * (pow(2.0, unisonDetunes[vi] / 12.0) - 1.0) | ||
| unisonIncrements[vi] = min((frequency + df) / sampleRate, 0.49) | ||
| } | ||
|
|
||
| let stringDetunes: [Double] = [-0.08, -0.025, 0.025, 0.08] | ||
| var stringIncrements = [Double](repeating: 0.0, count: 4) | ||
| for vi in 0..<4 { | ||
| let df = frequency * (pow(2.0, stringDetunes[vi] / 12.0) - 1.0) | ||
| stringIncrements[vi] = min((frequency + df) / sampleRate, 0.49) | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (performance): The audio callback now constructs both increment buffers and executes all 11 pow calls for every active audio block, even when snap.oscillator1 is a normal oscillator rather than .unison or .strings. This adds array allocation/deallocation and expensive math to the common non-detuned path, where the previous code performed none of this work, increasing the chance of missing real-time audio deadlines.
Triggers: When an active voice uses any oscillator other than .unison or .strings.
Suggested fix: Only calculate these buffers when the selected oscillator requires them, or store reusable buffers/constants outside the audio callback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d09e978ca5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Precalculate unison and string increments outside the frame loop | ||
| let unisonDetunes: [Double] = [-0.35, -0.22, -0.10, 0.0, 0.10, 0.22, 0.35] | ||
| var unisonIncrements = [Double](repeating: 0.0, count: 7) |
There was a problem hiding this comment.
For every active voice using a normal oscillator such as .sine, .saw, or .triangle—12 of the 14 shipped presets—this unconditionally constructs both detune/increment array pairs and executes all 11 pow calls even though the subsequent switch reads neither bank; .unison and .strings also calculate each other's unused bank. This introduces recurring CPU work and allocation jitter into the real-time AVAudioSourceNode callback for the common path, so select and compute only the bank required by snap.oscillator1 and reuse fixed storage/constants rather than creating arrays per callback.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
What: Moved
powcalculations andunisonDetunes/stringDetunesarrays out of the inner loop (which runs once per audio frame) and precalculated their values in a buffer arraysunisonIncrementsandstringIncrements.Why: The inner DSP loop inside
AudioEngine.swiftis extremely performance-sensitive as it is called many times (e.g., 44100 times per second per voice when active). Performing 11 exponential calculations (pow) inside this inner loop every frame for the.unisonand.stringsmodes introduces unnecessary CPU load that can cause audio dropouts and increased processing time. SincefrequencyandsampleRateare constant for a given AudioBuffer block,powcan be evaluated once before the frame-loop, saving 4096 * 11pow()calls per typical CoreAudio chunk per voice.Measured Improvement: Due to the testing environment lacking
swifttoolchain, direct benchmarking could not be generated and executed. However, theoretically, moving math operations spanning arrays (unisonDetunes,stringDetunes) outside a tight per-framefor frame in 0..<Int(frameCount)loop drops their execution count drastically, thereby directly eliminating overhead. CPU usage for.unisonand.stringspatches should drop measurably.PR created automatically by Jules for task 6839792153999069520 started by @sp80808
Note
Low Risk
Refactor-only DSP math with no API or behavioral intent changes; worst case is equivalent audio with slightly different work when non-unison/string presets run (increments computed unconditionally per buffer).
Overview
SynthVoice now precomputes per-voice phase increments for
.unison(7 detuned saws) and.strings(4 detuned voices) once per audio buffer, immediately before the per-sample frame loop inAudioEngine.swift.Previously, each sample re-created the detune tables and ran
powto derive phase increments inside the oscillator switch. The inner loop now only readsunisonIncrements/stringIncrementswhile advancing phases and polyBLEP—same math, far fewer transcendental calls when those oscillator modes are active.This is a CPU-focused change in the real-time render path; output should be unchanged because
frequencyis already fixed for the duration of each callback buffer.Reviewed by Cursor Bugbot for commit d09e978. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by Sourcery
Enhancements: