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():

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.