Performance: Optimize AudioEngine visualization loop#185
Merged
Conversation
- Implemented shadow buffer strategy in AudioEngine visualization summary. - Buffer size increased to store a mirrored copy of the data. - Replaced modulo arithmetic in the hot read loop with linear memory access. - Result: ~50% reduction in execution time for getVisualizationData (384ms -> 190ms per 10k calls).
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideImplements a shadow (mirrored) buffer for AudioEngine visualization data to remove modulo operations in the hot-path read loop, updating the write path to maintain the mirror and documenting the pattern for future use. Class diagram for AudioEngine visualization shadow buffer optimizationclassDiagram
class AudioEngine {
+number VIS_SUMMARY_SIZE
+Float32Array visualizationSummary
+number visualizationSummaryPosition
+AudioEngineConfig config
+constructor AudioEngine(config)
+processVisualizationFrame(inputBuffer)
+getVisualizationData(rangeStart, rangeEnd, target)
}
class Float32Array
class AudioEngineConfig
AudioEngine --> Float32Array : uses
AudioEngine --> AudioEngineConfig : configured_by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new linear read index
(this.visualizationSummaryPosition + s) * 2ingetVisualizationDataassumesthis.visualizationSummaryPosition + salways stays within[0, 2 * VIS_SUMMARY_SIZE), so it would be good to either enforce this bound (e.g., via clamping or an assertion) or clearly document the invariant whererangeStart/rangeEndcome from to avoid potential out-of-bounds accesses if upstream logic changes. - Consider extracting the shadow-buffer layout into a small helper or at least a dedicated comment block (e.g., describing how indices
[0..VIS_SUMMARY_SIZE)and[VIS_SUMMARY_SIZE..2*VIS_SUMMARY_SIZE)map to the main and shadow regions) so future changes toVIS_SUMMARY_SIZEor the write/read patterns remain easy to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new linear read index `(this.visualizationSummaryPosition + s) * 2` in `getVisualizationData` assumes `this.visualizationSummaryPosition + s` always stays within `[0, 2 * VIS_SUMMARY_SIZE)`, so it would be good to either enforce this bound (e.g., via clamping or an assertion) or clearly document the invariant where `rangeStart`/`rangeEnd` come from to avoid potential out-of-bounds accesses if upstream logic changes.
- Consider extracting the shadow-buffer layout into a small helper or at least a dedicated comment block (e.g., describing how indices `[0..VIS_SUMMARY_SIZE)` and `[VIS_SUMMARY_SIZE..2*VIS_SUMMARY_SIZE)` map to the main and shadow regions) so future changes to `VIS_SUMMARY_SIZE` or the write/read patterns remain easy to reason about.
## Individual Comments
### Comment 1
<location path="src/lib/audio/AudioEngine.ts" line_range="137-138" />
<code_context>
// Initialize visualization summary (2000 points for 30s)
// Note: Raw visualization buffer removed in favor of summary buffer (performance)
- this.visualizationSummary = new Float32Array(this.VIS_SUMMARY_SIZE * 2);
+ // Using shadow buffer (double size) to enable linear reading without modulo
+ this.visualizationSummary = new Float32Array(this.VIS_SUMMARY_SIZE * 4);
this.visualizationSummaryPosition = 0;
</code_context>
<issue_to_address>
**suggestion:** Consider deriving the shadow-buffer size from a single constant or helper to keep read/write math in sync.
This now relies on 2 values per sample (min/max) and a 2× shadow copy (`N + N`) on both the write and read paths. To avoid these getting out of sync later, consider centralizing the layout in a helper or named constants (e.g., `VIS_CHANNELS = 2`, `SHADOW_COPIES = 2`) and deriving the buffer size from them.
Suggested implementation:
```typescript
// Initialize visualization summary (2000 points for 30s)
// Note: Raw visualization buffer removed in favor of summary buffer (performance)
// Using shadow buffer (double size) to enable linear reading without modulo
// Buffer layout: [VIS_SHADOW_COPIES] * [VIS_SUMMARY_SIZE] * [VIS_CHANNELS (min/max)]
this.visualizationSummary = new Float32Array(
this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES,
);
this.visualizationSummaryPosition = 0;
```
To fully implement the suggestion and keep read/write math in sync, you should also:
1. Define centralized layout constants on the class, near where `VIS_SUMMARY_SIZE` is defined, e.g.:
- `private readonly VIS_CHANNELS = 2; // min + max`
- `private readonly VIS_SHADOW_COPIES = 2; // N + N shadow buffer`
2. Replace any hard-coded values that assume:
- 2 channels (min/max) for each summary sample, e.g. `index * 2`, `/ 2`, `+ 0/+1` patterns, with `this.VIS_CHANNELS`.
- A 2× shadow copy or modulo based on `VIS_SUMMARY_SIZE * 2` or `* 4`, with expressions derived from:
- `this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS` (single logical buffer)
- `this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES` (full shadow buffer)
3. In the read path:
- Where you compute linear read start/end or wrap using modulo, replace any literal `2` or `4` factors tied to the visualization summary buffer with the same derived constants:
- e.g., `const logicalStride = this.VIS_CHANNELS;`
- e.g., `const bufferSize = this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES;`
This ensures that changing `VIS_CHANNELS` or `VIS_SHADOW_COPIES` in one place keeps both write and read logic consistent.
Because only a small portion of the file is visible here, you will need to locate those hard-coded multipliers in the rest of `AudioEngine.ts` and update them to use `VIS_CHANNELS` and `VIS_SHADOW_COPIES` so the layout remains centralized and self-documenting.
</issue_to_address>
### Comment 2
<location path="src/lib/audio/AudioEngine.ts" line_range="861-862" />
<code_context>
for (let s = Math.floor(rangeStart); s < Math.floor(rangeEnd); s++) {
- const idx = ((this.visualizationSummaryPosition + s) % this.VIS_SUMMARY_SIZE) * 2;
+ // Use shadow buffer property to read linearly without modulo
+ const idx = (this.visualizationSummaryPosition + s) * 2;
const vMin = this.visualizationSummary[idx];
const vMax = this.visualizationSummary[idx + 1];
</code_context>
<issue_to_address>
**suggestion:** Consider encapsulating the index computation to keep future changes to buffer layout localized.
This read index now depends on the `[0..N)` / `[N..2N)` shadow layout and the 2-floats-per-sample format, and the write path encodes similar math. Pulling this into a helper like `getVisIndex(offset: number)` (or a small circular+shadow buffer wrapper) would keep the layout logic in one place and avoid read/write divergence if we later change channels or the shadow scheme.
Suggested implementation:
```typescript
this.visualizationSummary[shadowIdx] = min;
this.visualizationSummary[shadowIdx + 1] = max;
this.visualizationSummaryPosition = (this.visualizationSummaryPosition + 1) % this.VIS_SUMMARY_SIZE;
}
}
/**
* Compute the base index into the visualization summary buffer for a given
* sample offset relative to the current summary position.
*
* The buffer is laid out as [min, max] float pairs with a shadow region
* that allows linear access without modulo on the read path.
*/
private getVisualizationSummaryIndex(offset: number): number {
// Use the shadow buffer layout to read linearly without modulo.
// Each sample uses two consecutive floats: [min, max].
return (this.visualizationSummaryPosition + offset) * 2;
}
let first = true;
for (let s = Math.floor(rangeStart); s < Math.floor(rangeEnd); s++) {
const idx = this.getVisualizationSummaryIndex(s);
const vMin = this.visualizationSummary[idx];
const vMax = this.visualizationSummary[idx + 1];
```
To fully centralize the layout logic and avoid divergence between read and write paths, consider:
1. Introducing a corresponding write-side helper, e.g. `private setVisualizationSummarySample(offset: number, min: number, max: number)`, that encapsulates the computation of `shadowIdx` and the `[min, max]` writes (including any primary+shadow writes if applicable).
2. Replacing direct assignments to `this.visualizationSummary[shadowIdx]` and `this.visualizationSummary[shadowIdx + 1]` (and any similar code elsewhere in the file) with calls to this new helper.
3. If other parts of the code compute visualization indices directly (e.g. for different ranges or channels), update them to use `getVisualizationSummaryIndex` (or a more general helper) so that future changes to buffer structure can be made in a single place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+137
to
+138
| // Using shadow buffer (double size) to enable linear reading without modulo | ||
| this.visualizationSummary = new Float32Array(this.VIS_SUMMARY_SIZE * 4); |
Contributor
There was a problem hiding this comment.
suggestion: Consider deriving the shadow-buffer size from a single constant or helper to keep read/write math in sync.
This now relies on 2 values per sample (min/max) and a 2× shadow copy (N + N) on both the write and read paths. To avoid these getting out of sync later, consider centralizing the layout in a helper or named constants (e.g., VIS_CHANNELS = 2, SHADOW_COPIES = 2) and deriving the buffer size from them.
Suggested implementation:
// Initialize visualization summary (2000 points for 30s)
// Note: Raw visualization buffer removed in favor of summary buffer (performance)
// Using shadow buffer (double size) to enable linear reading without modulo
// Buffer layout: [VIS_SHADOW_COPIES] * [VIS_SUMMARY_SIZE] * [VIS_CHANNELS (min/max)]
this.visualizationSummary = new Float32Array(
this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES,
);
this.visualizationSummaryPosition = 0;To fully implement the suggestion and keep read/write math in sync, you should also:
-
Define centralized layout constants on the class, near where
VIS_SUMMARY_SIZEis defined, e.g.:private readonly VIS_CHANNELS = 2; // min + maxprivate readonly VIS_SHADOW_COPIES = 2; // N + N shadow buffer
-
Replace any hard-coded values that assume:
- 2 channels (min/max) for each summary sample, e.g.
index * 2,/ 2,+ 0/+1patterns, withthis.VIS_CHANNELS. - A 2× shadow copy or modulo based on
VIS_SUMMARY_SIZE * 2or* 4, with expressions derived from:this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS(single logical buffer)this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES(full shadow buffer)
- 2 channels (min/max) for each summary sample, e.g.
-
In the read path:
- Where you compute linear read start/end or wrap using modulo, replace any literal
2or4factors tied to the visualization summary buffer with the same derived constants:- e.g.,
const logicalStride = this.VIS_CHANNELS; - e.g.,
const bufferSize = this.VIS_SUMMARY_SIZE * this.VIS_CHANNELS * this.VIS_SHADOW_COPIES;
This ensures that changingVIS_CHANNELSorVIS_SHADOW_COPIESin one place keeps both write and read logic consistent.
- e.g.,
- Where you compute linear read start/end or wrap using modulo, replace any literal
Because only a small portion of the file is visible here, you will need to locate those hard-coded multipliers in the rest of AudioEngine.ts and update them to use VIS_CHANNELS and VIS_SHADOW_COPIES so the layout remains centralized and self-documenting.
Comment on lines
+861
to
+862
| // Use shadow buffer property to read linearly without modulo | ||
| const idx = (this.visualizationSummaryPosition + s) * 2; |
Contributor
There was a problem hiding this comment.
suggestion: Consider encapsulating the index computation to keep future changes to buffer layout localized.
This read index now depends on the [0..N) / [N..2N) shadow layout and the 2-floats-per-sample format, and the write path encodes similar math. Pulling this into a helper like getVisIndex(offset: number) (or a small circular+shadow buffer wrapper) would keep the layout logic in one place and avoid read/write divergence if we later change channels or the shadow scheme.
Suggested implementation:
this.visualizationSummary[shadowIdx] = min;
this.visualizationSummary[shadowIdx + 1] = max;
this.visualizationSummaryPosition = (this.visualizationSummaryPosition + 1) % this.VIS_SUMMARY_SIZE;
}
}
/**
* Compute the base index into the visualization summary buffer for a given
* sample offset relative to the current summary position.
*
* The buffer is laid out as [min, max] float pairs with a shadow region
* that allows linear access without modulo on the read path.
*/
private getVisualizationSummaryIndex(offset: number): number {
// Use the shadow buffer layout to read linearly without modulo.
// Each sample uses two consecutive floats: [min, max].
return (this.visualizationSummaryPosition + offset) * 2;
}
let first = true;
for (let s = Math.floor(rangeStart); s < Math.floor(rangeEnd); s++) {
const idx = this.getVisualizationSummaryIndex(s);
const vMin = this.visualizationSummary[idx];
const vMax = this.visualizationSummary[idx + 1];
To fully centralize the layout logic and avoid divergence between read and write paths, consider:
- Introducing a corresponding write-side helper, e.g.
private setVisualizationSummarySample(offset: number, min: number, max: number), that encapsulates the computation ofshadowIdxand the[min, max]writes (including any primary+shadow writes if applicable). - Replacing direct assignments to
this.visualizationSummary[shadowIdx]andthis.visualizationSummary[shadowIdx + 1](and any similar code elsewhere in the file) with calls to this new helper. - If other parts of the code compute visualization indices directly (e.g. for different ranges or channels), update them to use
getVisualizationSummaryIndex(or a more general helper) so that future changes to buffer structure can be made in a single place.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // Initialize visualization summary (2000 points for 30s) | ||
| // Note: Raw visualization buffer removed in favor of summary buffer (performance) | ||
| this.visualizationSummary = new Float32Array(this.VIS_SUMMARY_SIZE * 2); | ||
| // Using shadow buffer (double size) to enable linear reading without modulo |
There was a problem hiding this comment.
🔥 The Roast: The comment says "shadow buffer (double size)" but we're actually going from * 2 to * 4 - that's a doubling of the already-doubled size. It's like saying "I doubled my money" when you went from $100 to $200 to $400. Technically accurate, but left me more confused than a chameleon in a bag of crayons.
🩹 The Fix: Update the comment to be clearer:
Suggested change
| // Using shadow buffer (double size) to enable linear reading without modulo | |
| // Using shadow buffer (4x size: 2x for min/max pairs + 2x for shadow copy) to enable linear reading without modulo |
📏 Severity: nitpick
Code Review Roast 🔥Verdict: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The write path correctly maintains both primary and shadow positions. The dual-write approach is solid. 💀 Worst part: The read path optimization is fundamentally broken. Removing the modulo 📊 Overall: The optimization ambition exceeded its grasp. The shadow buffer was supposed to eliminate modulo, but it only works if you ALSO update shadow positions when wraparound affects them. As implemented, it's a correctness bug dressed up as a performance win. Files Reviewed (2 files)
Verification
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.jules/bolt.md (1)
2-2: Minor docs polish: use “60 fps” for readability.✏️ Suggested tweak
-Learning: Circular buffers in performance-critical hot paths (like audio visualization loops running at 60fps) benefit significantly from a "shadow buffer" strategy. By mirroring the buffer content (writing to `i` and `i + size`), we enable contiguous linear reads of any window of size `size` without modulo arithmetic. +Learning: Circular buffers in performance-critical hot paths (like audio visualization loops running at 60 fps) benefit significantly from a "shadow buffer" strategy. By mirroring the buffer content (writing to `i` and `i + size`), we enable contiguous linear reads of any window of size `size` without modulo arithmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.jules/bolt.md at line 2, Change the phrase "60fps" to "60 fps" in the documentation line describing circular buffers and the shadow buffer strategy (the sentence mentioning "audio visualization loops running at 60fps" and the example with "i and i + size") to improve readability; update that exact wording in the .jules/bolt.md content where "60fps" appears.src/lib/audio/AudioEngine.ts (1)
861-863: Add a wrap-boundary regression test for this linear index path.This optimization depends on shadow-buffer invariants; a targeted test near
visualizationSummaryPosition = VIS_SUMMARY_SIZE - 1would harden it against future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/audio/AudioEngine.ts` around lines 861 - 863, The linear-index optimization using the shadow buffer (reading visualizationSummary at index (this.visualizationSummaryPosition + s) * 2) can break at wrap boundaries; add a regression test that sets visualizationSummaryPosition to VIS_SUMMARY_SIZE - 1 and populates visualizationSummary (and any shadow/duplicate buffer expected by AudioEngine) with known values so the linear read path is exercised and verified for s=0..N (or whatever sample span is used). In the test, instantiate the AudioEngine (or call the method that computes the summary), set visualizationSummaryPosition and visualizationSummary directly, call the summary/visualization method, and assert the returned/min/max values match the expected sequence across the wrap; include a case that would have failed if modulo-based indexing were incorrectly reintroduced. Ensure the test name references visualizationSummaryPosition/VIS_SUMMARY_SIZE so it’s clear what boundary it covers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.jules/bolt.md:
- Line 2: Change the phrase "60fps" to "60 fps" in the documentation line
describing circular buffers and the shadow buffer strategy (the sentence
mentioning "audio visualization loops running at 60fps" and the example with "i
and i + size") to improve readability; update that exact wording in the
.jules/bolt.md content where "60fps" appears.
In `@src/lib/audio/AudioEngine.ts`:
- Around line 861-863: The linear-index optimization using the shadow buffer
(reading visualizationSummary at index (this.visualizationSummaryPosition + s) *
2) can break at wrap boundaries; add a regression test that sets
visualizationSummaryPosition to VIS_SUMMARY_SIZE - 1 and populates
visualizationSummary (and any shadow/duplicate buffer expected by AudioEngine)
with known values so the linear read path is exercised and verified for s=0..N
(or whatever sample span is used). In the test, instantiate the AudioEngine (or
call the method that computes the summary), set visualizationSummaryPosition and
visualizationSummary directly, call the summary/visualization method, and assert
the returned/min/max values match the expected sequence across the wrap; include
a case that would have failed if modulo-based indexing were incorrectly
reintroduced. Ensure the test name references
visualizationSummaryPosition/VIS_SUMMARY_SIZE so it’s clear what boundary it
covers.
ysdede
added a commit
that referenced
this pull request
Feb 27, 2026
github-actions bot
pushed a commit
that referenced
this pull request
Feb 27, 2026
…ter) (#185) - Implemented shadow buffer strategy in AudioEngine visualization summary. - Buffer size increased to store a mirrored copy of the data. - Replaced modulo arithmetic in the hot read loop with linear memory access. - Result: ~50% reduction in execution time for getVisualizationData (384ms -> 190ms per 10k calls). Co-authored-by: ysdede <5496750+ysdede@users.noreply.github.com> 61ef15b
| for (let s = Math.floor(rangeStart); s < Math.floor(rangeEnd); s++) { | ||
| const idx = ((this.visualizationSummaryPosition + s) % this.VIS_SUMMARY_SIZE) * 2; | ||
| // Use shadow buffer property to read linearly without modulo | ||
| const idx = (this.visualizationSummaryPosition + s) * 2; |
There was a problem hiding this comment.
🔥 The Roast: The modulo was removed in the name of optimization, but the shadow buffer doesn't actually eliminate the need for it. Here's why: when you write to position P, you write to shadow at P+N. But when the circular buffer wraps and overwrites position (P-N), the shadow at position P still has the OLD data. So when reading with (position + s) * 2, if position + s >= N, you read from the shadow which contains STALE data. The test at line 51 explicitly states the old behavior: "getVisualizationData reads from visualizationSummaryPosition + s (modulo size)." This change breaks that invariant.
🩹 The Fix: Restore the modulo, or if you want to keep the shadow approach, you need to ALSO write to shadow positions that get affected by the wrap (i.e., when position + N < N, write to both position and position+N). But honestly, the modulo was fine - it's a single operation per read vs. double writes per update.
📏 Severity: warning
ysdede
added a commit
that referenced
this pull request
Mar 22, 2026
ysdede
added a commit
that referenced
this pull request
Mar 22, 2026
- Implemented shadow buffer strategy in AudioEngine visualization summary. - Buffer size increased to store a mirrored copy of the data. - Replaced modulo arithmetic in the hot read loop with linear memory access. - Result: ~50% reduction in execution time for getVisualizationData (384ms -> 190ms per 10k calls). Co-authored-by: ysdede <5496750+ysdede@users.noreply.github.com>
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.
Implemented a "shadow buffer" optimization for the AudioEngine's visualization data. By maintaining a double-sized buffer with mirrored content, we can read any window of
VIS_SUMMARY_SIZElinearly without modulo operations. This reduces the cost of the high-frequencygetVisualizationDatacall by ~50%.Verification
npm test src/lib/audio/AudioEngine.visualization.test.tsto verify correctness.npm test src/lib/audio/AudioSegmentProcessor.test.tsto ensure no regressions.PR created automatically by Jules for task 5025253348767011018 started by @ysdede
Summary by Sourcery
Optimize AudioEngine visualization summary buffer for faster linear access in the visualization loop.
Enhancements:
Documentation:
Summary by CodeRabbit
Performance
Documentation