perf: a file view re-reads the file only when a structural change really dropped it
Three tightenings, all measured with the reloads now reported on stderr.
`set_math_channels()` treats an identical list as no change at all. It rebuilt
unconditionally, and a rebuild replaces the buffer and drops its history -- which a file
view then has to re-read from disk. The comparison is the graphs', and it leaves the nodes'
`x`/`y` out deliberately: those are the canvas's own, no op reads them, and a graph that
merely *moved* should not cost the buffer.
`reload_file_view()` fires only when the store is actually empty. A change that really
happened leaves it so in a file view (the rebuild replaces it, and there is no producer to
refill it); one that found nothing to do leaves it full, and reading a 200 MB file again for
that is the waste. `ScopeModel::stored_slots()` is the one-line accessor it needed.
Every reload now says so, because a second read of a file is worth accounting for:
[udpscope] reloaded <path> after a structural change: 402 samples back
Measured, `--open` a file:
session with no graphs 0 reloads
the math probe (3 real changes) 3 reloads, and all the readings stay equal
(`math x2 402 samples vs 402 time slots`)
I had offered this as "one extra read at `--open` startup", and that was wrong: with no
session there is no `apply_math_channels()` call at all, so there was never an extra read
to remove. What the tightenings buy is that a no-op application is now free, and that a
reload cannot happen unless the buffer was really lost. Each *genuine* change still costs
one re-read -- a rebuild drops the history by design, and preserving it across a rebuild is
a model change rather than this one.
This commit is contained in:
1 parent
b090cc2a21
commit
47d49e75ff
5 files changed
+45
-1
No files matched your search
@@ -42,7 +42,13 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
|||||||
channel to an open waveform therefore emptied *every* channel -- `samples=0` on the sources too, which a user
|
channel to an open waveform therefore emptied *every* channel -- `samples=0` on the sources too, which a user
|
||||||
reads as "the plot is broken" -- and the same emptiness follows any structural change in a file view.
|
reads as "the plot is broken" -- and the same emptiness follows any structural change in a file view.
|
||||||
`reload_file_view()` (in `ui.cpp`) re-reads the file after such a change, which is also what computes the
|
`reload_file_view()` (in `ui.cpp`) re-reads the file after such a change, which is also what computes the
|
||||||
derived channels over it: the load's own pushes run the math once the set is in place. It is why `--open`
|
derived channels over it: the load's own pushes run the math once the set is in place. It fires only when the
|
||||||
|
buffer was *really* dropped -- `stored_slots() > 0` means the change found nothing to do -- and it says so
|
||||||
|
on stderr (`reloaded <path> after a structural change`), because a second read of a file is worth
|
||||||
|
accounting for. `set_math_channels()` in turn treats an identical list as no change at all, so a no-op
|
||||||
|
application (the session apply, a re-open) neither rebuilds nor drops nor re-reads; the comparison leaves the
|
||||||
|
nodes' `x`/`y` out on purpose, since those are the canvas's own and no op reads them -- a graph that only
|
||||||
|
*moved* is not a change, and rebuilding for it would drop the buffer to store two numbers. It is why `--open`
|
||||||
works at all with a session that defines math channels, since `main()` loads the file *before* the UI (and
|
works at all with a session that defines math channels, since `main()` loads the file *before* the UI (and
|
||||||
so before the session's graphs) exists.
|
so before the session's graphs) exists.
|
||||||
- **Every path that fills a store must skip a derived channel, not just the packet path.**
|
- **Every path that fills a store must skip a derived channel, not just the packet path.**
|
||||||
|
|||||||
@@ -58,6 +58,17 @@ struct MathNode {
|
|||||||
double hi = 1.0;
|
double hi = 1.0;
|
||||||
double x = 0.0; ///< the editor's box position, logical pixels.
|
double x = 0.0; ///< the editor's box position, logical pixels.
|
||||||
double y = 0.0;
|
double y = 0.0;
|
||||||
|
|
||||||
|
/// Whether two nodes are the same node for the *engine's* purposes.
|
||||||
|
///
|
||||||
|
/// `x`/`y` are the canvas's own and no op reads them, so a graph that only moved is not a change:
|
||||||
|
/// rebuilding for it replaces the buffer and drops its history -- and in a file view that means
|
||||||
|
/// re-reading the file -- to store two numbers nothing but the editor draws with.
|
||||||
|
bool operator==(const MathNode& other) const {
|
||||||
|
return op == other.op && source == other.source && in_a == other.in_a && in_b == other.in_b &&
|
||||||
|
gain == other.gain && offset == other.offset && fc == other.fc && q == other.q &&
|
||||||
|
lo == other.lo && hi == other.hi;
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
/// A derived channel: a graph of nodes and which node is the answer.
|
/// A derived channel: a graph of nodes and which node is the answer.
|
||||||
@@ -67,6 +78,11 @@ struct MathChannel {
|
|||||||
bool enabled = false;
|
bool enabled = false;
|
||||||
std::vector<MathNode> nodes;
|
std::vector<MathNode> nodes;
|
||||||
int output = -1; ///< the node whose value is the channel's.
|
int output = -1; ///< the node whose value is the channel's.
|
||||||
|
|
||||||
|
bool operator==(const MathChannel& other) const {
|
||||||
|
return name == other.name && unit == other.unit && enabled == other.enabled &&
|
||||||
|
output == other.output && nodes == other.nodes;
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
/// The per-channel runtime: the filter memory and the per-node scratch. Kept apart from the
|
/// The per-channel runtime: the filter memory and the per-node scratch. Kept apart from the
|
||||||
|
|||||||
@@ -257,6 +257,11 @@ void ScopeModel::append_math_signals_locked() {
|
|||||||
|
|
||||||
void ScopeModel::set_math_channels(std::vector<MathChannel> channels) {
|
void ScopeModel::set_math_channels(std::vector<MathChannel> channels) {
|
||||||
std::lock_guard<std::mutex> lock(mutex_);
|
std::lock_guard<std::mutex> lock(mutex_);
|
||||||
|
// An identical list is not a change. The rebuild below replaces the buffer and drops its history --
|
||||||
|
// which a file view then has to re-read from disk -- and `apply_math_channels()` runs this on every
|
||||||
|
// 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;
|
||||||
math_ = std::move(channels);
|
math_ = std::move(channels);
|
||||||
// Lift the previous synthetic entries out of the signal list; `append_math_signals_locked()`
|
// 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.
|
// puts the current ones back, so an edit is the same kind of structural rebuild as a CONFIG.
|
||||||
@@ -314,6 +319,11 @@ void ScopeModel::push_loaded_slot(const std::vector<f32>& values, f64 t, f64 abs
|
|||||||
push_math_sample(slot);
|
push_math_sample(slot);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
std::uint64_t ScopeModel::stored_slots() {
|
||||||
|
std::lock_guard<std::mutex> lock(mutex_);
|
||||||
|
return store_ ? store_->time().size() : 0;
|
||||||
|
}
|
||||||
|
|
||||||
void ScopeModel::push_math_sample(const std::vector<f32>& sample) {
|
void ScopeModel::push_math_sample(const std::vector<f32>& sample) {
|
||||||
// The common case is no derived channels at all, and this runs once per *stored slot* -- so the
|
// 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.
|
// early-out is what keeps a stream with no math from paying for the feature.
|
||||||
|
|||||||
@@ -281,6 +281,10 @@ class ScopeModel {
|
|||||||
/// evaluated from those, in order, so a math channel may use another one.
|
/// evaluated from those, in order, so a math channel may use another one.
|
||||||
void push_math_sample(const std::vector<f32>& sample);
|
void push_math_sample(const std::vector<f32>& sample);
|
||||||
|
|
||||||
|
/// 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();
|
||||||
|
|
||||||
/// Pushes one stored slot of a **file** load -- the values, the shared clock and the absolute
|
/// Pushes one stored slot of a **file** load -- the values, the shared clock and the absolute
|
||||||
/// clock -- and then the derived channels, in the producer's own order and under the model's own
|
/// clock -- and then the derived channels, in the producer's own order and under the model's own
|
||||||
/// lock. The loader cannot simply hold the lock and push into `live()` itself: every setter it
|
/// lock. The loader cannot simply hold the lock and push into `live()` itself: every setter it
|
||||||
|
|||||||
@@ -4190,6 +4190,10 @@ gboolean on_app_key_pressed(GtkEventControllerKey*, guint keyval, guint, GdkModi
|
|||||||
/// math once the set is in place.
|
/// math once the set is in place.
|
||||||
bool reload_file_view(App& app) {
|
bool reload_file_view(App& app) {
|
||||||
if (!app.replaying && app.model->config().open_waveform.empty()) return false;
|
if (!app.replaying && app.model->config().open_waveform.empty()) return false;
|
||||||
|
// Nothing to restore: a change that really happened leaves the store empty in a file view -- the
|
||||||
|
// rebuild replaces it and there is no producer to refill it -- while one that found nothing to do
|
||||||
|
// leaves it full. Reading the file again for the second kind is the waste this avoids.
|
||||||
|
if (app.model->stored_slots() > 0) return false;
|
||||||
const std::string path =
|
const std::string path =
|
||||||
!app.waveform_name.empty() ? app.waveform_name : app.model->config().open_waveform;
|
!app.waveform_name.empty() ? app.waveform_name : app.model->config().open_waveform;
|
||||||
if (path.empty()) return false;
|
if (path.empty()) return false;
|
||||||
@@ -4199,6 +4203,10 @@ bool reload_file_view(App& app) {
|
|||||||
set_notice(app, "cannot reload " + path + " after the change: " + error, 6);
|
set_notice(app, "cannot reload " + path + " after the change: " + error, 6);
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
// Said out loud because it is the one place a file is read twice: a structural change really did
|
||||||
|
// drop the buffer, and the file is the only thing that can put it back.
|
||||||
|
std::fprintf(stderr, "[udpscope] reloaded %s after a structural change: %llu samples back\n",
|
||||||
|
path.c_str(), static_cast<unsigned long long>(report.samples));
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user