udpscope/docs/review-2026-10-13.md

# Project review — 2026-10-13

A status review for performance, defects and gaps. Every claim below was read back against the
current tree; where a candidate finding turned out to be wrong it is listed as dismissed, with the
reason, rather than dropped — a false alarm that is not written down gets re-raised.

## Summary

The project is in good shape. All ten `todo.md` items are implemented, every deviation from
`SPECS.md` is documented in README's *Review findings not (yet) implemented* and *Limitations*, there
is no `TODO`/`FIXME`/`HACK` anywhere in `src/`, no dead command-line option, and no settings field
that is written but never read. The tree builds clean.

- **Defects: none confirmed.** Four candidates were investigated and all four are correct as
  written (§2).
- **Performance: one real win on the ingest hot path** (§1.1) — a 64-bit `div` per sample per
  channel that the code's own comment believes is already cheap. Then one real win on the idle
  frame path (§1.2), and three small ones. **§1.1 and §1.2 have since been implemented**; the three
  small ones (§1.3-§1.5) have not.
- **Gaps: no missing requirement.** What is missing is only what was never asked for (§4), plus one
  stale sentence in README (§3).

## 1. Performance

### 1.1 A 64-bit division per sample per channel on the producer hot path

> **Implemented** (2026-10-13), as the fix below describes: `Acquisition::acc_mask_` holds the
> per-channel masks, built where the accumulators are sized; `reserve_hole()` moved with it; the
> misleading comment at `src/model.cpp:120-122` now says what the mask actually buys. Written up in
> README under *What the ingest filter and a still frame cost*.

`src/acquisition.cpp:856-857`, in the innermost loop of `on_data()`:

```cpp
const u64 period = g * part.stride[i];
if (tick % period != period - 1) continue;
```

Three facts make this the most expensive avoidable thing in the application:

1. **`part.stride[i]` is always 1.** `plan_partition()` assigns it (`src/model.cpp:49`) and then
   explicitly re-asserts it (`src/model.cpp:70`), with the comment above it explaining why
   per-channel strides were abandoned. So `period == g` for every channel, always — the load and
   the multiply are provably redundant.
