Choose the clutter convolution FFT length by measurement - #60
Closed
Purple10101 wants to merge 3 commits into
Closed
Purple10101 wants to merge 3 commits into
Purple10101 wants to merge 3 commits into
Conversation
WienerHopf planned fftFiltX/fftFiltW/fftFilt at nBins + nSamples + 1. At the shipped config (fs 2e6, cpi 0.5, delay -10..400) that is 1000411 = 269 x 3719, both prime, so FFTW falls back to Rader instead of a fast codelet. The length only has to be at least the linear-convolution length (nSamples + nBins - 1) for the result to be alias-free, and only the first nSamples outputs are read, so it is free to round up. nextFastFftLength() picks the smallest length built from 2/3/5/7 with at most one factor of 11 or 13, giving 1002375 for the shipped config. This is structural rather than bad luck: the length depends on CPI and delay span, so filter cost jumps unpredictably whenever either is touched. Ambiguity already solves the same problem with next_hamming(); the clutter filter never got the treatment. Also fixes a latent index bug the ported test caught: `i - delayMin` promotes to unsigned, so a positive delayMin wraps at 2^32 and reads the wrong sample. Negative and zero delayMin cancel exactly, so the shipped config (-10) was unaffected. Measured on x86, production geometry, pinned to 4 cores, median of 6: clutter filter 281.12 ms -> 200.63 ms (1.40x). Output differs only by floating-point reassociation: RMS relative difference 6.05e-14, max relative error 2.95e-15, far below the 1e-6 regression tolerance. Ported from mickeyslaven/blah2-VectorWarp de45d9b (MIT). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit padded the clutter convolution to the smallest length built from 2/3/5/7 (plus at most one 11 or 13). On x86 that measured 1.40x faster. On the Pi 5 it is a regression, and an A-B-A live swap on owl-ded9 against v0.4.3 measured it: A1 v0.4.3 n=76 clutter_filter 763.2 ms cpi 1211.5 ms B padded n=47 clutter_filter 828.8 ms cpi 1276.5 ms A2 v0.4.3 n=46 clutter_filter 758.7 ms cpi 1206.1 ms That is +67.9 ms (0.918x), Welch t = 12.7, against a -4.5 ms A1/A2 drift on identical code. v0.4.3 to this branch's parent changes no C++ at all, so the comparison isolates the padding. The cause is that blah2.cpp calls fftw_plan_with_nthreads(4), so every plan is a 4-thread plan. FFTW parallelises across a Cooley-Tukey factor, so how evenly the factors divide the thread count matters more than how small they are. Benchmarked on a Pi 5 against the image's own NEON FFTW, 2 forward plus 1 backward, best of 5: length 1 thread 4 threads 1016064 = 2^8*3^4*7^2 94.24 ms 1036800 = 2^9*3^4*5^2 140.23 ms 1048576 = 2^20 248.07 ms 151.18 ms 1000411 = 269*3719 466.03 ms 255.86 ms (unpadded) 1003520 = 2^12*5*7^2 172.30 ms 266.23 ms 1002375 = 3^6*5^3*11 273.55 ms 393.63 ms (what we padded to) Single-threaded the padding is a 1.7x win; threaded it is a 1.54x loss. The ordering is not predictable from the factorisation either way: 2^8*3^4*7^2 beats 2^20 by 1.6x, and 2^13*5^3 is worse than two large primes. Any static rule is guesswork, so stop writing one. fastestFftLength() times the admissible candidates within 2% of the minimum, plus the unpadded length itself, and keeps the quickest. It runs once in a constructor, after the thread count is set, in a process that ran 139 days without a restart on jn1. nextFastFftLength() stays: it is the fallback when nothing can be planned or allocated, and the test still pins its arithmetic. The clutter test now asserts the properties the convolution depends on (at least the alias-free length, and one of the offered candidates) rather than a fixed length that varies by machine, and covers the candidate enumeration directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Timing the candidate lengths costs about 7 s on a Pi 5 (15 candidates, each planned and run four times at ~1e6 points), paid on every start on top of the existing sleep(5). That roughly doubles time to first CPI, which matters most on the nodes that restart most. The answer only changes when the geometry, the thread count or the FFTW build changes, so cache it against exactly those three. The file lives in the save directory, which is a host mount, so it survives a container restart; BLAH2_FFT_CACHE overrides the path. Only the first start after a change pays the sweep. A few entries are kept rather than one, so moving a node between configurations and back does not force a re-measurement each way. The cache is an optimisation and never a contract. A corrupt line, an unreadable file, a read-only or missing save mount, or a remembered length shorter than the geometry now needs, all fall back to measuring. Nothing about correctness rides on it: every candidate is at least the alias-free convolution length, so a stale entry costs speed, never accuracy. fftw_plan_with_nthreads(4) becomes kPlannerThreads, shared with the cache key, since the ranking inverts between 1 and 4 threads and the key would otherwise silently lie. Tests cover the round trip (and assert the cached path is quicker than measuring), isolation between geometries, no eviction when a second geometry is added, recovery from a corrupt file, and an unwritable path. Built and run on a Pi 5 against the deployed FFTW: all geometries pass, including the positive delayMin case that exercises the signed-index fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 15, 2026
Author
|
Closing for now. The work is not lost: the branch stays on the remote and the measurements are recorded in the PR description above. Reopen or re-raise when there is a decision on how to take this forward. |
1 similar comment
Author
|
Closing for now. The work is not lost: the branch stays on the remote and the measurements are recorded in the PR description above. Reopen or re-raise when there is a decision on how to take this forward. |
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.
Pads the clutter convolution to a faster FFT length, but picks that length by timing candidates on the machine that will run them rather than by a formula, and caches the result.
Measured on owl-ded9 (Pi 5), live A-B-A against v0.4.3, 60 s per run, pooled control n=172:
-257.2 ms on the filter, Welch t = -54.3, p10-p90 bands fully separated.
Read the commits in order, the first one is superseded
dee78bdports the upstream heuristic and its message claims 1.40x, measured on x86. That number is real but does not hold on ARM, where the same change is a 1.54x regression.c4b3db9documents that and replaces the rule. Please don't reviewdee78bdin isolation.Why the formula was wrong
WienerHopfplanned atnSamples + nBins + 1, which at the shipped geometry is 1,000,411 = 269 x 3719, both prime, so FFTW falls back to Rader. Padding is free: the transform only has to reach the alias-free lengthnSamples + nBins - 1, and only the firstnSamplesoutputs are read.The trap is that blah2 calls
fftw_plan_with_nthreads(4). FFTW parallelises across a Cooley-Tukey factor, so how evenly the factors divide the thread count matters more than how small they are. Benchmarked on a Pi 5 against the image's own NEON FFTW, 2 forward plus 1 backward:Note
2^8 x 3^4 x 7^2beats2^20by 1.6x and2^13 x 5^3is worse than two large primes, so "prefer powers of two" does not work either. Hence measuring.Startup cost
Timing 15 candidates costs 8.2 s, so the answer is cached against geometry, slack, thread count and FFTW build, in
/opt/blah2/save/fft-length.cache(host mount, survives container restart,BLAH2_FFT_CACHEoverrides). Cold start 15.39 s, warm 7.16 s.The cache is an optimisation and never a contract: a corrupt line, unreadable file, missing mount or too-short remembered length all fall back to measuring. Every candidate clears the alias-free length, so a stale entry costs speed, never accuracy.
Verification
Output differs only by floating-point reassociation, ~6e-14 relative, about 264 dB down, far under thermal noise.
testClutterFftvalidates the filtered output against an independent direct time-domain convolution to 1e-9, built and run on a Pi 5 against the deployed FFTW, passing for all geometries including positivedelayMin.Also fixes a latent bug:
i - delayMinpromoted to unsigned, so a positivedelayMinwrapped at 2^32 and read the wrong sample. The shipped -10 was unaffected.Known issues
testjob is a false green. It prints "No unit tests directory found" and exits 0, because the runtime image copies only/blah2/bin/blah2. Tests compile on ARM but never execute. Worth fixing before this merges, since the branch leans on them.🤖 Generated with Claude Code