Skip to content

Optimizations - #132

Merged
IanButterworth merged 7 commits into
masterfrom
perf-postprocessing
Aug 7, 2026
Merged

Optimizations#132
IanButterworth merged 7 commits into
masterfrom
perf-postprocessing

Conversation

@IanButterworth

@IanButterworth IanButterworth commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Note

This PR was written by Claude (Claude Code), from a performance review requested and supervised by @IanButterworth.

Follow-up to #131 (now merged; this branch has been rebased onto master). This PR reduces post-processing cost when many detections are kept — the stage that scales with detection count — and picks up the two forward-pass wins that survived measurement. Every commit was verified against the stored reference results for v2_COCO, v2_tiny_COCO, v3_tiny_COCO and v3_COCO (bit-identical for the post-processing commits; < 1e-6 for the batchnorm fold), plus the NMS unit tests and the soft-NMS fuzz check.

All numbers below were re-measured after the rebase, on current master vs this branch, same machine back-to-back (M2 Pro, CPU, single image, Julia 1.12, current deps incl. TimerOutputs v1).

End-to-end benchmark

Typical settings (detect_thresh=0.5, overlap_thresh=0.5, default allocator, min of 15):

Model master this PR Δ
v3_416_COCO 336.7 ms 273.7 ms −19%
v3_tiny_416_COCO 41.5 ms 32.9 ms −21%
v4_416_COCO 589.3 ms 505.1 ms −14%

Post-processing benchmark

Worst case: v3_416_COCO with all 10,647 candidates kept (detect_thresh=0.0, overlap_thresh=1.0, disallow_bumper=true), TimerOutputs stage breakdown (min of 5):

Stage master this PR
processing outputs 10.9 ms / 18.5 MiB 11.0 ms / 5.4 MiB
filter detections 730 µs / 9.1 MiB 190 µs / 4.5 MiB
nms 13.7 ms / 53.9 MiB 11.4 ms / 14.2 MiB
post-processing total 25.4 ms / 81.5 MiB 22.7 ms / 24.1 MiB

The 3.4× allocation reduction is the headline for dense scenes: in continuous-inference loops, post-processing GC pressure compounds.

Post-processing commits

  1. Fuse post-processing broadcasts in place — the ~17 per-head transform statements were view[...] = RHS, each materializing the RHS array before copying; .= fuses them into single passes.
  2. Restructure NMS: single global sort, allocation-free IoU indexing — the big one, two parts:
    • perform_detection_nms scanned all columns per batch (findall), then per distinct class (Set + findall), then ran a per-group sortperm + matrix copy through nested index-vector views. Now one stable sortperm by (batch, class, −score) plus one gather makes every (batch, class) group a contiguous, already-sorted column range.
    • nms! allocated an index slice per suppression round to build its IoU view — O(N²) bytes per group, the bulk of the old 54 MiB. bboxiou! gained a column-index variant so rounds read through a view of the persistent index vector with zero per-round allocation.
    • ⚠️ One visible change (also in the CHANGELOG): output columns are now ordered by batch, then ascending class id, then descending score; classes were previously in first-appearance order. Within-class ordering is unchanged (stable sort).
  3. Flatten output blocks to attribute-major with a single allocationextend_for_attributes (zeros + cat) followed by permutedims copied each head's data three times; now a single permutedims! writes straight into the first a rows of the final (a+4)-row buffer. GPU arrays keep the dense cat+permutedims path via a new fast_scalar_indexing trait (CUDAExt sets it false for CuArray).
  4. Gather kept detections without materializing the concatenationkeepdetections needed cat(outweights...) plus a boolean-mask gather (two full copies); a new vector method counts and copies kept columns directly from the per-head matrices, with function barriers keeping the loops concretely typed. GPU falls back to the cat path via the same trait.

Forward pass

