From 5eb20f3cdb226f17a06138efadc38715e12f6968 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 12 Oct 2026 22:19:17 +0200 Subject: [PATCH] perf: editing a math graph recomputes that graph, not every channel `set_math_channels()` rebuilt the store for any change, and a rebuild replaces the buffer: every channel in the window started again, which is the "modifying one expression invalidates all the others" report. The plan is a *partition over the channels*, so it only moves when the set moves. When the set is unchanged -- same channels, names and enables -- nothing is rebuilt now: the store is untouched, every other channel keeps its samples and its runtimes, and the surgical path prepares and recomputes only the affected graphs. The affected set is the edited channels plus whatever *names* them, because one channel's output is another's input, and the recompute computes the whole list in order (the inputs have to be there) while writing only those in the mask -- which is why the step-loop is now `math_apply_locked(sample, write_mask, timed)`. Adding, removing or renaming a channel still moves the plan, and still rebuilds. [math-probe] graph edit: stored_slots 800 -> 900 (want the second at least the first, and non-zero) The count grows because the stream keeps filling while the edit runs; the point is that it never falls to 0, and that every channel afterwards still reads `N samples vs M time slots` with N == M. --- docs/claude/invariants-math-channels.md | 11 +++ docs/claude/probe-readings.md | 11 +++ src/model.cpp | 119 +++++++++++++++++++++++- src/model.hpp | 9 ++ src/ui_probe.cpp | 8 ++ 5 files changed, 154 insertions(+), 4 deletions(-) diff --git a/docs/claude/invariants-math-channels.md b/docs/claude/invariants-math-channels.md index 58c8d1b..36510ae 100644 --- a/docs/claude/invariants-math-channels.md +++ b/docs/claude/invariants-math-channels.md @@ -280,3 +280,14 @@ filter's history remains the last *real* samples it saw, and the trace resumes a with at most the transient a re-start costs. The exit dump's `math '': finite=N/M` line is the assertion: `finite` must be about `M` minus the hole share, and `finite=0` beside non-zero `hole_slots` is this defect returned. + +- **A graph *edit* does not move the plan; adding or removing a channel does.** The plan is a partition over + the channels, so `set_math_channels()` takes a surgical path whenever the *set* is unchanged -- same channels, + names and enables: the store is never touched, every other channel keeps its samples **and** its runtimes, and + only the edited graphs are prepared again. `math_apply_locked()` computes the whole list on a recompute + (the graphs are walked in order, so a channel's inputs must be there) but writes only the channels in the + affected mask; the affected set is the edited channels plus whatever *names* them, since one channel's output + is another's input. Before this, every edit rebuilt the store and every channel in the window started again, + which is the "modifying one expression invalidates all the others" report. + `[math-probe] graph edit: stored_slots N -> M` reads it: M must be at least N -- the store keeps filling + while the edit runs -- and never 0. diff --git a/docs/claude/probe-readings.md b/docs/claude/probe-readings.md index c7158c4..4bda204 100644 --- a/docs/claude/probe-readings.md +++ b/docs/claude/probe-readings.md @@ -1255,3 +1255,14 @@ refusal it may still hold the previous text. switches one off **through its own acquire tick in the signal list** -- the tick is a box inside the row, which is the shape `signal_tick_visible()` walks -- and reads the plots back. `traces_left=0` is the rule: a disabled signal leaves every plot rather than staying as a slot that draws nothing, and the notice is what says so. + +## `[math-probe] graph edit:` + +``` +[math-probe] graph edit: stored_slots 800 -> 900 (want the second at least the first, and non-zero) +``` + +`UDPSCOPE_PROBE_MATH_MS` applies an edit to one graph through the editor. The plan is a partition over the +channels and an edit does not move it, so the store survives: the count *grows* (the stream keeps filling +while the edit runs) and never falls to 0, which is what a rebuild would do. Every channel then still reads +`N samples vs M time slots` with `N == M`, so no channel was left behind either. diff --git a/src/model.cpp b/src/model.cpp index 6b1eb87..721abed 100644 --- a/src/model.cpp +++ b/src/model.cpp @@ -262,6 +262,60 @@ void ScopeModel::set_math_channels(std::vector channels) { // session start, where a session with no graphs hands over an empty list against a model that has // none either. Comparing the graphs is what makes that call free. if (channels == math_) return; + // An edit of an existing graph -- the same channels, names and enables -- does not move the *plan*, which + // is a partition over the channels. Nothing has to be rebuilt, and every other channel keeps its history: + // only the edited graphs, and the channels that read them, are recomputed. + bool same_set = channels.size() == math_.size(); + for (std::size_t i = 0; same_set && i < channels.size(); ++i) { + same_set = channels[i].name == math_[i].name && channels[i].enabled == math_[i].enabled; + } + if (same_set) { + const auto math_index_of = [this](const std::string& name) -> int { + for (std::size_t k = 0; k < math_.size(); ++k) { + if (math_[k].name == name) return static_cast(k); + } + return -1; + }; + std::vector affected(math_.size(), false); + std::vector edited(math_.size(), false); + for (std::size_t i = 0; i < channels.size(); ++i) { + if (!(channels[i] == math_[i])) { + affected[i] = true; + edited[i] = true; + } + } + // A channel's output is another's *input*, so whatever names a changed channel is recomputed too -- and + // nothing else is touched at all. + for (bool moved = true; moved;) { + moved = false; + for (std::size_t i = 0; i < channels.size(); ++i) { + if (!affected[i]) continue; + for (const MathNode& node : channels[i].nodes) { + if (node.op != MathOp::Source) continue; + const int k = math_index_of(node.source); + if (k >= 0 && !affected[static_cast(k)]) { + affected[static_cast(k)] = true; + moved = true; + } + } + } + } + math_ = std::move(channels); + const auto index_of = [this](const std::string& name) -> int { + for (std::size_t c = 0; c < channels_.size(); ++c) { + if (channels_[c].name == name) return static_cast(c); + } + return -1; + }; + // Every affected channel is prepared again: a runtime carried over from the old graph is history for a + // filter nobody asked for, and one whose *inputs* changed is history for inputs it no longer has. + for (std::size_t i = 0; i < math_.size(); ++i) { + if (affected[i]) math_prepare(math_[i], math_rt_[i], part_.slot_dt(), index_of); + } + recompute_math_locked(affected); + generation_.fetch_add(1, std::memory_order_release); + return; + } math_ = std::move(channels); // Lift the previous synthetic entries out of the signal list; `append_math_signals_locked()` // puts the current ones back, so an edit is the same kind of structural rebuild as a CONFIG. @@ -325,6 +379,12 @@ std::uint64_t ScopeModel::stored_slots() { } void ScopeModel::push_math_sample(const std::vector& sample) { + // The producer's entry: every derived channel, and timed (see the cost sampling below). + math_apply_locked(sample, nullptr, true); +} + +void ScopeModel::math_apply_locked(const std::vector& sample, const std::vector* write_mask, + bool timed) { // The common case is no derived channels at all, and this runs once per *stored slot* -- so the // early-out is what keeps a stream with no math from paying for the feature. if (math_.empty()) return; @@ -333,9 +393,14 @@ void ScopeModel::push_math_sample(const std::vector& sample) { // slot is itself able to drop packets, and timing one slot at a time charges it a whole clock // read. A run amortises both: one pair of reads per 64 measured slots, one pair per 65536 slots // overall, and the figure converges over the millions this counter sees. - const std::uint64_t slot = math_slot_count_++; - math_slots_.store(math_slot_count_, std::memory_order_relaxed); - const bool timed = slot % kMathCostSample == 0; + // The sampling counts *producer* slots: a recompute must not inflate `math_slots_`, which the dump + // reports as the run's own cadence. + std::uint64_t slot = 0; + if (timed) { + slot = math_slot_count_++; + math_slots_.store(math_slot_count_, std::memory_order_relaxed); + timed = slot % kMathCostSample == 0; + } if (timed) math_cost_t0_ = now_ns(); // Values for this slot, extended as the derived ones are computed: a graph may name another math // channel, and nodes are evaluated in list order, so by the time one is asked for it is there. @@ -348,7 +413,11 @@ void ScopeModel::push_math_sample(const std::vector& sample) { if (channel < 0 || m >= math_rt_.size()) continue; const double out = math_step(math_[m], math_rt_[m], math_values_.data(), math_values_.size()); - live().values(static_cast(channel)).push(static_cast(out)); + // A recompute computes every channel -- the graphs are a list walked in order, so a channel's inputs + // have to be there -- but only the affected ones take the sample. + if (write_mask == nullptr || (*write_mask)[m]) { + live().values(static_cast(channel)).push(static_cast(out)); + } if (static_cast(channel) < math_values_.size()) { math_values_[static_cast(channel)] = static_cast(out); } @@ -361,6 +430,48 @@ void ScopeModel::push_math_sample(const std::vector& sample) { } } +void ScopeModel::recompute_math_locked(const std::vector& affected) { + if (!store_ || math_.empty()) return; + BufferStore& store = *store_; + const std::size_t slots = static_cast(store.time().size()); + if (slots == 0) return; + + std::vector derived(channels_.size(), false); + for (std::size_t m = 0; m < math_.size(); ++m) { + const int channel = m < math_channel_.size() ? math_channel_[m] : -1; + if (channel < 0) continue; + derived[static_cast(channel)] = true; + // Only the affected channels start again. The rest keep their samples *and* their runtimes: their graphs + // are untouched, so the state they hold is the answer to the inputs they have always had. + if (affected[m]) store.values(static_cast(channel)).clear(); + } + + // One bulk copy per channel per chunk, through the ring's own reader. A derived channel is fed a zero: + // `math_apply_locked()` overwrites its value as it walks the list in order, and reading its half-rebuilt + // ring here would read out of range. + constexpr std::size_t kChunk = 1u << 14; + const std::size_t base = static_cast(store.time().base()); + std::vector> inputs(channels_.size()); + for (std::size_t c = 0; c < channels_.size(); ++c) { + if (!derived[c]) inputs[c].resize(kChunk); + } + std::vector slot(channels_.size(), 0.0f); + for (std::size_t start = 0; start < slots; start += kChunk) { + const std::size_t n = std::min(kChunk, slots - start); + for (std::size_t c = 0; c < channels_.size(); ++c) { + if (derived[c]) continue; + store.values(c).copy_from(base + start, n, inputs[c].data()); + } + for (std::size_t i = 0; i < n; ++i) { + for (std::size_t c = 0; c < channels_.size(); ++c) { + slot[c] = derived[c] ? 0.0f : inputs[c][i]; + } + math_apply_locked(slot, &affected, /*timed=*/false); + } + } + // The pyramid is the consumer's and the tick publishes it; the samples are written here. +} + void ScopeModel::rebuild_channels() { channels_.clear(); // Which channel each math channel ended up with, by name: a disabled one has none, and the diff --git a/src/model.hpp b/src/model.hpp index 2437880..48b4d19 100644 --- a/src/model.hpp +++ b/src/model.hpp @@ -281,6 +281,15 @@ class ScopeModel { /// evaluated from those, in order, so a math channel may use another one. void push_math_sample(const std::vector& sample); + /// Applies the graphs to one stored slot. `write_mask`, when given, names the derived channels whose + /// rings may be written -- `nullptr` writes all of them, which is what the producer does; a recompute + /// passes the channels a graph change actually affects, so the others keep the samples they have. + void math_apply_locked(const std::vector& sample, const std::vector* write_mask, bool timed); + + /// Recomputes the derived channels in `affected` from the samples the store already holds: their rings are + /// rebuilt from scratch, and the channels a chart change does *not* touch are left exactly as they were. + void recompute_math_locked(const std::vector& affected); + /// How many slots the store currently holds. A file view reads it to tell a structural change that /// dropped the buffer (0) from one that could not have (see `reload_file_view()` in ui.cpp). std::uint64_t stored_slots(); diff --git a/src/ui_probe.cpp b/src/ui_probe.cpp index c0df2e7..c7a9e57 100644 --- a/src/ui_probe.cpp +++ b/src/ui_probe.cpp @@ -424,7 +424,15 @@ gboolean math_probe(gpointer user) { ed->name != nullptr ? gtk_editable_get_text(GTK_EDITABLE(ed->name)) : "-"; const std::string unit_read = ed->unit != nullptr ? gtk_editable_get_text(GTK_EDITABLE(ed->unit)) : "-"; + // The edit must not cost the buffer. The plan is a partition over the channels and an edit does not + // move it, so every channel keeps its history and only the edited graph -- and whatever reads it -- is + // recomputed. Before this, `set_math_channels()` rebuilt the store and every channel started again. + const std::uint64_t slots_before = a->model->stored_slots(); on_math_apply(nullptr, ed); // frees the dialog: nothing below may touch `ed` + const std::uint64_t slots_after = a->model->stored_slots(); + std::fprintf(stderr, "[math-probe] graph edit: stored_slots %llu -> %llu (want the second at least the first, and non-zero)\n", + static_cast(slots_before), + static_cast(slots_after)); const std::size_t saved = a->math.empty() ? 0u : a->math[static_cast(index)].nodes.size();