diff --git a/docs/review-2026-10-13.md b/docs/review-2026-10-13.md new file mode 100644 index 0000000..5aca7f6 --- /dev/null +++ b/docs/review-2026-10-13.md @@ -0,0 +1,217 @@ +# 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.