CPU hygiene wins from VectorWarp: deque copies, JSON, ambiguity, CFAR - #65
Merged
Merged
Conversation
IqData::get_data() returns the deque by value. It was called three times a CPI, once in SpectrumAnalyser and twice in WienerHopf. At a million samples that is 16 MB and roughly 31,000 deque-chunk allocations each, so about 48 MB and 94,000 allocations a CPI to read data nothing then mutates. view_data() returns a const reference instead. Measured on a Pi 5, one copy of a 1,000,000-sample deque<complex<double>> is 10.95 ms, so the three cost 32.85 ms a CPI. Reading the same samples through the reference costs 3.10 ms, which is work that happens either way. SpectrumAnalyser also built a full-length fftshift vector with push_back and no reserve, then read every decimation-th element out of it. The shift is now applied in the decimation loop, which touches exactly the same elements, and the output vector is reserved. Same values, one fewer million-element vector. Deliberately not touched: the normalisation and the frequency axis in the same function. The frequency loop is a known latent bug (i is unsigned, so i = -nSpectrum/2 wraps and the loop never runs, leaving the axis empty) but fixing it changes what the front end displays and deserves its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
to_json() built a RapidJSON DOM over the whole map, 301 x 411 = 123,711 doubles at the shipped geometry, and serialised it. delay_bin_to_km() then parsed all 665 KB of that back into a second DOM, rewrote 411 delay values and serialised the lot again. The map went through the writer twice and the parser once so that one axis could be converted to km. to_json_km() writes it once, straight to the buffer, with no DOM at either end and the delay axis already converted. Measured on a Pi 5 at the shipped geometry: 34.12 ms -> 19.21 ms, a saving of 14.91 ms a CPI. For reference owl-ded9 reports output_radar_data at 35.5 ms, so the bench matches the node. Key order and SetMaxDecimalPlaces(2) are unchanged, so the bytes are identical and nothing downstream can tell the difference. testJsonKm asserts exactly that against the old pair across eight cases, including the shipped geometry, both fleet Doppler spans, an empty detection list and values chosen to stress the decimal cap. The old pair is kept solely as that oracle: deleting it would delete the proof. Not taken from upstream: their 1e-30 log clamp and their dropped save.map bound, both of which change behaviour. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three avoidable copies in Ambiguity::process: The range loop staged 2*nDelayBins+1 values into dataCorr so that a window of nDelayBins could be read straight back out, then copied that window into a vector and handed it to set_row. The window is just dataZi at (j + delayMin), wrapped, so it now goes directly into the map. The Doppler loop called get_col(), which builds and returns a fresh vector per delay bin, then set_col(), which took another by value. A 301-deep column was copied four times over to be transformed once. Both ends now index map.data. set_row and set_col took their vector by value, copying it before the element-wise loop even started. They take a const reference now, which helps every caller. Measured on a Pi 5 at the shipped geometry this is worth 2.30 ms -> 0.76 ms, so 1.54 ms a CPI. That is well short of the 15.7 ms the upstream fork reports for this stage, and the remaining gap is not in these copies. Nor is it in the pop_front drain further up: removing that saves only about 1.9 ms per buffer, because IqData::push_back evicts when full, so the cost moves to the next extract rather than disappearing. Replacing the handoff with clear() + insert() measures 16.69 ms against the current 11.19 ms, so that apparently obvious optimisation is a regression. None of it is pursued here. Fixes a latent out-of-bounds read as a byproduct. dataCorr was 2*nDelayBins+1 long and the window started at nDelayBins + delayMin, so any delayMin >= 2 read past the end of it. The shipped -10 was unaffected. The rewrite has no staging array, so the bound cannot be exceeded, and testAmbiguityIndexing pins both that and index-for-index equivalence with the old arithmetic across both fleet Doppler spans. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Per cell, and there are 123,711 of them a CPI, the detector heap-allocated a std::vector<int> of training indices and called pow(pfa, -1.0/nCells). Per row it copied the row out of the map with get_row() and computed 10*log10() for every cell when only a detection needs it. The training window and the false-alarm factor depend on the delay bin, not on Doppler, so they are identical for every row. They are now built once a CPI into a table of nDelayBins entries. The training cells are summed by index range instead of through a vector of indices, still left then right in the original order, since a rolling or prefix sum rounds differently and would move the threshold. The row is read in place and the log is computed only on a detection. Measured on a Pi 5 at the shipped geometry: 34.97 ms -> 3.71 ms, a saving of 31.26 ms a CPI and a 9.4x on the stage. The upstream fork reports 36.4 -> 3.5 on their Pi 4, so this lands where they did. CHANGES DETECTION OUTPUT. The left training loop used k > 0 while the right used k >= 0, so bin 0 was excluded from the left window and included on the right. That asymmetry was a bug and it is corrected here to k >= 0. On synthetic maps with planted targets the count moves by at most one detection, always dropping a marginal one at the low-delay edge where the window was short. Real signal still needs a before and after on a node. testCfarHoist separates the two concerns. It asserts the hoisted detector is bit-identical, in count and in every delay, Doppler and SNR value, to an oracle that is a literal transcription of the original loop with the edge fix applied, so the performance rewrite is proven faithful on its own. It then runs the same oracle both ways to report what the edge fix alone moved. A cell whose window is empty is skipped rather than computed. The original produced a NaN threshold there, which no cell ever exceeded, so this is the same outcome without the arithmetic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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.
Four ordinary C++ hygiene changes, no algorithm changes, taken from the upstream fork
mickeyslaven/blah2-VectorWarp(MIT, 30hours copyright intact) and re-derived and re-measured on our hardware and geometry.Live A-B-A against current main
Control is a build of main, not the shipped v0.4.3, so the FFT-length gain merged in #64 is not double-counted. 240 s per phase, throughput from
nCpiadvance rather than polling (a tight poll loop costs ~200 ms/CPI on a saturated node):Baseline drift between the two control runs was 0.1% to 0.5%, so the effect is 20 to 90 times the noise. Both remote nodes were re-tested an hour later with fresh swaps and reproduced closely (1.166x/87.0 ms and 1.125x/78.5 ms).
The measured saving also matches the isolated component benchmarks, which predicted 80.6 ms/CPI. That agreement is worth as much as the headline: it means bench numbers for this pipeline transfer to the node.
The four changes
739a61bget_data()returned the sample deque by value, three times a CPI. At a million samples that is 16 MB and ~31,000 allocations each, so ~48 MB and ~94,000 allocations a CPI to read data nothing then mutates. Now a const reference. The spectrum stage also drops a full-lengthfftshiftvector built bypush_backwith no reserve.002b9f2e1f3c2252fc4b1pow()per cell, 123,711 times a CPI, and computed10*log10()for every cell when only detections need it. The window depends on delay, not Doppler, so it is now a table built once per CPI.The detector ratio reproduced to two decimal places on two different boards: 9.03x and 9.00x.
Verification
Each change is pinned by a test comparing against the code it replaces, not a snapshot. All run in CI as of #63.
to_json+delay_bin_to_kmpair across 8 cases, including the shipped geometry and values chosen to stress the decimal cap. The old path is kept deliberately as the oracle; deleting it would delete the proof.testClutterFft.Detection output changes, and has been checked on real signal
The CFAR left training loop used
k > 0while the right usedk >= 0, so delay bin 0 was excluded on one side and included on the other. That asymmetry was a bug and it is corrected here. On synthetic maps the count moves by at most one detection per map, always dropping a marginal one at the low-delay edge, and the test reports the hoist and the edge fix separately so they are never confused.fairforest-b, running this branch, still produces ADS-B-verified tracks, which exercises CFAR -> centroid -> interpolate -> tracker -> ADS-B truth correlation against independent ground truth. Note precisely what that shows: real aircraft are still detected and correlated. It is a presence check, not a sensitivity comparison, and would not reveal the one-marginal-detection effect predicted above. Judged sufficient alongside the synthetic bound. The fork owner separately verified radar performance on their own hardware.
Honest notes
delayMin >= 2. The shipped -10 was unaffected.pop_frontdrain saves only ~1.9 ms per buffer, becausepush_backevicts when full so the cost moves to the next extract. Replacing the handoff withclear()+insert()measures 16.69 ms against the current 11.19 ms, so the obvious optimisation is a regression.IqData'smin,maxandmeanare never assigned anywhere and are serialised every CPI.🤖 Generated with Claude Code