2. **`tick % period` is a real `div`.** `g` is a runtime `u64`, so the compiler cannot strength-
   reduce it, whatever its value. A 64-bit integer division is ~20-40 cycles of latency on x86.
   The intent was otherwise: `src/model.cpp:120-122` says a power-of-two stride "keeps `tick %
   period` cheap", which would be true of a compile-time constant and is not true of a variable.
   **`time_stride` being a power of two currently buys nothing here.**
3. **The same condition is already computed.** `tick % g == g - 1` is evaluated at
   `src/acquisition.cpp:811` for the time ring, and again at `:881` for the math push — and
   `:857` is the same test once more per channel.

Cost model: `(n_channels + 2)` divisions per **raw** sample, i.e. per wire sample, not per stored
one — the decimation filter deliberately reads every sample (`:850-852`). At the fast mock's
1.3 MS/s with 4 channels that is ~7.8 M divisions a second on the producer thread, which is the
thread whose budget overrun costs packets rather than frames.

Fix: compute the group boundary **once per sample** and reuse it. `g` is a power of two, so
`const u64 mask = g - 1;` hoisted out of the packet loop turns all three sites into
`(tick & mask) == mask`. To keep the per-channel generality the type still carries, precompute a
per-channel mask vector when the partition is planned rather than per sample.

The same expression appears in the hole path at `src/acquisition.cpp:966` (inside
`reserve_hole()`'s double loop). That one is cosmetic — it runs only on packet loss — but it should
move with its twin so the two paths keep matching.

How to verify: the fast mock (`tools/mock_udpstreamer.py --rate 1300000 --batch 2000`), reading the
exit dump's `gaps=` and the producer's CPU share before and after. Expect the win to show as
headroom rather than as a changed reading on a stream the producer already keeps up with.

### 1.2 The figure is re-rendered 60 times a second even when the picture cannot change

> **Implemented** (2026-10-13), conservatively: a signature of what a frozen view can be re-framed
> by, opening the gate for `kSettleRenders` frames, plus a `kStaticRenderEvery` floor that renders
> anyway every four ticks, and the layout editor excluded outright. `[pause-probe]`'s new `skips=`
> field reports it: a measured run read 0 live, 33 paused, with `columns == graded`, `render blank
> frames: 0` and the layout floor still converged.

`src/ui.cpp:3709` queues a GL render unconditionally at the end of every tick, and the tick is a
fixed 16 ms timeout (`src/ui_app.hpp:1023`). In `Held` and `Paused` the bound data is an immutable
snapshot and the axis is pinned, so with no pointer interaction the frame is provably identical to
the last one — and it is still fully re-rendered. On llvmpipe, or on a laptop looking at a latched
event, that is the whole GPU cost of the application for nothing.

Fix: queue the render only when something that can change the picture did — new samples acquired,
a mode/epoch/pin change, a mark/cursor/label move, an interaction, a resize. Keep it conservative:
render unconditionally while the layout is still settling, since the layout editor's buttons are
placed from the *previous* frame's rects and several probes read a value one frame after the ask.

How to verify: the exit dump's per-plot painted-frame counts and `layout over N frames` are exactly
the two readings that would catch an over-eager skip.

### 1.3 The event publish blocks the UI tick for 69-115 ms

Already documented, with measurements, at `README.md:2189-2191` — listed here only because the
README prices exactly one remedy (a second live ring) and declines it for good reasons. Two
options it does not price, both cheaper than a second budget's worth of memory:

- do the snapshot copy on the **producer** side of the handoff, so the stall lands where a stall is
  already tolerated at a latch (the pause publish already blocks the producer, 22-86 ms);
- chunk the copy across frames behind a sequence number, publishing only when complete.

Worth a design note before any code — the README's own A/B (`gaps=0` either way) says this buys a
smoother frame at a latch, not recovered packets, so it is a polish item, not a correctness one.

### 1.4 The status detail string is rebuilt and re-applied every frame

`src/ui.cpp:4063` calls `format_detail(snap)` every tick and `:4082` pushes the result into
`gtk_widget_set_tooltip_text()` every tick. The popover is already guarded by its own visibility
(`:4083-4087`); the tooltip is not, and the string is long enough that the code tracks its maximum
length. Nothing reads it unless the pointer rests on the status label.

Fix: keep the last applied detail string and skip both the format and the setter when it is
unchanged. Low risk, no behaviour change.

### 1.5 Search marks are re-filtered and re-sorted every frame

`src/ui.cpp:1760-1768`. Two heap vectors, a pass over every hit and an `std::sort` per frame for as
long as a search result set exists. Correctly short-circuited when there are no hits
(`:1753`), and being recomputed every frame is load-bearing for a *live* view (the marks are stored
in stream seconds and the window rolls under them) — but in `Held`/`Paused` neither the hits nor
the view range move, so the whole result can be cached and invalidated on a view-range or hit-set
change.

### 1.6 Noted, not recommended: the model mutex spans all of `on_data()`

`src/acquisition.cpp:558` takes the lock and holds it to the end of the function (~line 913),
covering the sample loop, the math evaluation, the trigger engine, and hole reserve/backfill. The
UI tick takes the same mutex (`src/ui.cpp:3285`). This is the documented design ("taken once per
DATA frame", `docs/claude/architecture.md:15-16`) and it is the right call at today's rates, but it
is the coupling by which math-channel cost on the producer turns into UI frame jitter. The existing
`math_timed` per-slot instrumentation is the thing to watch if math graphs get heavier.

## 2. Defect candidates investigated and dismissed

Recorded so they are not re-raised.

- **`capture_edge()` loading `memory_order_relaxed`** (`src/model.hpp:492`) while its partner
  `trigger_pending()` carries release/acquire. **Correct as written.** The consumer reaches
  `capture_edge()` at `src/ui.cpp:3275` only *after* `display_mode()` at `:3273` has done the
  acquire load of `trigger_pending_`; the acquire establishes the happens-before, so a relaxed load
  afterwards cannot see the stale edge, and an acquire also forbids the later load being reordered
  ahead of it. The probe path (`src/ui_probe.cpp:1129-1130`) reads the flag first too.
  *One fragility worth a line of doc:* the correctness is a property of **read order at the call
  site**, and `src/model.hpp:489-491` says a consumer that sees the flag sees the edge without
  saying that the consumer must therefore read the flag first. Tightening that comment is the whole
  of the work.
- **`math_values_.data()` passed to `math_step()` across a possible reallocation**
  (`src/model.cpp:409-422`). **Safe.** `push_math_sample()` is only reached from `on_data()`
  (`src/acquisition.cpp:881`, `:978`), which holds the model mutex for its whole body, and
  `set_math_channels()` takes the same mutex.
- **`model_.signals()` indexed by `channels[0].signal` after the lock was released**
  (`src/acquisition.cpp:593-595`). **Safe** — same reason: the lock is held from `:558` to the end
  of the function, so no CONFIG can re-shape the signal list underneath it.
- **`events_thumb` left dangling across a list rebuild** (`src/ui_events.cpp:632`). **Already
  handled** — nulled before the rows are removed and rebound after, with the old texture unreffed.

Nothing in the trigger/pause/file-view/replay seams, the math-channel dependency handling, the
measurement and cursor paths, or the export/session writers reproduced a wrong number or a lost
action on inspection. That is a statement about a code read, not about a test run: README's own
`Limitations` is right that these paths are checked headless, and the seams between two *user*
actions are where the probes are thinnest.

## 3. One documentation defect

`README.md:2304-2307` says the interactive paths — "drag and drop, the dialogs, the right-click
menu" — "were not driven by an automated test". That is now stale in all three particulars:
`UDPSCOPE_PROBE_DROP_MS` drives the drop handler, `UDPSCOPE_PROBE_ROWMENU_MS` walks the
right-click menu and clicks through its submenu, and `UDPSCOPE_PROBE_DEFAULTS_MS`,
`UDPSCOPE_PROBE_CONFIGS_MS`, `UDPSCOPE_PROBE_PREFS_MS` and `UDPSCOPE_PROBE_HELP_MS` all open and
read back dialogs. What remains genuinely undriven is the *gesture* layer (a real pointer drag, a
real popover dismissal) rather than the handlers behind it, which is a much narrower claim and the
one worth making.

## 4. Gaps

### Requirements: complete

`SPECS.md` was walked bullet by bullet. Everything is implemented, and the five places where the
implementation differs are each documented with a reason at `README.md:2182-2240`: one live buffer
plus an immutable `EventSnapshot` rather than two live rings; allocation at the first CONFIG rather
than at startup; no per-channel decimation (the spec's formula aliased a 2 kHz sine on a 1 MHz clock
to a flat line); pre-trigger % as a share of the event window; and a capture drawn as it fills.
`todo.md`'s ten items are all really there, including item 9's full preference list — the
light/dark/auto theme, the reset-to-defaults and the cache controls are all in `src/settings.hpp`
and `src/ui_prefs.cpp`.

### Not asked for, worth considering

Ranked by value against cost, not by ambition:

1. **Save the current view as an image.** The cheapest of these by a wide margin: the event cache
   already has a sample-driven image writer (`write_preview()`, `src/event_store.cpp:266`), so a
   menu item and a file chooser is most of the work. A scope user screenshots constantly.
2. **A draggable trigger level on the plot.** The trigger marker exists but is a vertical cursor
   and deliberately non-selectable (`src/plot_host.cpp:250`); the level is reachable only through
   the panel's spin button. A horizontal handle on the trigger channel's row is the standard
   gesture on every bench scope.
3. **Per-trace vertical offset and scale.** Up to eight signals share one row's y axis, so two
   signals of different magnitudes cannot both be read. This is the gap a user hits soonest.
4. **An FFT / spectrum row.** The largest of these, and the one a UDPStreamer user is most likely
   to ask for next; it fits the figure's row model (one row, one derived series) rather than
   fighting it.
5. **XY mode**, and **undo for layout edits** — both real scope features, both a poor fit for the
   current one-x-window figure and the layout file respectively. Mentioned for completeness.

## 5. Suggested order

1. §1.1, the ingest-path division — small, local, the clearest win, and it corrects a comment that
   is currently misleading about why the power of two matters.
2. §3, the stale README sentence — minutes, and it is wrong today.
3. §1.4 and §1.5, the per-frame string and sort — small and safe.
4. §2's doc tightening on `capture_edge()`.
5. §1.2, the conditional render — the bigger win of the two performance items, but the one that
   needs care against the layout-settling probes, so it deserves its own change.
6. §4.1, the screenshot — the cheapest real feature on the list.