perf(spectrum): reuse GPU FFT-trace vertex scratch buffers + guard 1-bin spectrum#3782
Conversation
…ethersdr#3781) renderGpuFrame() allocated three QVectors every frame to build the GPU FFT trace -- the line and fill vertex buffers (up to 2*kMaxFftBins*kFftVertStride floats each) plus the point buffer -- then freed them at end of frame. At the FFT frame rate on the GUI thread this is up to ~850 KB of heap alloc+free per frame of pure churn. Hoist them to reused members (m_fftLineScratch / m_fftFillScratch / m_fftPtScratch); resize() keeps capacity, so after warm-up the steady state is allocation-free. Mechanically identical output -- every element is overwritten each frame as before. Measured: steady-state renderP95 unchanged (0.3 ms, within noise -- the alloc was already sub-millisecond); the benefit is removing per-frame GUI-thread heap traffic, not frame time. Also guards trace generation + draw on n >= 2: the central-difference normal reads pts[i+-1] and the position step divides by (n-1), so a 1-bin spectrum would divide by zero and read out of bounds. n is always >> 2 in practice; defensive. Principle VII -- Untrusted Input Is Validated At The Boundary: the n >= 2 guard validates the incoming FFT bin count before indexing. Principle XI -- measured and reported honestly (within noise, not overstated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Reviewed the diff against the current renderGpuFrame() source. Both changes are correct and low-risk — thanks for the careful, honestly-reported writeup.
Scratch reuse (m_fftLineScratch / m_fftFillScratch / m_fftPtScratch). Verified the buffers are fully overwritten every frame before use — resize() followed by element-by-element writes across the generation loop (lineVerts[li..], fillVerts[fi..], pts[i]), so there's no stale-data hazard from persisting them as members. They're touched only inside renderGpuFrame() (single-threaded GUI/render path), so hoisting to members introduces no aliasing concern. Output is byte-identical, as claimed. Moving Pt → FftScratchPt in the header is a clean, necessary consequence. The "not a frame-time win, removes GUI-thread heap churn" framing is accurate and appreciated.
n >= 2 guard. Confirmed this closes real UB for a degenerate 1-bin spectrum: pts[i].x = 2.0f*i/(n-1) - 1.0f divides by zero, and the i == 0 normal branch reads pts[1] out of bounds. The important detail you got right is applying the same threshold to both the generation block and the draw block — since the FFT VBOs persist across frames, guarding only one side could have left cb->draw(n*2) reading a stale/partially-written buffer. With both gated identically, skip-generation and skip-draw stay consistent. Good.
One optional, non-blocking note: the software paint path generates its own FFT trace vertices and isn't touched here — if a 1-bin spectrum is genuinely reachable (rather than purely defensive), the same degenerate-input guard would belong there too for render-path parity. Out of scope for this PR; flagging only so it's on the record.
Nothing blocking from me. Clean change, accurate measurements, correct boundary fix. 👍
🤖 aethersdr-agent · cost: $1.6899 · model: claude-opus-4-8
|
Reviewed and tested locally by cherry-picking this onto current I don’t see any blockers. The scratch-buffer reuse looks mechanically safe: the line/fill/point buffers are resized and then fully overwritten before each upload, and they remain confined to the GPU render path. The I would describe this as allocator-pressure cleanup rather than a measured frame-time improvement: the PR’s own |
rfoust
left a comment
There was a problem hiding this comment.
Approved based on the current-main local build and code review above. The change is narrow, build-clean locally, and I do not see blocking risk in the GPU FFT scratch-buffer reuse or the n >= 2 guard.
Summary
Fixes #3781.
Two low-risk hardening changes to the GPU FFT-trace vertex generation in
renderGpuFrame():Reuse vertex scratch buffers across frames. The line/fill vertex arrays (up to
2*kMaxFftBins*kFftVertStridefloats each) and the point array were allocated and freed every frame — up to ~850 KB of heap alloc+free per frame on the GUI thread. Hoisted to reused members (m_fftLineScratch/m_fftFillScratch/m_fftPtScratch);resize()keeps capacity, so steady state is allocation-free. Output is mechanically identical (every element is overwritten each frame as before).Guard
n >= 2. For a 1-bin spectrum the generator divides by(n - 1)and readspts[1]out of bounds (central-difference normal).nis always ≫ 2 in practice; this is defensive boundary validation. Applied to both the generation and draw blocks.Measurements
renderP95 unchanged at 0.3 ms (i9-13900K / RTX 4090, GPU path) — the allocation was already sub-millisecond, so this is not a frame-time win, reported honestly. The value is removing per-frame GUI-thread heap traffic + closing the degenerate-input UB.
A parallel per-row allocation in
updateWaterfallRow()was considered and deliberately left alone:m_prevTileScanline = scanlineshares the buffer into a persistent member each row, so a member-hoist would detach (re-allocate) on the nextresize()regardless — zero reuse — andwfUpdateP95is 0.1 ms for the whole function. Not worth the COW/aliasing risk.Constitution principle honored
Principle VII — Untrusted Input Is Validated At The Boundary (the
n >= 2guard validates the incoming bin count before indexing). Principle XI — Fixes Are Demonstrated (measured before/after; reported as within-noise rather than overstated).Test plan
Checklist
AppSettingscalls (N/A)MeterSmoother(N/A)