Measurement first (v3_416, CPU): the pass is gemm-bound, not allocation-bound — the default bumper path already recycles the ~890 MiB of forward allocations down to ~260 KiB with no time change, Julia threads do nothing (conv threading is BLAS-side: 1 BLAS thread = 842 ms, 6 = 317 ms), and AppleAccelerate drops v3_416 from ~317 ms to ~207 ms (−35%) with zero code changes. Two commits follow from this:

  1. Fold batchnorm into conv weights at load time (darknet's fuse_conv_batchnorm) — BN at inference is a per-channel affine map, so it folds exactly into the kernel and bias, deleting the BatchNorm pass over every conv output. This is the bulk of the end-to-end wins in the first table. Max abs deviation vs stored refs < 1e-6, so no reference updates. Side benefit: the positional Flux.BatchNorm internal constructor — a compat hazard across Flux versions flagged in the original review — is no longer used.
  2. README: CPU performance tips — AppleAccelerate on Apple silicon (~35%), MKL on Intel, BLAS threads as the parallelism knob, and batching for throughput.

Considered and deliberately not done

  • The remaining ~11 ms in "processing outputs" is real compute (~900k sigmoid/exp evaluations); faster approximations would change numerics.
  • The O(n²) IoU work when nothing gets suppressed is inherent to hard NMS at overlap_thresh=1.0; at realistic thresholds the loop shrinks as boxes are culled.
  • In-place conv! into the preallocated W buffers: mostly moot since the pass isn't allocation-bound.
  • A Metal.jl backend for Apple silicon is the big structural option (GPU-class speedup) but needs its own extension work — the post-processing kernels are CUDA-specific today.

Caveats

🤖 Generated with Claude Code

@IanButterworth
IanButterworth marked this pull request as draft August 5, 2026 20:19
@IanButterworth IanButterworth changed the title Reduce post-processing cost when many detections are kept Optimizations Aug 5, 2026
@IanButterworth
IanButterworth force-pushed the review-fixes branch 2 times, most recently from ba0d761 to b97f4c4 Compare August 5, 2026 20:51
Base automatically changed from review-fixes to master August 6, 2026 17:12
IanButterworth and others added 7 commits August 6, 2026 13:15
Convert the per-output transform assignments from A[...] = RHS (which
materializes RHS before copying into the view) to fused in-place .=
broadcasts. Outputs verified unchanged vs stored refs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two scaling problems when many detections reach NMS:

- perform_detection_nms scanned all columns once per batch (findall),
  then once per distinct class (Set + findall), then ran a per-group
  sortperm and matrix copy, all through nested index-vector views.
  Now all detections are ordered once by (batch, class, -score) with a
  single stable sortperm and one gather; every (batch, class) group is
  then a contiguous, already-sorted column range.
- nms! allocated an index slice per suppression round to build the IoU
  view (O(N^2) bytes across a group). bboxiou! gained a column-index
  variant so rounds now read through a view of the persistent index
  vector with no per-round allocation.

For v3_416 with all 10647 candidates kept (detect_thresh=0,
overlap_thresh=1), NMS drops 14.6 ms / 53.9 MiB -> 11.6 ms / 14.2 MiB.

Output content is unchanged (verified vs stored refs for v2, v2_tiny,
v3_tiny, v3, plus NMS unit tests and the soft-NMS fuzz check); column
order is now batch, then ascending class id, then descending score,
where classes were previously in first-appearance order.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
extend_for_attributes allocated a zeros block, cat copied everything,
permutedims copied everything again, and the batch-number fill ran on
the intermediate 5D layout. On the CPU path (Array/AllocArray) the
whole sequence is now one allocation: permutedims! straight into the
first `a` rows of the (a+4)-row destination, zero-fill of the 4
attribute rows, and a contiguous range fill for batch numbers on the
final layout.

GPU arrays keep the dense cat + permutedims path, gated by a new
fast_scalar_indexing trait that CUDAExt sets to false for CuArray.

"processing outputs" allocations drop 11.0 MiB -> 3.7 MiB for v3_416
with 10647 candidates. Outputs verified unchanged vs stored refs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
keepdetections previously required cat-ing all output heads into one
matrix, then built a boolean mask and gathered a second copy. A new
vector method counts and copies the kept columns from the per-head
matrices directly (function barriers keep the loops concretely typed;
GPU arrays fall back to the cat path via the fast_scalar_indexing
trait).

"filter detections" drops 743 us / 9.1 MiB -> 199 us / 4.5 MiB for
v3_416 with all 10647 candidates kept. Outputs verified unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
At inference batchnorm is a per-channel affine map, so fold it into the
conv kernel and bias when weights are loaded (as darknet's
fuse_conv_batchnorm does), removing the BatchNorm layer and its full
read+write pass over every conv output.

Forward pass on an M2 Pro (CPU, single image, min of 15):
v3_416 303.7 -> 290.4 ms, v3_tiny_416 36.0 -> 32.6 ms,
v4_416 543.7 -> 514.0 ms.

Numerics shift only by float rounding: max abs deviation vs the stored
reference detections is < 1e-6 across v2/v2_tiny/v3_tiny/v3, far inside
the test tolerance, so no reference updates are needed.

Side benefit: the positional Flux.BatchNorm internal constructor -- a
compat hazard across Flux versions -- is no longer used.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The forward pass is gemm-bound: AppleAccelerate is ~35% faster
end-to-end than the default OpenBLAS for v3_416 on an M2 Pro, and BLAS
thread count (not Julia threads) is what controls conv parallelism.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@IanButterworth
IanButterworth marked this pull request as ready for review August 6, 2026 17:30
@IanButterworth
IanButterworth merged commit 43a9f7f into master Aug 7, 2026
22 of 30 checks passed
@IanButterworth
IanButterworth deleted the perf-postprocessing branch August 7, 2026 02:17
IanButterworth added a commit that referenced this pull request Aug 7, 2026
Everything raised across the bug/performance/model-coverage/quality
review series that was not fixed in #131/#132/#137/#138, with effort
estimates and the explicitly-rejected items recorded so the reasoning
is not lost.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
IanButterworth added a commit that referenced this pull request Aug 7, 2026
…r deps (#138)

* Remove unused flipdict and createcountdict helpers

Neither has any call site in src, test, or examples.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Remove unused lhtan_grad activation gradient

Gradient kernels are training-only; the package does no training and
nothing references it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Remove dead cu_functional guard

cu_functional() held a CUDA.allowscalar(false) call but was never
invoked, so it never ran. allowscalar is a global CUDA.jl session flag,
so a package should not impose it on load either -- scalar-indexing
policy belongs to the user. Delete the dead function and its Ref.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Remove pre-1.9 extension fallback shim

Base.get_extension exists since Julia 1.9 and the package requires
1.10, so the include branch was unreachable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Drop redundant using statements in prepareimage.jl

The including module already imports these.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Parameterize AllocWrappedModel

Concretely-typed fields avoid dynamic dispatch on every call through
the wrapper, which is the default CPU entry path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Keep weights-file training metadata instead of discarding it

seen/seen_images were read (necessarily, to advance the stream) and
then dropped; store them in cfg where darknetversion already lives.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Key benchmark() by model name and cover all models

The hardcoded constructor list with numeric select indices was stale
(none of the six new models) and fragile. Iterate YOLO_MODELS keys
instead; each model loads at its native default size.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Fix benchmark table printing under PrettyTables v3

Compat allows PrettyTables 2 and 3, but v3 renamed the header kwarg to
column_labels, so benchmark()'s final table throw a MethodError on any
v3 resolve. Gate on pkgversion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Use Base.identity for the linear activation

linear(x) = x duplicated identity; the codebase already used identity
elsewhere for the same purpose.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Validate input dims against the network's actual maximum stride

assertdimconform required width/height divisible by the FIRST CONV'S
FILTER COUNT -- an output-channel number with no relation to spatial
downsampling. It worked for the classic models only because their first
conv happens to have 32 (or 16) filters, and it wrongly rejected valid
sizes (yolov7x at 416, first conv filters=40).

Compute the real constraint instead: walk the cfg blocks tracking each
layer's cumulative downsample (conv/maxpool multiply by stride, upsample
divides, reorg multiplies, routes adopt the referenced layer's scale)
and require divisibility by the maximum. Verified to give 32 for all
classic models and 64 for v4_p6, matching their head strides; v7x now
constructs at 416 and invalid sizes still throw.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Extract shared draw_boxes geometry helper

Both draw_boxes! variants recomputed the identical image/model ratio
and coordinate-index mapping; factor it into _box_geometry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Add a precompile workload

PrecompileTools was a dependency with no workload. Use the (recently
fixed) dummy-weight path to compile cfg parsing, model construction and
the full inference + NMS pipeline at precompile time: time-to-first-
inference in a fresh session drops from ~12.8s to ~0.7s on an M2 Pro
(package load ~3.8s warm).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Move benchmark() behind a BenchmarkTools/PrettyTables extension

BenchmarkTools and PrettyTables were hard dependencies used only by the
benchmark() utility, taxing load time for every detection-only user.
They are now weakdeps triggering a BenchmarkExt extension; calling
benchmark() without them loaded raises a MethodError with a hint
explaining what to load.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Narrow Flux compat to tested versions

0.12/0.13 predate the extension mechanism this package relies on and
have never been exercised by CI; advertising them is risk without
evidence.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Fail on missing detection references instead of self-blessing

get!(RES_REFS, key, computed) meant a new model/image combination
passed trivially on its first run without comparing anything. Missing
keys are now test failures, with the regeneration script referenced.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Commit the test-reference generation script

The script that produced resrefs.jl and the reference images lived
outside the repo; anyone needing to regenerate references (new model,
intentional output change) had to reconstruct it. It regenerates any
subset of models, asserts darknet parity for every model/image
combination before blessing, and uses the same reduced test sizes as
the suite. Round-trip verified on v3_tiny (max ref drift 9e-7, within
the suite's 0.05 tolerance).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Add changelog entries for quality pass

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Document remaining review findings in dev/REVIEW_BACKLOG.md

Everything raised across the bug/performance/model-coverage/quality
review series that was not fixed in #131/#132/#137/#138, with effort
estimates and the explicitly-rejected items recorded so the reasoning
is not lost.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@IanButterworth IanButterworth mentioned this pull request Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant