From a18678d1cb01de82767ace5a36b40983477960f3 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:00:27 -0700 Subject: [PATCH 1/9] test(projects): a single-fixture sibling of quad-strips, for attributing frame cost MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Frame cost on the desk S3 is flat ~8.4 ms per fixture+output chain; the way to attribute that per-chain cost is to profile one chain against four. The brief's 1-fixture workload was ad hoc, so the oracle could not be re-run. Same shader, fixture, mapping and endpoint as quad-strips channel 1 — the two must stay comparable, which the README says out loud. Plan: 2026-07-31-2225-persist-dataflow-resolution --- projects/test/quad-strips-1fix/README.md | 22 ++++++++++ projects/test/quad-strips-1fix/clock.json | 8 ++++ projects/test/quad-strips-1fix/fixture1.json | 41 +++++++++++++++++++ .../test/quad-strips-1fix/fixture1.map2d.json | 18 ++++++++ projects/test/quad-strips-1fix/output1.json | 19 +++++++++ projects/test/quad-strips-1fix/project.json | 19 +++++++++ projects/test/quad-strips-1fix/shader.glsl | 39 ++++++++++++++++++ projects/test/quad-strips-1fix/shader.json | 21 ++++++++++ 8 files changed, 187 insertions(+) create mode 100644 projects/test/quad-strips-1fix/README.md create mode 100644 projects/test/quad-strips-1fix/clock.json create mode 100644 projects/test/quad-strips-1fix/fixture1.json create mode 100644 projects/test/quad-strips-1fix/fixture1.map2d.json create mode 100644 projects/test/quad-strips-1fix/output1.json create mode 100644 projects/test/quad-strips-1fix/project.json create mode 100644 projects/test/quad-strips-1fix/shader.glsl create mode 100644 projects/test/quad-strips-1fix/shader.json diff --git a/projects/test/quad-strips-1fix/README.md b/projects/test/quad-strips-1fix/README.md new file mode 100644 index 000000000..3283fec8d --- /dev/null +++ b/projects/test/quad-strips-1fix/README.md @@ -0,0 +1,22 @@ +# Quad strips (1 fixture) + +The single-fixture sibling of `../quad-strips`, kept as the **engine +frame-cost oracle**. Same shader, same fixture and mapping, same output +endpoint as channel 1 of the four-channel project — only the other three +fixture+output chains are gone. + +Its reason to exist: frame cost on the desk ESP32-S3 was measured as flat +~8.4 ms *per fixture+output chain*, independent of render resolution and LED +count (see `docs/debt/s3-frame-cost-scales-per-fixture.md`). Profiling one +chain against four is how that per-chain cost is attributed, so both +workloads must stay comparable — **change one, change the other**. + +## Profiling + +```bash +lp-cli profile projects/test/quad-strips-1fix --collect cpu +``` + +Defaults to steady-render mode; the run writes `report.txt` (self/inclusive +cycles, stack high-water) under `profiles/--…/`. Run the +four-fixture project the same way for the scaling comparison. diff --git a/projects/test/quad-strips-1fix/clock.json b/projects/test/quad-strips-1fix/clock.json new file mode 100644 index 000000000..79834c140 --- /dev/null +++ b/projects/test/quad-strips-1fix/clock.json @@ -0,0 +1,8 @@ +{ + "kind": "Clock", + "controls": { + "running": true, + "rate": 1, + "scrub_offset_seconds": 0 + } +} diff --git a/projects/test/quad-strips-1fix/fixture1.json b/projects/test/quad-strips-1fix/fixture1.json new file mode 100644 index 000000000..de464edc5 --- /dev/null +++ b/projects/test/quad-strips-1fix/fixture1.json @@ -0,0 +1,41 @@ +{ + "kind": "Fixture", + "render_size": { + "width": 30, + "height": 4 + }, + "bindings": { + "input": { + "source": "bus:visual.out" + }, + "output": { + "target": "bus:control.out/ch1" + } + }, + "sampling": "direct", + "diagnostic_mode": "off", + "mapping": { + "kind": "Map2d", + "source": "fixture1.map2d.json" + }, + "color_order": "rgb", + "transform": [ + [ + 1, + 0, + 0 + ], + [ + 0, + 1, + 0 + ], + [ + 0, + 0, + 1 + ] + ], + "brightness": 38, + "gamma_correction": false +} diff --git a/projects/test/quad-strips-1fix/fixture1.map2d.json b/projects/test/quad-strips-1fix/fixture1.map2d.json new file mode 100644 index 000000000..abdac29a7 --- /dev/null +++ b/projects/test/quad-strips-1fix/fixture1.map2d.json @@ -0,0 +1,18 @@ +{ + "format": 1, + "sample_diameter": 3.0, + "canvas": [ + 0.0, + 0.0, + 100.0, + 100.0 + ], + "objects": [ + { + "name": "strip1", + "shape": { + "grid": { "origin": [1.6667, 12.5], "cols": 30, "rows": 1, "pitch": 3.3333 } + } + } + ] +} diff --git a/projects/test/quad-strips-1fix/output1.json b/projects/test/quad-strips-1fix/output1.json new file mode 100644 index 000000000..a69a4434d --- /dev/null +++ b/projects/test/quad-strips-1fix/output1.json @@ -0,0 +1,19 @@ +{ + "kind": "Output", + "endpoint": "ws281x:rmt:D10", + "bindings": { + "input": { + "source": "bus:control.out/ch1" + } + }, + "options": { + "white_point": [ + 1, + 1, + 1 + ], + "interpolation_enabled": false, + "dithering_enabled": false, + "lut_enabled": false + } +} diff --git a/projects/test/quad-strips-1fix/project.json b/projects/test/quad-strips-1fix/project.json new file mode 100644 index 000000000..6fd153189 --- /dev/null +++ b/projects/test/quad-strips-1fix/project.json @@ -0,0 +1,19 @@ +{ + "kind": "Project", + "format": 2, + "name": "Quad strips (1 fixture)", + "nodes": { + "clock": { + "ref": "./clock.json" + }, + "shader": { + "ref": "./shader.json" + }, + "fixture1": { + "ref": "./fixture1.json" + }, + "output1": { + "ref": "./output1.json" + } + } +} diff --git a/projects/test/quad-strips-1fix/shader.glsl b/projects/test/quad-strips-1fix/shader.glsl new file mode 100644 index 000000000..86531e31d --- /dev/null +++ b/projects/test/quad-strips-1fix/shader.glsl @@ -0,0 +1,39 @@ +// Quad-strip bring-up pattern: four horizontal bands, one per strip. +// +// Each band has its own base hue (band 1 red, 2 green, 3 blue, 4 amber) so a +// strip wired to the wrong channel — or a channel bleeding into its +// neighbour — is visible at a glance, and each band carries a bright chase +// dot moving at a band-specific speed and phase so per-channel animation +// independence is visible too. A dim base fill keeps every strip identifiably +// lit even where the dot is not. + +layout(binding = 0) uniform vec2 outputSize; +layout(binding = 1) uniform float time; + +vec3 bandColor(float band) { + if (band < 0.5) { + return vec3(1.0, 0.0, 0.0); + } + if (band < 1.5) { + return vec3(0.0, 1.0, 0.0); + } + if (band < 2.5) { + return vec3(0.0, 0.0, 1.0); + } + return vec3(1.0, 0.6, 0.0); +} + +vec4 render(vec2 pos) { + vec2 uv = pos / outputSize; + float band = floor(uv.y * 4.0); + + // Band-specific chase: distinct speed and phase per strip. + float speed = 0.25 + band * 0.1; + float head = fract(time * speed + band * 0.25); + float d = abs(uv.x - head); + d = min(d, 1.0 - d); + float dot_i = smoothstep(0.12, 0.0, d); + + vec3 color = bandColor(band) * (0.12 + 0.88 * dot_i); + return vec4(color, 1.0); +} diff --git a/projects/test/quad-strips-1fix/shader.json b/projects/test/quad-strips-1fix/shader.json new file mode 100644 index 000000000..e07252ce0 --- /dev/null +++ b/projects/test/quad-strips-1fix/shader.json @@ -0,0 +1,21 @@ +{ + "kind": "Shader", + "source": "shader.glsl", + "render_order": 0, + "bindings": { + "output": { + "target": "bus:visual.out" + } + }, + "consumed": { + "time": { + "kind": "value", + "value": "f32", + "default": 0, + "label": "Time", + "description": "Project clock time in seconds", + "default_bind": "bus:time" + } + }, + "float_mode": "fixed" +} From 6e8f90e61a52a7a1b26570886087f61d363b375f Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:14:08 -0700 Subject: [PATCH 2/9] refactor(lpc-engine): name the two reasons resolution goes stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolver had one `clear_frame_cache()` doing two unrelated jobs: "a new frame started, so last frame's values are stale" and "the graph changed shape, so everything I know is suspect". Persisting resolution across frames is a change to what the first one does — impossible to make safely while both callers say the same thing. So: `begin_frame()` and `invalidate_structure()`, classified at every call site, plus counters that make "did this frame re-derive the graph?" a testable question instead of a profiling exercise. Two gaps surfaced while classifying: - `Engine::add_binding` never invalidated at all. Harmless while every frame cleared anyway; a stale-value bug the moment it doesn't. - `clear_bindings` ran on the tree, leaving the resolver's knowledge of a binding graph that no longer exists to be invalidated by whatever happened to run next. It now goes through the engine and invalidates on its own. Behaviour is unchanged: both operations still clear everything. The tests pin each way the graph can change — re-bind, bus priority, node reattach, selector switch (the playlist's mechanism, without a playlist) — so that P3's caching has an oracle that predates it. One of those pins was wrong when first written: it assumed priority arbitrates consumed-slot bindings the way it does bus providers. It does not — consumed slots resolve by owner depth then registration order, and a re-bind replaces the set rather than outranking it. The test now pins what the engine actually does. Plan: 2026-07-31-2225-persist-dataflow-resolution --- .../lpc-engine/src/dataflow/resolver/mod.rs | 2 +- .../src/dataflow/resolver/resolve_session.rs | 51 +++- .../src/dataflow/resolver/resolver.rs | 100 ++++++- lp-core/lpc-engine/src/engine/engine.rs | 32 +- lp-core/lpc-engine/src/engine/mod.rs | 2 + .../lpc-engine/src/engine/project_apply.rs | 9 +- .../engine/resolution_persistence_tests.rs | 275 ++++++++++++++++++ lp-core/lpc-engine/src/engine/test_support.rs | 91 ++++++ 8 files changed, 533 insertions(+), 29 deletions(-) create mode 100644 lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs diff --git a/lp-core/lpc-engine/src/dataflow/resolver/mod.rs b/lp-core/lpc-engine/src/dataflow/resolver/mod.rs index 62faab5df..d6be3b8be 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/mod.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/mod.rs @@ -27,6 +27,6 @@ pub use resolve_session::{EngineSession, ResolveSession}; pub use resolve_trace::{ ResolveLogLevel, ResolveTrace, ResolveTraceError, ResolveTraceEvent, TraceGuard, }; -pub use resolver::Resolver; +pub use resolver::{ResolveFrameCounters, Resolver}; pub use resolver_cache::ResolverCache; pub use tick_resolver::{SessionHostResolver, TickResolver}; diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs index 3b064a6e1..a72e27142 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs @@ -62,10 +62,13 @@ impl<'a> EngineSession<'a> { query: QueryKey, ) -> Result { if let Some(pv) = self.resolver.cache().get(&query) { + let pv = pv.clone(); + self.resolver.counters_mut().cache_hits += 1; self.trace.record_cache_hit(&query); - return Ok(pv.clone()); + return Ok(pv); } + self.resolver.counters_mut().uncached_resolves += 1; self.trace .try_push_active(&query) .map_err(SessionResolveError::from)?; @@ -95,16 +98,33 @@ impl<'a> EngineSession<'a> { QueryKey::ConsumedSlotAccessor { node, accessor } => { self.resolve_consumed_slot(host, *node, accessor.path(), query) } - QueryKey::ProducedSlot { .. } => { - self.trace.record_produce_start(query); - let r = host.produce(query, self); - match &r { - Ok(_) => self.trace.record_produce_end(query), - Err(_) => self.trace.record_resolve_error(query), + QueryKey::ProducedSlot { .. } => self.produce_through_host(host, query), + } + } + + /// Ask the host to produce `query`, counting how the answer was obtained. + /// + /// An authored-def answer is structural work (it re-reads a definition + /// that only a project change can alter); a producer answer is per-frame + /// work that must happen every frame. + fn produce_through_host( + &mut self, + host: &mut H, + query: &QueryKey, + ) -> Result { + self.trace.record_produce_start(query); + let r = host.produce(query, self); + match &r { + Ok(production) => { + match production.source { + ProductionSource::Default => self.resolver.counters_mut().def_produces += 1, + _ => self.resolver.counters_mut().producer_produces += 1, } - r + self.trace.record_produce_end(query); } + Err(_) => self.trace.record_resolve_error(query), } + r } fn resolve_bus( @@ -113,6 +133,7 @@ impl<'a> EngineSession<'a> { channel: &ChannelName, query: &QueryKey, ) -> Result { + self.resolver.counters_mut().binding_lookups += 1; let candidates = host.providers_for_bus(channel); let entry = select_highest_priority_bus_provider(channel, &candidates)?; self.trace.record_select_binding(query, entry.0); @@ -126,6 +147,7 @@ impl<'a> EngineSession<'a> { slot: &SlotPath, query: &QueryKey, ) -> Result { + self.resolver.counters_mut().merge_policy_reads += 1; let policy = host.merge_policy_for_consumed_slot(node, slot); self.trace.record_select_merge_policy(query, policy); match policy { @@ -142,17 +164,12 @@ impl<'a> EngineSession<'a> { slot: &SlotPath, query: &QueryKey, ) -> Result { + self.resolver.counters_mut().binding_lookups += 1; if let Some(entry) = host.binding_for_consumed_slot(node, slot) { self.trace.record_select_binding(query, entry.0); return self.resolve_binding_source(host, entry.0, &entry.1.source); } - self.trace.record_produce_start(query); - let r = host.produce(query, self); - match &r { - Ok(_) => self.trace.record_produce_end(query), - Err(_) => self.trace.record_resolve_error(query), - } - r + self.produce_through_host(host, query) } fn resolve_error_merge_consumed_slot( @@ -162,6 +179,7 @@ impl<'a> EngineSession<'a> { slot: &SlotPath, query: &QueryKey, ) -> Result { + self.resolver.counters_mut().binding_lookups += 1; let entries = host.bindings_for_consumed_slot(node, slot); if entries.len() > 1 { return Err(SessionResolveError::other(format!( @@ -178,6 +196,7 @@ impl<'a> EngineSession<'a> { slot: &SlotPath, query: &QueryKey, ) -> Result { + self.resolver.counters_mut().binding_lookups += 1; let entries = host.bindings_for_consumed_slot(node, slot); if entries.is_empty() { return self.resolve_latest_consumed_slot(host, node, slot, query); @@ -207,6 +226,7 @@ impl<'a> EngineSession<'a> { ) -> Result { match source { BindingSource::Literal(spec) => { + self.resolver.counters_mut().literal_materializations += 1; let product = materialize_literal_product(spec, self.revision); Ok(Production::leaf(product, ProductionSource::Literal)) } @@ -246,6 +266,7 @@ impl<'a> EngineSession<'a> { self.trace .try_push_active(&bus_query) .map_err(SessionResolveError::from)?; + self.resolver.counters_mut().binding_lookups += 1; let mut providers = host.providers_for_bus(channel); providers.sort_by_key(|(provider_ref, entry)| (entry.priority, *provider_ref)); for (provider_ref, provider) in providers.iter() { diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs index 2cdf5d2e3..92ed11da6 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs @@ -1,4 +1,27 @@ -//! Resolver — same-frame demand-resolution cache owner. +//! Resolver — demand-resolution cache owner. +//! +//! # Invalidation contract +//! +//! The resolver distinguishes two kinds of "this is no longer valid", and +//! every caller must pick the right one: +//! +//! - [`Resolver::begin_frame`] — a new frame starts. Values resolved *for the +//! previous frame* are stale, because producers tick again and time moves. +//! The shape of the graph has not changed. +//! - [`Resolver::invalidate_structure`] — the graph itself changed: bindings, +//! tree topology, node definitions, or project state. Everything the +//! resolver knows is suspect, including which binding wins a slot. +//! +//! **Any future engine mutation that can change what a query resolves +//! *through* — as opposed to what it resolves *to* this frame — must call +//! [`Resolver::invalidate_structure`].** Getting this wrong does not fail +//! loudly; it serves a stale answer. The known sites are enumerated in +//! `docs/adr/2026-07-31-resolver-persistent-resolution.md`. +//! +//! Frame-scoped and structure-scoped invalidation are deliberately separate +//! names rather than one `clear()`, so that persisting resolution across +//! frames is a change of what each one *does*, not a hunt for which callers +//! meant which. use crate::dataflow::resolver::resolve_error::ResolveError; use crate::dataflow::resolver::resolver_cache::ResolverCache; @@ -7,16 +30,57 @@ use lpc_model::Revision; use lpc_model::WithRevision; use lps_shared::LpsValueF32; -/// Owns the same-frame [`ResolverCache`] for engine demand resolution. +/// Resolution work performed during one frame. +/// +/// These exist to make "did the frame re-resolve the graph from cold?" a +/// testable question rather than a profiling exercise: a steady-state frame +/// over an unchanged graph should perform no structural work at all. They are +/// always on — each is a single increment on a path that already allocates. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct ResolveFrameCounters { + /// Binding-index lookups (`binding_for_consumed_slot`, + /// `bindings_for_consumed_slot`, `providers_for_bus`). + pub binding_lookups: u32, + /// Merge-policy reads, each of which introspects an authored node def. + pub merge_policy_reads: u32, + /// Productions answered from an authored def (`ProductionSource::Default`). + pub def_produces: u32, + /// Binding literals materialized into productions. + pub literal_materializations: u32, + /// Productions answered by ticking a producer node. + pub producer_produces: u32, + /// Queries answered from cache. + pub cache_hits: u32, + /// Queries that had to be resolved. + pub uncached_resolves: u32, +} + +impl ResolveFrameCounters { + /// Work that only a structural change can invalidate. + /// + /// A steady-state frame over an unchanged graph should report zero here; + /// anything else means the frame re-derived the graph it already knew. + pub fn structural_work(&self) -> u32 { + self.binding_lookups + self.merge_policy_reads + self.def_produces + } +} + +/// Owns the [`ResolverCache`] for engine demand resolution. #[derive(Clone, Debug, Default)] pub struct Resolver { cache: ResolverCache, + structure_epoch: u64, + frame_counters: ResolveFrameCounters, + last_frame_counters: ResolveFrameCounters, } impl Resolver { pub fn new() -> Self { Self { cache: ResolverCache::new(), + structure_epoch: 0, + frame_counters: ResolveFrameCounters::default(), + last_frame_counters: ResolveFrameCounters::default(), } } @@ -28,9 +92,39 @@ impl Resolver { &mut self.cache } - pub fn clear_frame_cache(&mut self) { + /// Start a new frame: per-frame resolved values are discarded. + pub fn begin_frame(&mut self) { + self.last_frame_counters = self.frame_counters; + self.frame_counters = ResolveFrameCounters::default(); + self.cache.clear(); + } + + /// The graph changed shape: every cached decision and value is suspect. + pub fn invalidate_structure(&mut self) { + self.structure_epoch = self.structure_epoch.wrapping_add(1); self.cache.clear(); } + + /// How many times the graph has changed shape. Only equality across two + /// observations is meaningful; the absolute value is not. + pub fn structure_epoch(&self) -> u64 { + self.structure_epoch + } + + /// Counters for the frame currently being resolved. + pub fn frame_counters(&self) -> &ResolveFrameCounters { + &self.frame_counters + } + + /// Counters for the last frame that completed — readable after the tick + /// returns, which is when tests can look at them. + pub fn last_frame_counters(&self) -> &ResolveFrameCounters { + &self.last_frame_counters + } + + pub(crate) fn counters_mut(&mut self) -> &mut ResolveFrameCounters { + &mut self.frame_counters + } } /// Materialize a direct [`lpc_model::LpValue`] literal to a versioned slot value. diff --git a/lp-core/lpc-engine/src/engine/engine.rs b/lp-core/lpc-engine/src/engine/engine.rs index 859ba3145..bb29b0242 100644 --- a/lp-core/lpc-engine/src/engine/engine.rs +++ b/lp-core/lpc-engine/src/engine/engine.rs @@ -169,7 +169,7 @@ impl Engine { } self.demand_roots.retain(|root| !ids.contains(root)); self.tree.remove_subtree(node, frame)?; - self.resolver.clear_frame_cache(); + self.resolver.invalidate_structure(); Ok(()) } @@ -181,7 +181,7 @@ impl Engine { ) -> Result<(), EngineError> { self.cleanup_runtime_node(node, frame)?; self.attach_runtime_node(node, runtime, frame)?; - self.resolver.clear_frame_cache(); + self.resolver.invalidate_structure(); Ok(()) } @@ -241,7 +241,22 @@ impl Engine { draft: BindingDraft, revision: Revision, ) -> Result { - self.tree.add_binding(draft, revision) + let binding_ref = self.tree.add_binding(draft, revision)?; + // A new binding can win a slot that already resolved, so every + // cached decision about that slot is now wrong. + self.resolver.invalidate_structure(); + Ok(binding_ref) + } + + /// Drop every node-owned binding. The loader's binding phase re-registers + /// from defs afterwards; see [`crate::node::RuntimeNodeTree::clear_bindings`]. + /// + /// Goes through the engine rather than the tree so that emptying the + /// binding graph invalidates resolution on its own, instead of depending + /// on a later `add_binding` happening to do it. + pub fn clear_bindings(&mut self, revision: Revision) { + self.tree.clear_bindings(revision); + self.resolver.invalidate_structure(); } /// Optional graphics backend for core shader nodes; clone is cheap (`Arc`). @@ -415,7 +430,7 @@ impl Engine { } fn tick_nodes(&mut self, registry: &ProjectRegistry, delta_ms: u32) -> Result<(), EngineError> { - self.resolver.clear_frame_cache(); + self.resolver.begin_frame(); self.frame_num = self.frame_num.next(); self.revision = advance_revision(); self.frame_time = @@ -500,7 +515,10 @@ impl Engine { let key = QueryKey::Bus(lpc_model::ChannelName(channel.to_string())); let fid = self.revision; let mut resolver_tmp = core::mem::replace(&mut self.resolver, Resolver::new()); - resolver_tmp.clear_frame_cache(); + // A forced-fresh read, not an invalidation: the caller wants values + // re-resolved rather than whatever the last tick left behind. The + // graph has not changed, so structural knowledge must survive. + resolver_tmp.begin_frame(); let mut session = EngineSession::new( fid, &mut resolver_tmp, @@ -1844,7 +1862,7 @@ pub(crate) fn resolve_with_engine_host( ) -> Result<(Production, ResolveTrace), SessionResolveError> { let fid = eng.revision; let mut resolver_tmp = core::mem::replace(&mut eng.resolver, Resolver::new()); - resolver_tmp.clear_frame_cache(); + resolver_tmp.begin_frame(); let mut session = EngineSession::new(fid, &mut resolver_tmp, ResolveTrace::new(log_level)); let mut producers_ticked = VecSet::new(); let time_s = eng.frame_time.total_ms as f32 / 1000.0; @@ -1878,7 +1896,7 @@ pub(super) fn resolve_twice_same_frame_with_engine_host( ) -> Result<(Production, Production), SessionResolveError> { let fid = eng.revision; let mut resolver_tmp = core::mem::replace(&mut eng.resolver, Resolver::new()); - resolver_tmp.clear_frame_cache(); + resolver_tmp.begin_frame(); let mut session = EngineSession::new( fid, &mut resolver_tmp, diff --git a/lp-core/lpc-engine/src/engine/mod.rs b/lp-core/lpc-engine/src/engine/mod.rs index bc397fd86..eb2a13aad 100644 --- a/lp-core/lpc-engine/src/engine/mod.rs +++ b/lp-core/lpc-engine/src/engine/mod.rs @@ -21,6 +21,8 @@ mod project_read_runtime; mod project_read_shapes; mod project_read_stream; mod project_runtime_index; +#[cfg(test)] +mod resolution_persistence_tests; mod srgb8_lut; #[cfg(test)] pub(crate) mod test_support; diff --git a/lp-core/lpc-engine/src/engine/project_apply.rs b/lp-core/lpc-engine/src/engine/project_apply.rs index 25eaba015..c55e55317 100644 --- a/lp-core/lpc-engine/src/engine/project_apply.rs +++ b/lp-core/lpc-engine/src/engine/project_apply.rs @@ -58,7 +58,10 @@ impl Engine { changes: &ProjectChangeSummary, ) -> Result { if changes.is_empty() { - self.resolver_mut().clear_frame_cache(); + // Conservative: an apply that summarized to "nothing changed" + // still ran against the registry, and a false negative here + // serves stale resolution rather than failing loudly. + self.resolver_mut().invalidate_structure(); self.project_runtime_index_mut() .rebuild_asset_consumers(®istry.inventory().tree); return Ok(RuntimeApplyResult::default()); @@ -216,10 +219,10 @@ impl Engine { // (dozens of entries) and by construction identical to a fresh load // (incremental binding apply, Option C). let projected_nodes = ProjectLoader::ensure_runtime_spine(registry, self, frame)?; - self.tree_mut().clear_bindings(frame); + self.clear_bindings(frame); ProjectLoader::register_projected_bindings(registry, self, &projected_nodes, frame)?; - self.resolver_mut().clear_frame_cache(); + self.resolver_mut().invalidate_structure(); Ok(result) } diff --git a/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs b/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs new file mode 100644 index 000000000..8e403d8fb --- /dev/null +++ b/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs @@ -0,0 +1,275 @@ +//! What must still change when resolution stops being recomputed per frame. +//! +//! The resolver caches resolution across frames and drops that knowledge only +//! when [`Resolver::invalidate_structure`] says the graph changed shape. Every +//! test here names one way the graph can change and asserts the engine still +//! reports the new answer. They are written against observable values rather +//! than cache internals, so they pin behaviour that must hold whether or not +//! anything is cached at all — they were green before the caching existed, and +//! staying green is the point. +//! +//! The counter tests are the exception: they assert *how* a frame reached its +//! answer, which is the only way to catch resolution silently going back to +//! re-deriving the graph every tick. + +use super::test_support::{EngineTestBuilder, bus, literal, output, produced_slot}; +use crate::dataflow::binding::{BindingDraft, BindingPriority, BindingSource, BindingTarget}; +use lpc_model::{Kind, LpValue, Revision, SlotPath}; + +fn path(p: &str) -> SlotPath { + SlotPath::parse(p).expect("test slot path") +} + +/// Re-binding a consumed slot mid-run must change what it resolves to. +/// +/// This is the shape of the studio bind gesture and of every project apply: +/// bindings are load-time materializations, so a re-bind *replaces* the +/// binding set (`clear_bindings` then re-register from defs) rather than +/// adding a competing binding. Consumed slots resolve by owner depth and +/// registration order — priority arbitrates bus providers, not these — so +/// replacement is the only way a consumed slot's source changes. +#[test] +fn rebinding_a_consumed_slot_mid_run_changes_its_value() { + let mut harness = EngineTestBuilder::new() + .output_node("out") + .bind_demand_input("out", literal(1.0)) + .demand_root("out") + .build(); + + harness.tick(16).expect("first tick"); + assert_eq!(harness.output_f32("out"), Some(1.0)); + + let out = harness.node("out"); + harness.engine.clear_bindings(Revision::new(2)); + harness + .engine + .add_binding( + BindingDraft { + source: BindingSource::Literal(LpValue::F32(9.0)), + target: BindingTarget::ConsumedSlot { + node: out, + slot: path("in"), + }, + priority: BindingPriority::new(0), + kind: Kind::Color, + owner: out, + }, + Revision::new(2), + ) + .expect("re-register binding"); + + harness.tick(16).expect("second tick"); + assert_eq!( + harness.output_f32("out"), + Some(9.0), + "a re-bound slot must not keep resolving through the binding it no longer has" + ); +} + +/// A higher-priority bus provider added mid-run must take the channel over. +/// +/// Priority genuinely arbitrates here (unlike consumed slots), so this is the +/// one path where an *added* binding changes an existing answer. +#[test] +fn higher_priority_bus_provider_added_mid_run_wins_next_tick() { + let mut harness = EngineTestBuilder::new() + .output_node("out") + .bind_bus("chan", literal(1.0)) + .bind_demand_input("out", bus("chan")) + .demand_root("out") + .build(); + + harness.tick(16).expect("first tick"); + assert_eq!(harness.output_f32("out"), Some(1.0)); + + let out = harness.node("out"); + harness + .engine + .add_binding( + BindingDraft { + source: BindingSource::Literal(LpValue::F32(9.0)), + target: BindingTarget::BusChannel(lpc_model::ChannelName( + alloc::string::String::from("chan"), + )), + priority: BindingPriority::new(10), + kind: Kind::Color, + owner: out, + }, + Revision::new(2), + ) + .expect("add higher-priority provider"); + + harness.tick(16).expect("second tick"); + assert_eq!( + harness.output_f32("out"), + Some(9.0), + "a higher-priority provider must beat the cached resolution of the channel" + ); +} + +/// Replacing a producer node must change what its consumers see. +#[test] +fn reattached_producer_node_supplies_the_new_value() { + let mut harness = EngineTestBuilder::new() + .shader("src", output("outputs[0]", 2.0)) + .output_node("out") + .bind_demand_input("out", produced_slot("src", "outputs[0]")) + .demand_root("out") + .build(); + + harness.tick(16).expect("first tick"); + assert_eq!(harness.output_f32("out"), Some(2.0)); + + let src = harness.node("src"); + let replacement = super::test_support::dummy_shader_node(path("outputs[0]"), 7.0); + harness + .engine + .reattach_runtime_node(src, replacement, Revision::new(3)) + .expect("reattach producer"); + + harness.tick(16).expect("second tick"); + assert_eq!( + harness.output_f32("out"), + Some(7.0), + "a replaced producer must not keep serving the old node's cached production" + ); +} + +/// A node that demands a different producer than last frame — the playlist's +/// switch — must get the newly demanded one, not the previously cached one. +#[test] +fn selector_switching_targets_resolves_the_newly_demanded_producer() { + let mut harness = EngineTestBuilder::new() + .shader("a", output("outputs[0]", 3.0)) + .shader("b", output("outputs[0]", 4.0)) + .selector("sel", &[("a", "outputs[0]"), ("b", "outputs[0]")]) + .demand_root("sel") + .build(); + + harness.tick(16).expect("first tick"); + assert_eq!(harness.output_f32("sel"), Some(3.0)); + + // Runtime state only: no binding moves, no node is added or removed. + harness.select("sel", 1); + harness.tick(16).expect("second tick"); + assert_eq!( + harness.output_f32("sel"), + Some(4.0), + "switching which producer is demanded is not a structural change, and must not need one" + ); + + harness.select("sel", 0); + harness.tick(16).expect("third tick"); + assert_eq!(harness.output_f32("sel"), Some(3.0), "and back again"); +} + +/// Producers keep ticking every frame even when the graph is unchanged. +/// +/// Persisting *resolution* must not persist *values*: a shader still runs, and +/// its output still carries the current frame's revision. +#[test] +fn producers_still_tick_every_frame_on_an_unchanged_graph() { + let mut harness = EngineTestBuilder::new() + .shader("src", output("outputs[0]", 5.0)) + .output_node("out") + .bind_demand_input("out", produced_slot("src", "outputs[0]")) + .demand_root("out") + .build(); + + harness.tick(16).expect("first tick"); + harness.reset_shader_ticks("src"); + + for _ in 0..3 { + harness.tick(16).expect("steady tick"); + } + + assert_eq!( + harness.shader_ticks("src"), + 3, + "a cached route must still run the producer behind it, once per frame" + ); +} + +/// Steady-state frames over an unchanged graph do the same amount of work. +/// +/// This is the weaker, always-true form of the zero-structural-work assertion: +/// whatever a frame costs, it must not grow frame over frame. +#[test] +fn steady_state_frames_report_stable_counters() { + let mut harness = EngineTestBuilder::new() + .shader("src", output("outputs[0]", 1.5)) + .output_node("out") + .bind_demand_input("out", produced_slot("src", "outputs[0]")) + .demand_root("out") + .build(); + + harness.tick(16).expect("warm-up tick"); + harness.tick(16).expect("second tick"); + let second = *harness.engine.resolver().last_frame_counters(); + harness.tick(16).expect("third tick"); + let third = *harness.engine.resolver().last_frame_counters(); + + assert_eq!( + second, third, + "two steady frames over an unchanged graph must cost the same resolution work" + ); +} + +/// Structural invalidation is observable and monotonic, so that a future +/// mutation site that forgets to call it can be caught by a test rather than +/// by a stale value in the field. +#[test] +fn structural_mutations_bump_the_epoch() { + let mut harness = EngineTestBuilder::new() + .shader("src", output("outputs[0]", 1.0)) + .output_node("out") + .bind_demand_input("out", produced_slot("src", "outputs[0]")) + .demand_root("out") + .build(); + + harness.tick(16).expect("tick"); + let before_tick = harness.engine.resolver().structure_epoch(); + harness.tick(16).expect("another tick"); + assert_eq!( + harness.engine.resolver().structure_epoch(), + before_tick, + "ticking is not a structural change" + ); + + let out = harness.node("out"); + harness + .engine + .add_binding( + BindingDraft { + source: BindingSource::Literal(LpValue::F32(2.0)), + target: BindingTarget::ConsumedSlot { + node: out, + slot: path("other"), + }, + priority: BindingPriority::new(1), + kind: Kind::Color, + owner: out, + }, + Revision::new(4), + ) + .expect("add binding"); + assert!( + harness.engine.resolver().structure_epoch() > before_tick, + "adding a binding is a structural change" + ); + + let epoch_after_binding = harness.engine.resolver().structure_epoch(); + let src = harness.node("src"); + harness + .engine + .reattach_runtime_node( + src, + super::test_support::dummy_shader_node(path("outputs[0]"), 1.0), + Revision::new(5), + ) + .expect("reattach"); + assert!( + harness.engine.resolver().structure_epoch() > epoch_after_binding, + "reattaching a node is a structural change" + ); +} diff --git a/lp-core/lpc-engine/src/engine/test_support.rs b/lp-core/lpc-engine/src/engine/test_support.rs index 5abe50921..fcd3f86d9 100644 --- a/lp-core/lpc-engine/src/engine/test_support.rs +++ b/lp-core/lpc-engine/src/engine/test_support.rs @@ -36,6 +36,7 @@ pub(crate) struct EngineTestBuilder { shader_ticks: VecMap>, fixture_records: VecMap, output_records: VecMap, + selectors: VecMap>, } pub(crate) struct EngineTestHarness { @@ -45,6 +46,7 @@ pub(crate) struct EngineTestHarness { shader_ticks: VecMap>, fixture_records: VecMap, output_records: VecMap, + selectors: VecMap>, } pub(crate) struct OutputSpec { @@ -73,6 +75,7 @@ impl EngineTestBuilder { shader_ticks: VecMap::new(), fixture_records: VecMap::new(), output_records: VecMap::new(), + selectors: VecMap::new(), } } @@ -100,6 +103,23 @@ impl EngineTestBuilder { self } + /// A node that demands one of `targets` (label, slot) depending on the + /// returned selector — the playlist's switch mechanism. See + /// [`DummySelectorNode`]. + pub(crate) fn selector(mut self, label: &str, targets: &[(&str, &str)]) -> Self { + let record = RecordedValue::new(); + let selected = Arc::new(AtomicU32::new(0)); + let resolved: Vec<(NodeId, SlotPath)> = targets + .iter() + .map(|(target, slot)| (self.node_id(target), path(slot))) + .collect(); + let node = DummySelectorNode::new(resolved, Arc::clone(&selected), record.clone()); + self.attach_node(label, "selector", Box::new(node)); + self.output_records.insert(String::from(label), record); + self.selectors.insert(String::from(label), selected); + self + } + pub(crate) fn bind_bus(self, channel: &str, source: TestBindingSource) -> Self { self.bind_bus_with_priority(channel, source, 0) .expect("bind bus") @@ -166,6 +186,7 @@ impl EngineTestBuilder { shader_ticks: self.shader_ticks, fixture_records: self.fixture_records, output_records: self.output_records, + selectors: self.selectors, } } @@ -277,6 +298,15 @@ impl EngineTestHarness { ) } + /// Point a selector node at a different target. Runtime state only — no + /// binding or tree change, exactly like a playlist entry switch. + pub(crate) fn select(&self, label: &str, index: u32) { + self.selectors + .get(label) + .expect("selector label") + .store(index, Ordering::Relaxed); + } + pub(crate) fn tick(&mut self, delta_ms: u32) -> Result<(), super::EngineError> { self.engine.tick(&self.registry, delta_ms) } @@ -353,6 +383,16 @@ pub(crate) fn bus(channel: &str) -> TestBindingSource { TestBindingSource::Bus(channel_name(channel)) } +/// A standalone producer runtime for tests that swap a node's runtime out +/// from under its consumers (`reattach_runtime_node`). +pub(crate) fn dummy_shader_node(slot: SlotPath, value: f32) -> Box { + Box::new(DummyShaderNode::new( + slot, + LpsValueF32::F32(value), + Arc::new(AtomicU32::new(0)), + )) +} + pub(crate) fn path(path: &str) -> SlotPath { SlotPath::parse(path).expect("test slot path") } @@ -513,6 +553,57 @@ impl NodeRuntime for DummyOutputNode { } } +/// Demands a *different* producer's slot depending on its own runtime state — +/// the playlist's mechanism, without a playlist. +/// +/// A playlist switch changes nothing structural: no binding moves, no node is +/// added or removed. It just asks for a different child's output than it asked +/// for last frame. Resolution caching must survive that, so the behaviour has +/// a pin of its own that does not need the playlist node feature. +pub(crate) struct DummySelectorNode { + targets: Vec<(NodeId, SlotPath)>, + selected: Arc, + record: RecordedValue, +} + +impl DummySelectorNode { + fn new( + targets: Vec<(NodeId, SlotPath)>, + selected: Arc, + record: RecordedValue, + ) -> Self { + Self { + targets, + selected, + record, + } + } +} + +impl NodeRuntime for DummySelectorNode { + fn consume(&mut self, ctx: &mut TickContext<'_>) -> Result<(), NodeError> { + let index = self.selected.load(Ordering::Relaxed) as usize % self.targets.len(); + let (node, slot) = self.targets[index].clone(); + let pv = ctx + .resolve(QueryKey::ProducedSlot { node, slot }) + .map_err(|e| NodeError::msg(format!("selector resolve failed: {}", e.message)))?; + self.record.record(&pv.as_value().expect("value")); + Ok(()) + } + + fn destroy(&mut self, _ctx: &mut DestroyCtx<'_>) -> Result<(), NodeError> { + Ok(()) + } + + fn handle_memory_pressure( + &mut self, + _level: PressureLevel, + _ctx: &mut MemPressureCtx<'_>, + ) -> Result<(), NodeError> { + Ok(()) + } +} + fn channel_name(name: &str) -> ChannelName { ChannelName(String::from(name)) } From 2377a54d269f21d494230fc006521564a1d6615b Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:24:13 -0700 Subject: [PATCH 3/9] perf(lpc-engine): resolve the binding graph once, not once per frame MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolving a slot is two jobs: decide which binding answers it — owner depth, priority, merge policy, expanding bus providers, all reads of the binding graph and of authored defs — then get the value through that decision, which may tick a producer. Only the second is per-frame, but the frame did both, every tick, for every slot. So the decision becomes a cached `ResolvedRoute`, alive until the graph changes shape, and def-sourced values (binding literals, authored-def reads and their deep slot-data copies) join it. Producer values stay strictly per-frame — a cached route still runs the producer behind it. Queries are interned to `QueryId`, which is what makes the rest cheap: cache lookups index an array instead of comparing `SlotPath` strings, the cycle-detection stack becomes `Copy` (it was cloning a heap-backed key on every uncached resolve even with tracing off), and discarding a frame's values is a counter bump rather than dropping and reallocating every entry. 1-fixture profile, steady render, esp32c6 cycle model: total cycles 2,468,427 -> 1,218,709 (-51%) alloc+memcpy 44.0% -> 34.2% [jit] render 1.1% -> 2.2% (same cycles, half the frame) and `QueryKey::eq`, `merge_policy_for_consumed_slot`, `bindings_for_consumed_slot`, `slot_lookup` and `Vec::clone` are all gone from the top twenty. What replaces them at the top — `intern_query`, `hash_slot_path`, `SlotPath::parse` — is the cost of nodes re-deriving keys they could hold onto, which is P4's subject. The differential test is the one to keep: it runs a scene through binding replacement, a producer switch and a node reattach twice, cached and force-invalidated, and demands the two agree. A stale cache returns a plausible answer, so it survives assertions written by whoever wrote the cache; it does not survive being compared against not caching at all. Plan: 2026-07-31-2225-persist-dataflow-resolution --- .../lpc-engine/src/dataflow/resolver/mod.rs | 4 + .../src/dataflow/resolver/query_intern.rs | 233 ++++++++++++ .../src/dataflow/resolver/resolve_session.rs | 336 ++++++++++-------- .../src/dataflow/resolver/resolve_trace.rs | 108 +++--- .../src/dataflow/resolver/resolver.rs | 51 ++- .../src/dataflow/resolver/resolver_cache.rs | 273 +++++++++----- .../lpc-engine/src/dataflow/resolver/route.rs | 35 ++ .../engine/resolution_persistence_tests.rs | 166 ++++++++- 8 files changed, 906 insertions(+), 300 deletions(-) create mode 100644 lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs create mode 100644 lp-core/lpc-engine/src/dataflow/resolver/route.rs diff --git a/lp-core/lpc-engine/src/dataflow/resolver/mod.rs b/lp-core/lpc-engine/src/dataflow/resolver/mod.rs index d6be3b8be..92bd7fe92 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/mod.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/mod.rs @@ -10,6 +10,7 @@ //! carry maps, records, options, and receiver-owned merge results. pub mod production; +pub mod query_intern; pub mod query_key; pub mod resolve_error; pub mod resolve_host; @@ -17,9 +18,11 @@ pub mod resolve_session; pub mod resolve_trace; pub mod resolver; pub mod resolver_cache; +pub mod route; pub mod tick_resolver; pub use production::{Production, ProductionSource}; +pub use query_intern::{QueryId, QueryInternTable}; pub use query_key::QueryKey; pub use resolve_error::{ResolveError, SessionResolveError}; pub use resolve_host::ResolveHost; @@ -29,4 +32,5 @@ pub use resolve_trace::{ }; pub use resolver::{ResolveFrameCounters, Resolver}; pub use resolver_cache::ResolverCache; +pub use route::ResolvedRoute; pub use tick_resolver::{SessionHostResolver, TickResolver}; diff --git a/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs b/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs new file mode 100644 index 000000000..0c1385d1c --- /dev/null +++ b/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs @@ -0,0 +1,233 @@ +//! Interning of [`QueryKey`]s to dense [`QueryId`]s. +//! +//! Resolution addresses slots by name: a `QueryKey` owns a `SlotPath` (a `Vec` +//! of heap `String` segments) or a `ChannelName`. Comparing those on every +//! cache probe means string compares, and storing them in every cache entry, +//! trace frame, and route means cloning heap data per frame. +//! +//! Interning pays that cost once per distinct query per structural epoch. +//! Afterwards a query is a `u32`: cache lookups index an array, the cycle +//! stack is `Copy`, and nothing allocates. +//! +//! Ids are meaningful only within one epoch. [`QueryInternTable::clear`] runs +//! when the graph changes shape, and every id handed out before it is void. + +use alloc::vec::Vec; +use core::hash::{Hash, Hasher}; +use lpc_model::SlotPath; + +use crate::dataflow::resolver::query_key::QueryKey; + +/// A [`QueryKey`] reduced to an index, valid for one structural epoch. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub struct QueryId(u32); + +impl QueryId { + pub fn index(self) -> usize { + self.0 as usize + } +} + +/// FNV-1a. Small, no dependency, and good enough for a lookup filter — the +/// table verifies every candidate with `==`, so a collision costs a compare, +/// never a wrong answer. +#[derive(Default)] +struct FnvHasher(u64); + +impl Hasher for FnvHasher { + fn finish(&self) -> u64 { + self.0 + } + + fn write(&mut self, bytes: &[u8]) { + let mut hash = if self.0 == 0 { + 0xcbf2_9ce4_8422_2325 + } else { + self.0 + }; + for byte in bytes { + hash ^= *byte as u64; + hash = hash.wrapping_mul(0x1000_0000_01b3); + } + self.0 = hash; + } +} + +/// Hash the parts of a key that are cheap and highly discriminating. +/// +/// Deliberately *not* the whole key: a `SlotAccessor` carries compiled steps +/// that add nothing two accessors with the same node and path would not +/// already differ on in practice. Equality still decides. +fn hash_query(query: &QueryKey) -> u64 { + let mut hasher = FnvHasher::default(); + match query { + QueryKey::Bus(channel) => { + hasher.write_u8(0); + channel.hash(&mut hasher); + } + QueryKey::ProducedSlot { node, slot } => { + hasher.write_u8(1); + node.hash(&mut hasher); + hash_slot_path(slot, &mut hasher); + } + QueryKey::ConsumedSlot { node, slot } => { + hasher.write_u8(2); + node.hash(&mut hasher); + hash_slot_path(slot, &mut hasher); + } + QueryKey::ConsumedSlotAccessor { node, accessor } => { + hasher.write_u8(3); + node.hash(&mut hasher); + hash_slot_path(accessor.path(), &mut hasher); + } + } + hasher.finish() +} + +fn hash_slot_path(path: &SlotPath, hasher: &mut impl Hasher) { + path.hash(hasher); +} + +/// `QueryKey` ↔ [`QueryId`], scoped to a structural epoch. +#[derive(Clone, Debug, Default)] +pub struct QueryInternTable { + /// Indexed by [`QueryId`]: the key it stands for. + keys: Vec, + /// `(hash, id)` sorted by hash, so lookup is a binary search plus one + /// equality check in the common case. + by_hash: Vec<(u64, QueryId)>, +} + +impl QueryInternTable { + pub fn new() -> Self { + Self::default() + } + + /// Id for `query`, assigning one if this is the first time it is seen. + pub fn intern(&mut self, query: &QueryKey) -> QueryId { + let hash = hash_query(query); + let start = self.by_hash.partition_point(|(h, _)| *h < hash); + for (candidate_hash, id) in &self.by_hash[start..] { + if *candidate_hash != hash { + break; + } + if &self.keys[id.index()] == query { + return *id; + } + } + + let id = QueryId(self.keys.len() as u32); + self.keys.push(query.clone()); + self.by_hash.insert(start, (hash, id)); + id + } + + /// Id for `query` if it has been interned, without assigning one. + pub fn lookup(&self, query: &QueryKey) -> Option { + let hash = hash_query(query); + let start = self.by_hash.partition_point(|(h, _)| *h < hash); + self.by_hash[start..] + .iter() + .take_while(|(candidate_hash, _)| *candidate_hash == hash) + .find(|(_, id)| &self.keys[id.index()] == query) + .map(|(_, id)| *id) + } + + /// The key an id stands for. Cold path: error formatting and tracing. + pub fn key(&self, id: QueryId) -> Option<&QueryKey> { + self.keys.get(id.index()) + } + + pub fn len(&self) -> usize { + self.keys.len() + } + + pub fn is_empty(&self) -> bool { + self.keys.is_empty() + } + + pub fn clear(&mut self) { + self.keys.clear(); + self.by_hash.clear(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + use alloc::string::String; + use lpc_model::{ChannelName, NodeId}; + + fn bus(name: &str) -> QueryKey { + QueryKey::Bus(ChannelName(String::from(name))) + } + + fn produced(node: u32, slot: &str) -> QueryKey { + QueryKey::ProducedSlot { + node: NodeId::new(node), + slot: SlotPath::parse(slot).expect("slot path"), + } + } + + #[test] + fn interning_is_stable_and_distinct() { + let mut table = QueryInternTable::new(); + let a = table.intern(&bus("video")); + let b = table.intern(&bus("audio")); + let a_again = table.intern(&bus("video")); + + assert_eq!(a, a_again, "the same key must intern to the same id"); + assert_ne!(a, b); + assert_eq!(table.len(), 2, "re-interning must not grow the table"); + assert_eq!(table.key(a), Some(&bus("video"))); + } + + #[test] + fn keys_differing_only_by_variant_do_not_collide() { + let mut table = QueryInternTable::new(); + let produced_id = table.intern(&produced(3, "out")); + let consumed_id = table.intern(&QueryKey::ConsumedSlot { + node: NodeId::new(3), + slot: SlotPath::parse("out").expect("slot path"), + }); + assert_ne!(produced_id, consumed_id); + } + + #[test] + fn lookup_finds_interned_and_misses_unknown() { + let mut table = QueryInternTable::new(); + let id = table.intern(&produced(1, "color")); + assert_eq!(table.lookup(&produced(1, "color")), Some(id)); + assert_eq!(table.lookup(&produced(2, "color")), None); + assert_eq!(table.lookup(&produced(1, "other")), None); + } + + #[test] + fn clear_voids_every_id() { + let mut table = QueryInternTable::new(); + table.intern(&bus("a")); + table.intern(&bus("b")); + table.clear(); + assert!(table.is_empty()); + assert_eq!(table.lookup(&bus("a")), None); + // Ids restart, which is why they may not outlive an epoch. + assert_eq!(table.intern(&bus("z")), QueryId(0)); + } + + #[test] + fn many_keys_round_trip_through_the_hash_index() { + let mut table = QueryInternTable::new(); + let mut ids = Vec::new(); + for node in 0..64u32 { + ids.push(table.intern(&produced(node, "out"))); + } + for (node, id) in ids.iter().enumerate() { + assert_eq!( + table.lookup(&produced(node as u32, "out")), + Some(*id), + "every interned key must be findable regardless of insertion order" + ); + } + assert_eq!(table.len(), 64); + } +} diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs index a72e27142..eee2a3297 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs @@ -1,17 +1,20 @@ //! [`EngineSession`] — per-frame demand resolution and engine-dispatched work. use alloc::format; +use alloc::rc::Rc; use alloc::vec::Vec; use lp_collection::VecMap; use crate::dataflow::binding::{BindingEntry, BindingRef, BindingSource}; use crate::dataflow::resolver::production::{Production, ProductionSource}; +use crate::dataflow::resolver::query_intern::QueryId; use crate::dataflow::resolver::query_key::QueryKey; use crate::dataflow::resolver::resolve_error::SessionResolveError; use crate::dataflow::resolver::resolve_host::ResolveHost; use crate::dataflow::resolver::resolve_trace::ResolveTrace; use crate::dataflow::resolver::resolver::Resolver; use crate::dataflow::resolver::resolver::materialize_literal_product; +use crate::dataflow::resolver::route::ResolvedRoute; use lpc_model::{ChannelName, NodeId, Revision, SlotData, SlotMapDyn, SlotMerge, SlotPath}; /// Active engine session for one frame (or nested test scope). @@ -48,179 +51,264 @@ impl<'a> EngineSession<'a> { } pub fn publish(&mut self, query: QueryKey, production: Production) { - self.resolver.cache_mut().insert(query, production); + let id = self.resolver.intern_query(&query); + self.resolver.cache_mut().insert(id, production); } pub fn publish_produced_slot(&mut self, node: NodeId, slot: SlotPath, production: Production) { self.publish(QueryKey::ProducedSlot { node, slot }, production); } - /// Demand-resolve `query` for this frame (cache + cycle stack + host-owned bindings). + /// Demand-resolve `query` (cache + cycle stack + host-owned bindings). pub fn resolve( &mut self, host: &mut H, query: QueryKey, ) -> Result { - if let Some(pv) = self.resolver.cache().get(&query) { + let id = self.resolver.intern_query(&query); + self.resolve_interned(host, id, &query) + } + + fn resolve_interned( + &mut self, + host: &mut H, + id: QueryId, + query: &QueryKey, + ) -> Result { + if let Some(pv) = self.resolver.cache().get(id) { let pv = pv.clone(); self.resolver.counters_mut().cache_hits += 1; - self.trace.record_cache_hit(&query); + self.trace.record_cache_hit(query); return Ok(pv); } self.resolver.counters_mut().uncached_resolves += 1; self.trace - .try_push_active(&query) + .try_push_active(id, query) .map_err(SessionResolveError::from)?; - match self.resolve_uncached(host, &query) { - Ok(result) => { - self.trace.exit(&query); - self.resolver.cache_mut().insert(query, result.clone()); - Ok(result) - } - Err(err) => { - self.trace.exit(&query); - Err(err) - } - } + let result = self.resolve_uncached(host, id, query); + self.trace.exit(id); + let result = result?; + self.resolver.cache_mut().insert(id, result.clone()); + Ok(result) } fn resolve_uncached( &mut self, host: &mut H, + id: QueryId, query: &QueryKey, ) -> Result { - match query { - QueryKey::Bus(channel) => self.resolve_bus(host, channel, query), - QueryKey::ConsumedSlot { node, slot } => { - self.resolve_consumed_slot(host, *node, slot, query) - } - QueryKey::ConsumedSlotAccessor { node, accessor } => { - self.resolve_consumed_slot(host, *node, accessor.path(), query) - } - QueryKey::ProducedSlot { .. } => self.produce_through_host(host, query), + if let QueryKey::ProducedSlot { .. } = query { + return self.produce_through_host(host, query); } + let route = self.route_for(host, id, query)?; + self.resolve_through_route(host, &route, query) } - /// Ask the host to produce `query`, counting how the answer was obtained. + /// The cached decision about how `query` is answered, computing it on + /// first use in this structural epoch. /// - /// An authored-def answer is structural work (it re-reads a definition - /// that only a project change can alter); a producer answer is per-frame - /// work that must happen every frame. - fn produce_through_host( + /// Everything this computes — which binding wins, the merge policy, the + /// expansion of bus providers — reads the binding graph and authored + /// definitions. None of it can change without a structural change, so it + /// is exactly the work a frame should not repeat. + /// + /// Failures are deliberately not cached: an ambiguous channel or a cyclic + /// graph is a broken project, and it should keep reporting itself rather + /// than be remembered as a decision. + fn route_for( &mut self, host: &mut H, + id: QueryId, query: &QueryKey, - ) -> Result { - self.trace.record_produce_start(query); - let r = host.produce(query, self); - match &r { - Ok(production) => { - match production.source { - ProductionSource::Default => self.resolver.counters_mut().def_produces += 1, - _ => self.resolver.counters_mut().producer_produces += 1, - } - self.trace.record_produce_end(query); + ) -> Result, SessionResolveError> { + if let Some(route) = self.resolver.cache().route(id) { + return Ok(Rc::clone(route)); + } + let route = Rc::new(self.compute_route(host, query)?); + self.resolver + .cache_mut() + .insert_route(id, Rc::clone(&route)); + Ok(route) + } + + fn compute_route( + &mut self, + host: &mut H, + query: &QueryKey, + ) -> Result { + match query { + QueryKey::Bus(channel) => self.compute_bus_route(host, channel), + QueryKey::ConsumedSlot { node, slot } => { + self.compute_consumed_route(host, *node, slot, query) } - Err(_) => self.trace.record_resolve_error(query), + QueryKey::ConsumedSlotAccessor { node, accessor } => { + self.compute_consumed_route(host, *node, accessor.path(), query) + } + QueryKey::ProducedSlot { .. } => Ok(ResolvedRoute::Produce), } - r } - fn resolve_bus( + fn compute_bus_route( &mut self, - host: &mut (impl ResolveHost + ?Sized), + host: &mut H, channel: &ChannelName, - query: &QueryKey, - ) -> Result { + ) -> Result { self.resolver.counters_mut().binding_lookups += 1; let candidates = host.providers_for_bus(channel); - let entry = select_highest_priority_bus_provider(channel, &candidates)?; - self.trace.record_select_binding(query, entry.0); - self.resolve_binding_source(host, entry.0, &entry.1.source) + let (binding_ref, entry) = select_highest_priority_bus_provider(channel, &candidates)?; + Ok(ResolvedRoute::Binding { + binding_ref, + source: entry.source, + }) } - fn resolve_consumed_slot( + fn compute_consumed_route( &mut self, - host: &mut (impl ResolveHost + ?Sized), + host: &mut H, node: NodeId, slot: &SlotPath, query: &QueryKey, - ) -> Result { + ) -> Result { self.resolver.counters_mut().merge_policy_reads += 1; let policy = host.merge_policy_for_consumed_slot(node, slot); self.trace.record_select_merge_policy(query, policy); - match policy { - SlotMerge::Latest => self.resolve_latest_consumed_slot(host, node, slot, query), - SlotMerge::Error => self.resolve_error_merge_consumed_slot(host, node, slot, query), - SlotMerge::ByKey => self.resolve_by_key_consumed_slot(host, node, slot, query), + + if policy == SlotMerge::ByKey { + self.resolver.counters_mut().binding_lookups += 1; + let entries = host.bindings_for_consumed_slot(node, slot); + if !entries.is_empty() { + let mut inputs = Vec::new(); + for (binding_ref, entry) in entries.iter() { + self.expand_merge_inputs(host, *binding_ref, &entry.source, &mut inputs)?; + } + return Ok(ResolvedRoute::MergeByKey { inputs }); + } + return self.compute_latest_consumed_route(host, node, slot); } + + if policy == SlotMerge::Error { + self.resolver.counters_mut().binding_lookups += 1; + if host.bindings_for_consumed_slot(node, slot).len() > 1 { + return Err(SessionResolveError::other(format!( + "multiple bindings for non-mergeable consumed slot node={node:?} slot={slot}" + ))); + } + } + + self.compute_latest_consumed_route(host, node, slot) } - fn resolve_latest_consumed_slot( + fn compute_latest_consumed_route( &mut self, - host: &mut (impl ResolveHost + ?Sized), + host: &mut H, node: NodeId, slot: &SlotPath, - query: &QueryKey, - ) -> Result { + ) -> Result { self.resolver.counters_mut().binding_lookups += 1; - if let Some(entry) = host.binding_for_consumed_slot(node, slot) { - self.trace.record_select_binding(query, entry.0); - return self.resolve_binding_source(host, entry.0, &entry.1.source); - } - self.produce_through_host(host, query) + Ok(match host.binding_for_consumed_slot(node, slot) { + Some((binding_ref, entry)) => ResolvedRoute::Binding { + binding_ref, + source: entry.source, + }, + None => ResolvedRoute::Produce, + }) } - fn resolve_error_merge_consumed_slot( + /// Flatten a mergeable receiver's bindings into leaf sources. + /// + /// A bus source contributes all of its providers, in priority order, and + /// those may themselves be buses. The walk reads only bindings, so its + /// result is part of the route rather than of the frame. + fn expand_merge_inputs( &mut self, - host: &mut (impl ResolveHost + ?Sized), - node: NodeId, - slot: &SlotPath, - query: &QueryKey, - ) -> Result { + host: &mut H, + binding_ref: BindingRef, + source: &BindingSource, + out: &mut Vec<(BindingRef, BindingSource)>, + ) -> Result<(), SessionResolveError> { + let BindingSource::BusChannel(channel) = source else { + out.push((binding_ref, source.clone())); + return Ok(()); + }; + + let bus_query = QueryKey::Bus(channel.clone()); + let bus_id = self.resolver.intern_query(&bus_query); + self.trace + .try_push_active(bus_id, &bus_query) + .map_err(SessionResolveError::from)?; self.resolver.counters_mut().binding_lookups += 1; - let entries = host.bindings_for_consumed_slot(node, slot); - if entries.len() > 1 { - return Err(SessionResolveError::other(format!( - "multiple bindings for non-mergeable consumed slot node={node:?} slot={slot}" - ))); + let mut providers = host.providers_for_bus(channel); + providers.sort_by_key(|(provider_ref, entry)| (entry.priority, *provider_ref)); + for (provider_ref, provider) in providers.iter() { + if let Err(err) = self.expand_merge_inputs(host, *provider_ref, &provider.source, out) { + self.trace.exit(bus_id); + return Err(err); + } } - self.resolve_latest_consumed_slot(host, node, slot, query) + self.trace.exit(bus_id); + Ok(()) } - fn resolve_by_key_consumed_slot( + fn resolve_through_route( &mut self, - host: &mut (impl ResolveHost + ?Sized), - node: NodeId, - slot: &SlotPath, + host: &mut H, + route: &ResolvedRoute, query: &QueryKey, ) -> Result { - self.resolver.counters_mut().binding_lookups += 1; - let entries = host.bindings_for_consumed_slot(node, slot); - if entries.is_empty() { - return self.resolve_latest_consumed_slot(host, node, slot, query); - } - - let mut inputs = Vec::new(); - for (binding_ref, entry) in entries.iter() { - let binding_ref = *binding_ref; - self.trace.record_merge_input(query, binding_ref); - self.resolve_binding_source_for_merge( - host, - query, + match route { + ResolvedRoute::Binding { binding_ref, - &entry.source, - &mut inputs, - )?; + source, + } => { + self.trace.record_select_binding(query, *binding_ref); + self.resolve_binding_source(host, *binding_ref, source) + } + ResolvedRoute::Produce => self.produce_through_host(host, query), + ResolvedRoute::MergeByKey { inputs } => { + let mut productions = Vec::with_capacity(inputs.len()); + for (binding_ref, source) in inputs.iter() { + self.trace.record_merge_input(query, *binding_ref); + let mut production = self.resolve_binding_source(host, *binding_ref, source)?; + production.source = ProductionSource::BusBinding { + binding: *binding_ref, + }; + productions.push(production); + } + merge_maps_by_key(productions, query, &self.trace) + } } + } - merge_maps_by_key(inputs, query, &self.trace) + /// Ask the host to produce `query`, counting how the answer was obtained. + /// + /// An authored-def answer is structural work (it re-reads a definition + /// that only a project change can alter); a producer answer is per-frame + /// work that must happen every frame. + fn produce_through_host( + &mut self, + host: &mut H, + query: &QueryKey, + ) -> Result { + self.trace.record_produce_start(query); + let r = host.produce(query, self); + match &r { + Ok(production) => { + match production.source { + ProductionSource::Default => self.resolver.counters_mut().def_produces += 1, + _ => self.resolver.counters_mut().producer_produces += 1, + } + self.trace.record_produce_end(query); + } + Err(_) => self.trace.record_resolve_error(query), + } + r } - fn resolve_binding_source( + fn resolve_binding_source( &mut self, - host: &mut (impl ResolveHost + ?Sized), + host: &mut H, binding_ref: BindingRef, source: &BindingSource, ) -> Result { @@ -251,54 +339,6 @@ impl<'a> EngineSession<'a> { } } } - - fn resolve_binding_source_for_merge( - &mut self, - host: &mut (impl ResolveHost + ?Sized), - query: &QueryKey, - binding_ref: BindingRef, - source: &BindingSource, - out: &mut Vec, - ) -> Result<(), SessionResolveError> { - match source { - BindingSource::BusChannel(channel) => { - let bus_query = QueryKey::Bus(channel.clone()); - self.trace - .try_push_active(&bus_query) - .map_err(SessionResolveError::from)?; - self.resolver.counters_mut().binding_lookups += 1; - let mut providers = host.providers_for_bus(channel); - providers.sort_by_key(|(provider_ref, entry)| (entry.priority, *provider_ref)); - for (provider_ref, provider) in providers.iter() { - let provider_ref = *provider_ref; - self.trace.record_merge_input(query, provider_ref); - match self.resolve_binding_source_for_merge( - host, - query, - provider_ref, - &provider.source, - out, - ) { - Ok(()) => {} - Err(err) => { - self.trace.exit(&bus_query); - return Err(err); - } - } - } - self.trace.exit(&bus_query); - Ok(()) - } - _ => { - let mut production = self.resolve_binding_source(host, binding_ref, source)?; - production.source = ProductionSource::BusBinding { - binding: binding_ref, - }; - out.push(production); - Ok(()) - } - } - } } fn merge_maps_by_key( diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolve_trace.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolve_trace.rs index df4b78c54..1b48d9e8e 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolve_trace.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolve_trace.rs @@ -1,6 +1,7 @@ //! Active query stack (correctness) plus optional structured resolver events. use crate::dataflow::binding::BindingRef; +use crate::dataflow::resolver::query_intern::QueryId; use crate::dataflow::resolver::query_key::QueryKey; use alloc::vec::Vec; use core::cell::RefCell; @@ -57,20 +58,18 @@ pub enum ResolveTraceError { /// RAII scope for one active query; pops the stack on drop. pub struct TraceGuard<'a> { trace: &'a ResolveTrace, - query: QueryKey, + id: QueryId, } impl fmt::Debug for TraceGuard<'_> { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("TraceGuard") - .field("query", &self.query) - .finish() + f.debug_struct("TraceGuard").field("id", &self.id).finish() } } impl Drop for TraceGuard<'_> { fn drop(&mut self) { - self.trace.pop_guarded(&self.query); + self.trace.pop_guarded(self.id); } } @@ -79,8 +78,8 @@ impl<'a> TraceGuard<'a> { self.trace.active_stack.borrow().len() } - pub fn is_active(&self, key: &QueryKey) -> bool { - self.trace.is_active(key) + pub fn is_active(&self, id: QueryId) -> bool { + self.trace.is_active(id) } pub fn record_event(&self, event: ResolveTraceEvent) { @@ -96,10 +95,15 @@ impl<'a> TraceGuard<'a> { } } -/// Combined active stack and optional trace log. +/// Cycle detection plus optional structured events. +/// +/// The active stack holds [`QueryId`]s rather than keys: it is pushed and +/// popped on every uncached resolve, including with tracing off, so cloning a +/// heap-backed key into it was pure per-frame allocation. Events still carry +/// keys — they are only built when logging is on. pub struct ResolveTrace { log_level: ResolveLogLevel, - active_stack: RefCell>, + active_stack: RefCell>, events: RefCell>, } @@ -132,11 +136,12 @@ impl ResolveTrace { } } - /// Push `query` onto the active stack, or error if it is already active (cycle). - pub fn try_push_active(&self, query: &QueryKey) -> Result<(), ResolveTraceError> { + /// Push `id` onto the active stack, or error if it is already active + /// (cycle). `query` is only used to describe the cycle. + pub fn try_push_active(&self, id: QueryId, query: &QueryKey) -> Result<(), ResolveTraceError> { { let stack = self.active_stack.borrow(); - if stack.contains(query) { + if stack.contains(&id) { drop(stack); self.record_cycle_detected(query); return Err(ResolveTraceError::Cycle { @@ -144,27 +149,31 @@ impl ResolveTrace { }); } } - self.active_stack.borrow_mut().push(query.clone()); + self.active_stack.borrow_mut().push(id); self.record_begin_query(query); Ok(()) } - /// Push `query` if not already active; on success returns a guard that pops on drop. - pub fn enter<'a>(&'a self, query: QueryKey) -> Result, ResolveTraceError> { - self.try_push_active(&query)?; - Ok(TraceGuard { trace: self, query }) + /// Push `id` if not already active; on success returns a guard that pops on drop. + pub fn enter<'a>( + &'a self, + id: QueryId, + query: QueryKey, + ) -> Result, ResolveTraceError> { + self.try_push_active(id, &query)?; + Ok(TraceGuard { trace: self, id }) } - /// Pop `query` if it is the top of the active stack. - pub fn exit(&self, query: &QueryKey) { - self.pop_guarded(query); + /// Pop `id` if it is the top of the active stack. + pub fn exit(&self, id: QueryId) { + self.pop_guarded(id); } - pub fn is_active(&self, query: &QueryKey) -> bool { - self.active_stack.borrow().contains(query) + pub fn is_active(&self, id: QueryId) -> bool { + self.active_stack.borrow().contains(&id) } - pub fn active_stack(&self) -> Vec { + pub fn active_stack(&self) -> Vec { self.active_stack.borrow().clone() } @@ -308,14 +317,10 @@ impl ResolveTrace { self.events.borrow_mut().push(event); } - fn pop_guarded(&self, query: &QueryKey) { + fn pop_guarded(&self, id: QueryId) { let mut stack = self.active_stack.borrow_mut(); - debug_assert_eq!( - stack.last(), - Some(query), - "ResolveTrace guard exit mismatch", - ); - if stack.last() == Some(query) { + debug_assert_eq!(stack.last(), Some(&id), "ResolveTrace guard exit mismatch"); + if stack.last() == Some(&id) { stack.pop(); } } @@ -325,6 +330,7 @@ impl ResolveTrace { mod tests { use super::{ResolveLogLevel, ResolveTrace, ResolveTraceError, ResolveTraceEvent, TraceGuard}; use crate::dataflow::binding::BindingRef; + use crate::dataflow::resolver::query_intern::{QueryId, QueryInternTable}; use crate::dataflow::resolver::query_key::QueryKey; use lpc_model::NodeId; use lpc_model::SlotPath; @@ -336,35 +342,47 @@ mod tests { } } + /// The active stack works in ids, so tests need the same interning the + /// session does. + fn sample_id(table: &mut QueryInternTable, query: &QueryKey) -> QueryId { + table.intern(query) + } + #[test] fn detect_cycle_on_reenter_same_query() { let t = ResolveTrace::new(ResolveLogLevel::Off); let q = sample_key(); - let _g = t.enter(q.clone()).unwrap(); - let err = t.enter(q.clone()).unwrap_err(); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); + let _g = t.enter(id, q.clone()).unwrap(); + let err = t.enter(id, q.clone()).unwrap_err(); assert_eq!(err, ResolveTraceError::Cycle { query: q.clone() }); - assert!(t.is_active(&q)); + assert!(t.is_active(id)); } #[test] fn trace_guard_pops_active_stack() { let t = ResolveTrace::new(ResolveLogLevel::Off); let q = sample_key(); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); { - let g: TraceGuard<'_> = t.enter(q.clone()).unwrap(); + let g: TraceGuard<'_> = t.enter(id, q.clone()).unwrap(); assert_eq!(g.active_stack_len(), 1); - assert!(g.is_active(&q)); + assert!(g.is_active(id)); } assert!(t.active_stack().is_empty()); - assert!(!t.is_active(&q)); + assert!(!t.is_active(id)); } #[test] fn no_events_when_log_off() { let t = ResolveTrace::new(ResolveLogLevel::Off); let q = sample_key(); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); { - let _g = t.enter(q.clone()).unwrap(); + let _g = t.enter(id, q.clone()).unwrap(); drop(_g); } assert!(t.events().is_empty()); @@ -374,8 +392,10 @@ mod tests { fn no_events_when_log_off_including_cycle() { let t = ResolveTrace::new(ResolveLogLevel::Off); let q = sample_key(); - let _g = t.enter(q.clone()).unwrap(); - let _ = t.enter(q.clone()); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); + let _g = t.enter(id, q.clone()).unwrap(); + let _ = t.enter(id, q.clone()); assert!(t.events().is_empty()); } @@ -383,8 +403,10 @@ mod tests { fn basic_level_records_useful_events() { let t = ResolveTrace::new(ResolveLogLevel::Basic); let q = sample_key(); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); { - let g = t.enter(q.clone()).unwrap(); + let g = t.enter(id, q.clone()).unwrap(); g.record_event(ResolveTraceEvent::CacheHit(q.clone())); g.record_event(ResolveTraceEvent::SelectBinding { query: q.clone(), @@ -411,8 +433,10 @@ mod tests { fn cycle_emits_event_when_logging_on() { let t = ResolveTrace::new(ResolveLogLevel::Basic); let q = sample_key(); - let _g = t.enter(q.clone()).unwrap(); - let _ = t.enter(q.clone()); + let mut table = QueryInternTable::new(); + let id = sample_id(&mut table, &q); + let _g = t.enter(id, q.clone()).unwrap(); + let _ = t.enter(id, q.clone()); assert!(t.events().iter().any(|e| matches!( e, ResolveTraceEvent::CycleDetected { query: k } if k == &q diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs index 92ed11da6..af8f2d4f0 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs @@ -23,6 +23,8 @@ //! frames is a change of what each one *does*, not a hunt for which callers //! meant which. +use crate::dataflow::resolver::query_intern::{QueryId, QueryInternTable}; +use crate::dataflow::resolver::query_key::QueryKey; use crate::dataflow::resolver::resolve_error::ResolveError; use crate::dataflow::resolver::resolver_cache::ResolverCache; use lpc_model::LpValue; @@ -65,22 +67,24 @@ impl ResolveFrameCounters { } } -/// Owns the [`ResolverCache`] for engine demand resolution. +/// Owns the [`ResolverCache`] and the query intern table. #[derive(Clone, Debug, Default)] pub struct Resolver { cache: ResolverCache, + intern: QueryInternTable, structure_epoch: u64, frame_counters: ResolveFrameCounters, - last_frame_counters: ResolveFrameCounters, + force_invalidate_per_frame: bool, } impl Resolver { pub fn new() -> Self { Self { cache: ResolverCache::new(), + intern: QueryInternTable::new(), structure_epoch: 0, frame_counters: ResolveFrameCounters::default(), - last_frame_counters: ResolveFrameCounters::default(), + force_invalidate_per_frame: false, } } @@ -92,17 +96,42 @@ impl Resolver { &mut self.cache } + pub fn intern(&self) -> &QueryInternTable { + &self.intern + } + + /// Id for `query`, assigning one on first use this epoch. + pub fn intern_query(&mut self, query: &QueryKey) -> QueryId { + self.intern.intern(query) + } + /// Start a new frame: per-frame resolved values are discarded. pub fn begin_frame(&mut self) { - self.last_frame_counters = self.frame_counters; self.frame_counters = ResolveFrameCounters::default(); - self.cache.clear(); + if self.force_invalidate_per_frame { + self.invalidate_structure(); + return; + } + self.cache.begin_frame(); } /// The graph changed shape: every cached decision and value is suspect. pub fn invalidate_structure(&mut self) { self.structure_epoch = self.structure_epoch.wrapping_add(1); - self.cache.clear(); + self.cache.invalidate_structure(); + self.intern.clear(); + } + + /// Throw away *all* resolution every frame, as if the graph changed shape + /// each tick — the behaviour that predates cross-frame persistence. + /// + /// This exists so a test can run the same scene twice, cached and + /// uncached, and demand identical output. That differential is the only + /// check that reliably catches a stale cache: a wrong cached answer is + /// still a *plausible* answer, so it survives assertions written against + /// what the code does rather than against what it should do. + pub fn set_force_invalidate_per_frame(&mut self, force: bool) { + self.force_invalidate_per_frame = force; } /// How many times the graph has changed shape. Only equality across two @@ -111,17 +140,13 @@ impl Resolver { self.structure_epoch } - /// Counters for the frame currently being resolved. + /// Resolution work done for the current frame. Counting resets when the + /// next frame begins, so after a tick returns this reads as "what that + /// tick cost". pub fn frame_counters(&self) -> &ResolveFrameCounters { &self.frame_counters } - /// Counters for the last frame that completed — readable after the tick - /// returns, which is when tests can look at them. - pub fn last_frame_counters(&self) -> &ResolveFrameCounters { - &self.last_frame_counters - } - pub(crate) fn counters_mut(&mut self) -> &mut ResolveFrameCounters { &mut self.frame_counters } diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolver_cache.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolver_cache.rs index 9ef05957c..b44bb2686 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolver_cache.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolver_cache.rs @@ -1,75 +1,128 @@ -//! Engine-level same-frame resolver cache keyed by [`super::QueryKey`]. - -use crate::dataflow::resolver::production::Production; -use crate::dataflow::resolver::query_key::QueryKey; +//! Engine resolution cache: per-frame values over persistent structure. +//! +//! Three tables, all indexed by [`QueryId`], with different lifetimes: +//! +//! - **frame values** — what a query resolved to *this frame*. Discarded each +//! frame, because producers tick and time moves. +//! - **structural values** — what a query resolves to until the graph changes: +//! binding literals and authored-definition reads. A def read is a deep copy +//! of slot data, so re-doing it every frame was one of the larger costs. +//! - **routes** — [`ResolvedRoute`], the decision about *how* a query is +//! answered. See that type's documentation. +//! +//! Discarding frame values does not clear the table. Each entry carries the +//! frame it was written for, and a new frame simply stops matching — so a +//! steady scene neither frees nor reallocates its entries, it overwrites them +//! in place. That matters more than it looks: dropping every entry each frame +//! was most of the allocator traffic this cache exists to avoid. + +use alloc::rc::Rc; use alloc::vec::Vec; -/// Per-frame cache of [`Production`] entries addressed by [`QueryKey`]. -/// -/// Resolver caches are small in normal scenes, and they are rebuilt every frame. -/// A linear vec avoids per-entry tree allocation and pointer chasing on embedded -/// targets. +use crate::dataflow::resolver::production::{Production, ProductionSource}; +use crate::dataflow::resolver::query_intern::QueryId; +use crate::dataflow::resolver::route::ResolvedRoute; + +/// Monotonic per-frame stamp. Wrapping is harmless: entries are rewritten far +/// more often than `u32` wraps, and a stale entry would have to survive +/// exactly 2^32 frames to be mistaken for a current one. +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] +struct FrameStamp(u32); + #[derive(Clone, Debug, Default)] pub struct ResolverCache { - entries: Vec<(QueryKey, Production)>, + frame: FrameStamp, + values: Vec>, + structural: Vec>, + routes: Vec>>, } impl ResolverCache { pub fn new() -> Self { - Self { - entries: Vec::new(), - } + Self::default() } - pub fn get(&self, key: &QueryKey) -> Option<&Production> { - self.entries - .iter() - .find_map(|(entry_key, value)| (entry_key == key).then_some(value)) + /// Whether a production may outlive the frame that computed it. + /// + /// A binding literal and an authored-def read are both functions of the + /// project, not of the frame — nothing but a structural change can move + /// them. Everything else either ticks (producers) or is built from things + /// that tick (merges, values reached through a binding). + pub fn is_structural(source: &ProductionSource) -> bool { + matches!( + source, + ProductionSource::Literal | ProductionSource::Default + ) } - pub fn insert(&mut self, key: QueryKey, value: Production) -> Option { - if let Some((_, current)) = self - .entries - .iter_mut() - .find(|(entry_key, _)| entry_key == &key) - { - return Some(core::mem::replace(current, value)); - } - self.entries.push((key, value)); - None + /// Begin a frame: this frame's values start empty, structure survives. + pub fn begin_frame(&mut self) { + self.frame = FrameStamp(self.frame.0.wrapping_add(1)); + } + + /// The graph changed shape: everything cached is void. + pub fn invalidate_structure(&mut self) { + self.values.clear(); + self.structural.clear(); + self.routes.clear(); + // Ids are reassigned from zero after an epoch change, so the tables + // must not keep entries addressed by the old numbering. } - pub fn remove(&mut self, key: &QueryKey) -> Option { - let index = self - .entries - .iter() - .position(|(entry_key, _)| entry_key == key)?; - Some(self.entries.swap_remove(index).1) + pub fn get(&self, id: QueryId) -> Option<&Production> { + if let Some(Some((stamp, production))) = self.values.get(id.index()) { + if *stamp == self.frame { + return Some(production); + } + } + self.structural.get(id.index()).and_then(Option::as_ref) } - pub fn clear(&mut self) { - self.entries.clear(); + pub fn insert(&mut self, id: QueryId, production: Production) { + if Self::is_structural(&production.source) { + grow_to(&mut self.structural, id.index()); + self.structural[id.index()] = Some(production); + return; + } + grow_to(&mut self.values, id.index()); + self.values[id.index()] = Some((self.frame, production)); } - pub fn iter(&self) -> core::slice::Iter<'_, (QueryKey, Production)> { - self.entries.iter() + /// Routes are shared rather than copied: a cache hit on the hot path must + /// not deep-copy the binding sources it just avoided recomputing. + pub fn route(&self, id: QueryId) -> Option<&Rc> { + self.routes.get(id.index()).and_then(Option::as_ref) } - pub fn len(&self) -> usize { - self.entries.len() + pub fn insert_route(&mut self, id: QueryId, route: Rc) { + grow_to(&mut self.routes, id.index()); + self.routes[id.index()] = Some(route); } + /// Whether anything is cached for the current frame. Used by tests that + /// assert a fresh resolver starts empty. pub fn is_empty(&self) -> bool { - self.entries.is_empty() + self.structural.iter().all(Option::is_none) + && self + .values + .iter() + .all(|slot| !matches!(slot, Some((stamp, _)) if *stamp == self.frame)) + } +} + +fn grow_to(table: &mut Vec>, index: usize) { + if table.len() <= index { + table.resize_with(index + 1, || None); } } #[cfg(test)] mod tests { - use super::{Production, QueryKey, ResolverCache}; + use super::*; use crate::dataflow::binding::BindingRef; - use crate::dataflow::resolver::production::ProductionSource; - use crate::dataflow::resolver::{ResolveLogLevel, ResolveTrace, ResolveTraceEvent}; + use crate::dataflow::resolver::query_intern::QueryInternTable; + use crate::dataflow::resolver::query_key::QueryKey; + use alloc::string::String; use lpc_model::ChannelName; use lpc_model::NodeId; use lpc_model::Revision; @@ -77,68 +130,114 @@ mod tests { use lpc_model::WithRevision; use lps_shared::LpsValueF32; - fn sample_bus_key(name: &str) -> QueryKey { - QueryKey::Bus(ChannelName(alloc::string::String::from(name))) - } - - fn make_produced(frame: i64, source: ProductionSource) -> Production { + fn produced(value: f32, source: ProductionSource) -> Production { Production::value( - WithRevision::new(Revision::new(frame), LpsValueF32::F32(1.0)), + WithRevision::new(Revision::new(1), LpsValueF32::F32(value)), source, ) .expect("scalar production") } + fn producer_source() -> ProductionSource { + ProductionSource::ProducedSlot { + node: NodeId::new(0), + slot: SlotPath::parse("out").expect("path"), + } + } + + fn id(table: &mut QueryInternTable, name: &str) -> QueryId { + table.intern(&QueryKey::Bus(ChannelName(String::from(name)))) + } + #[test] - fn resolver_cache_insert_get_and_cache_hit_trace() { + fn frame_values_do_not_survive_the_frame() { + let mut table = QueryInternTable::new(); let mut cache = ResolverCache::new(); - let key = sample_bus_key("video"); - let pv = make_produced( - 1, - ProductionSource::BusBinding { - binding: BindingRef::new(NodeId::new(0), 0), - }, + let key = id(&mut table, "video"); + + cache.insert(key, produced(1.0, producer_source())); + assert!(cache.get(key).is_some()); + + cache.begin_frame(); + assert!( + cache.get(key).is_none(), + "a producer's value belongs to the frame that produced it" ); + } - assert!(cache.insert(key.clone(), pv.clone()).is_none()); - let got = cache.get(&key).unwrap(); - assert!(got.as_value().expect("value").eq(&LpsValueF32::F32(1.0))); - assert_eq!( - got.source, - ProductionSource::BusBinding { + #[test] + fn structural_values_survive_frames_but_not_invalidation() { + let mut table = QueryInternTable::new(); + let mut cache = ResolverCache::new(); + let key = id(&mut table, "literal"); + + cache.insert(key, produced(2.0, ProductionSource::Literal)); + cache.begin_frame(); + cache.begin_frame(); + assert!( + cache.get(key).is_some(), + "a binding literal cannot change without a structural change" + ); + + cache.invalidate_structure(); + assert!(cache.get(key).is_none()); + } + + #[test] + fn authored_def_reads_are_structural() { + assert!(ResolverCache::is_structural(&ProductionSource::Default)); + assert!(ResolverCache::is_structural(&ProductionSource::Literal)); + assert!(!ResolverCache::is_structural(&ProductionSource::Merged)); + assert!(!ResolverCache::is_structural(&producer_source())); + assert!(!ResolverCache::is_structural( + &ProductionSource::BusBinding { binding: BindingRef::new(NodeId::new(0), 0), } + )); + } + + #[test] + fn routes_survive_frames_but_not_invalidation() { + let mut table = QueryInternTable::new(); + let mut cache = ResolverCache::new(); + let key = id(&mut table, "routed"); + + cache.insert_route(key, Rc::new(ResolvedRoute::Produce)); + cache.begin_frame(); + assert!( + matches!(cache.route(key).map(|r| &**r), Some(ResolvedRoute::Produce)), + "which binding answers a query changes only when bindings change" ); - let trace = ResolveTrace::new(ResolveLogLevel::Basic); - { - let _g = trace.enter(key.clone()).unwrap(); - let _hit = cache.get(&key); - assert!(_hit.is_some()); - trace.record_event(ResolveTraceEvent::CacheHit(key.clone())); - } + cache.invalidate_structure(); + assert!(cache.route(key).is_none()); + } + + #[test] + fn a_later_frame_overwrites_rather_than_reallocating() { + let mut table = QueryInternTable::new(); + let mut cache = ResolverCache::new(); + let key = id(&mut table, "video"); + + cache.insert(key, produced(1.0, producer_source())); + cache.begin_frame(); + cache.insert(key, produced(3.0, producer_source())); - assert!(trace.events().iter().any(|e| matches!( - e, - ResolveTraceEvent::CacheHit(k) if k == &sample_bus_key("video") - ))); + let value = cache.get(key).expect("current frame value"); + assert!(value.as_value().expect("value").eq(&LpsValueF32::F32(3.0))); + assert_eq!(cache.values.len(), 1, "the table is reused, not regrown"); } #[test] - fn resolver_cache_remove_clear_len() { + fn sparse_ids_do_not_read_neighbouring_entries() { let mut cache = ResolverCache::new(); - let k = QueryKey::ProducedSlot { - node: NodeId::new(2), - slot: SlotPath::parse("color").unwrap(), - }; - cache.insert(k.clone(), make_produced(3, ProductionSource::Literal)); - assert_eq!(cache.len(), 1); - - cache.remove(&k); - assert!(cache.is_empty()); - - cache.insert(k.clone(), make_produced(4, ProductionSource::Default)); - cache.clear(); - assert_eq!(cache.len(), 0); + let mut table = QueryInternTable::new(); + let low = id(&mut table, "a"); + let _skipped = id(&mut table, "b"); + let high = id(&mut table, "c"); + + cache.insert(high, produced(5.0, producer_source())); + assert!(cache.get(low).is_none()); + assert!(cache.get(high).is_some()); } } diff --git a/lp-core/lpc-engine/src/dataflow/resolver/route.rs b/lp-core/lpc-engine/src/dataflow/resolver/route.rs new file mode 100644 index 000000000..b964d1e54 --- /dev/null +++ b/lp-core/lpc-engine/src/dataflow/resolver/route.rs @@ -0,0 +1,35 @@ +//! How a query reaches its value — the part that only a structural change +//! can alter. +//! +//! Resolving a consumed slot or a bus channel is two jobs stacked together: +//! *decide* which binding answers it (owner depth, priority, merge policy — +//! all reads of the binding graph and of authored definitions), then *get* +//! the value through that decision (which may tick a producer, and must +//! happen every frame). +//! +//! Only the second job is per-frame. A [`ResolvedRoute`] is the first job's +//! answer, cached until the graph changes shape. + +use alloc::vec::Vec; + +use crate::dataflow::binding::{BindingRef, BindingSource}; + +/// The decision about how one query is answered. +#[derive(Clone, Debug)] +pub enum ResolvedRoute { + /// Resolve through this binding's source. + Binding { + binding_ref: BindingRef, + source: BindingSource, + }, + /// No binding answers this query; the host produces it. + Produce, + /// A mergeable receiver: merge these sources by key, in this order. + /// + /// Bus providers are already expanded into leaves here — the recursive + /// walk that flattens them reads only the binding graph, so it is part of + /// the decision rather than part of the frame. + MergeByKey { + inputs: Vec<(BindingRef, BindingSource)>, + }, +} diff --git a/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs b/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs index 8e403d8fb..2776abdd2 100644 --- a/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs +++ b/lp-core/lpc-engine/src/engine/resolution_persistence_tests.rs @@ -14,6 +14,7 @@ use super::test_support::{EngineTestBuilder, bus, literal, output, produced_slot}; use crate::dataflow::binding::{BindingDraft, BindingPriority, BindingSource, BindingTarget}; +use alloc::vec::Vec; use lpc_model::{Kind, LpValue, Revision, SlotPath}; fn path(p: &str) -> SlotPath { @@ -190,12 +191,17 @@ fn producers_still_tick_every_frame_on_an_unchanged_graph() { ); } -/// Steady-state frames over an unchanged graph do the same amount of work. +/// A steady frame over an unchanged graph does *no* structural work. /// -/// This is the weaker, always-true form of the zero-structural-work assertion: -/// whatever a frame costs, it must not grow frame over frame. +/// This is the whole point of the change, stated as an assertion: once the +/// graph has been resolved, a frame walks no binding indexes, reads no merge +/// policies, and re-reads no authored definitions. It only runs producers. +/// +/// If this test starts failing, resolution has gone back to re-deriving the +/// graph every tick — which costs about 8 ms per fixture chain on an S3 and +/// is invisible to every other test in the suite. #[test] -fn steady_state_frames_report_stable_counters() { +fn steady_state_frame_does_zero_structural_work() { let mut harness = EngineTestBuilder::new() .shader("src", output("outputs[0]", 1.5)) .output_node("out") @@ -204,14 +210,65 @@ fn steady_state_frames_report_stable_counters() { .build(); harness.tick(16).expect("warm-up tick"); - harness.tick(16).expect("second tick"); - let second = *harness.engine.resolver().last_frame_counters(); - harness.tick(16).expect("third tick"); - let third = *harness.engine.resolver().last_frame_counters(); + let warm = *harness.engine.resolver().frame_counters(); + assert!( + warm.structural_work() > 0, + "the first frame must actually resolve the graph" + ); + + harness.tick(16).expect("steady tick"); + let steady = *harness.engine.resolver().frame_counters(); + + assert_eq!( + steady.structural_work(), + 0, + "a steady frame re-derived the graph it already knew: {steady:?}" + ); + assert!( + steady.producer_produces > 0, + "producers must still run every frame" + ); +} + +/// After a structural change, the graph is re-resolved exactly once. +#[test] +fn structural_change_costs_one_frame_of_re_resolution() { + let mut harness = EngineTestBuilder::new() + .shader("src", output("outputs[0]", 1.0)) + .output_node("out") + .bind_demand_input("out", produced_slot("src", "outputs[0]")) + .demand_root("out") + .build(); + + harness.tick(16).expect("warm-up"); + harness.tick(16).expect("steady"); + assert_eq!( + harness.engine.resolver().frame_counters().structural_work(), + 0 + ); + let src = harness.node("src"); + harness + .engine + .reattach_runtime_node( + src, + super::test_support::dummy_shader_node(path("outputs[0]"), 6.0), + Revision::new(9), + ) + .expect("reattach"); + + harness.tick(16).expect("frame after the change"); + assert!( + harness.engine.resolver().frame_counters().structural_work() > 0, + "the frame after a structural change must resolve the graph again" + ); + assert_eq!(harness.output_f32("out"), Some(6.0)); + + harness.tick(16).expect("back to steady"); assert_eq!( - second, third, - "two steady frames over an unchanged graph must cost the same resolution work" + harness.engine.resolver().frame_counters().structural_work(), + 0, + "and then go quiet again" ); } @@ -273,3 +330,92 @@ fn structural_mutations_bump_the_epoch() { "reattaching a node is a structural change" ); } + +/// The same scene, resolved cached and uncached, must produce the same values. +/// +/// Every other test here asserts a specific thing that must not go stale. This +/// one asserts the general case: run a scene that binds, re-binds, switches +/// producers and reattaches nodes, once with resolution persisting and once +/// with it thrown away every frame, and demand the two agree frame for frame. +/// +/// It is the only check that catches a stale cache without someone having +/// first thought of the particular way it could go stale — which matters, +/// because a wrong cached answer is a *plausible* answer, and plausible +/// answers survive assertions written by whoever wrote the cache. +#[test] +fn cached_and_uncached_resolution_agree_frame_for_frame() { + fn run(force_uncached: bool) -> Vec<(Option, Option)> { + let mut harness = EngineTestBuilder::new() + .shader("a", output("outputs[0]", 3.0)) + .shader("b", output("outputs[0]", 4.0)) + .selector("sel", &[("a", "outputs[0]"), ("b", "outputs[0]")]) + .output_node("out") + .bind_demand_input("out", literal(1.0)) + .demand_root("sel") + .demand_root("out") + .build(); + harness + .engine + .resolver_mut() + .set_force_invalidate_per_frame(force_uncached); + + let out = harness.node("out"); + let mut observed = Vec::new(); + for frame in 0..8u32 { + match frame { + // A playlist-style switch: runtime state only. + 2 => harness.select("sel", 1), + // A re-bind: the binding set is replaced, as an apply does. + 4 => { + harness + .engine + .clear_bindings(Revision::new(100 + frame as i64)); + harness + .engine + .add_binding( + BindingDraft { + source: BindingSource::Literal(LpValue::F32(5.0)), + target: BindingTarget::ConsumedSlot { + node: out, + slot: path("in"), + }, + priority: BindingPriority::new(0), + kind: Kind::Color, + owner: out, + }, + Revision::new(100 + frame as i64), + ) + .expect("rebind"); + } + // A node replacement under a consumer that is mid-run. + 6 => { + let b = harness.node("b"); + harness + .engine + .reattach_runtime_node( + b, + super::test_support::dummy_shader_node(path("outputs[0]"), 8.0), + Revision::new(100 + frame as i64), + ) + .expect("reattach"); + } + _ => {} + } + harness.tick(16).expect("tick"); + observed.push((harness.output_f32("sel"), harness.output_f32("out"))); + } + observed + } + + let cached = run(false); + let uncached = run(true); + assert_eq!( + cached, uncached, + "persisting resolution changed what the engine resolves to" + ); + // Guard against the test passing because nothing ever changed. + assert!( + cached.windows(2).any(|w| w[0] != w[1]), + "the scenario must actually move, or agreement proves nothing" + ); +} From 49e4d16211404805b0fbbebe06fafeeda441f6f0 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:32:13 -0700 Subject: [PATCH 4/9] perf(lpc-engine): stop rebuilding query keys the caller already had MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three sources of per-frame key churn, all downstream of P3's routes: - `TickResolver::resolve` took its key by value, so every caller cloned a `SlotPath` into a fresh `QueryKey` per call and dropped it again. It now borrows. - Routes stored a `BindingSource`, so following one rebuilt the target key and re-hashed it. They now store the target's `QueryId` — ids and routes die together on a structural change, so the id can simply be part of the decision. - Nodes read authored slots by constant path (`power.some`, `diagnostic_mode`, `input`), parsing those strings every frame. The resolver memoizes them, since it already has exactly the right lifetime. Interned keys move behind `Rc` so resolution can hold one while mutating the resolver around it, which is what lets route following work from an id alone. 1-fixture profile, cumulative against the P1 baseline: total cycles 2,468,427 -> 1,146,190 (-54%) alloc+memcpy 44.0% -> 34.4% [jit] render 1.1% -> 2.4% (same cycles, less than half the frame) Two measured follow-ups, both left alone deliberately: - `SlotPath::parse` (2.7%) is now almost entirely the shader node re-reading its consumed-slot *definitions* every frame through `format!`-built paths — the same re-derive-what-only-changes-structurally shape as this plan, but in shader config sync rather than in the resolver. Out of scope here, and it owns its own change-detection correctness. - A cache hit still clones `ProductionSource`, which carries a `SlotPath`. Cheap per hit, not free. Plan: 2026-07-31-2225-persist-dataflow-resolution --- .../src/dataflow/resolver/query_intern.rs | 18 ++- .../src/dataflow/resolver/resolve_session.rs | 120 ++++++++++++------ .../src/dataflow/resolver/resolver.rs | 31 +++++ .../lpc-engine/src/dataflow/resolver/route.rs | 26 +++- .../src/dataflow/resolver/tick_resolver.rs | 24 +++- lp-core/lpc-engine/src/engine/engine.rs | 10 +- lp-core/lpc-engine/src/engine/test_support.rs | 6 +- lp-core/lpc-engine/src/node/contexts.rs | 27 +++- .../src/nodes/fixture/fixture_node.rs | 29 ++--- .../lpc-engine/src/nodes/fluid/fluid_node.rs | 2 +- .../src/nodes/output/output_node.rs | 2 +- .../src/nodes/playlist/playlist_node.rs | 4 +- .../src/nodes/radio/control_radio_node.rs | 2 +- .../src/nodes/shader/compute_shader_node.rs | 2 +- .../src/nodes/shader/shader_node.rs | 6 +- lp-core/lpc-engine/tests/runtime_spine.rs | 2 +- 16 files changed, 218 insertions(+), 93 deletions(-) diff --git a/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs b/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs index 0c1385d1c..9447f1768 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/query_intern.rs @@ -12,6 +12,7 @@ //! Ids are meaningful only within one epoch. [`QueryInternTable::clear`] runs //! when the graph changes shape, and every id handed out before it is void. +use alloc::rc::Rc; use alloc::vec::Vec; use core::hash::{Hash, Hasher}; use lpc_model::SlotPath; @@ -92,7 +93,10 @@ fn hash_slot_path(path: &SlotPath, hasher: &mut impl Hasher) { #[derive(Clone, Debug, Default)] pub struct QueryInternTable { /// Indexed by [`QueryId`]: the key it stands for. - keys: Vec, + /// + /// Shared rather than owned, so that resolution can hold on to a key while + /// mutating the resolver around it without copying heap data. + keys: Vec>, /// `(hash, id)` sorted by hash, so lookup is a binary search plus one /// equality check in the common case. by_hash: Vec<(u64, QueryId)>, @@ -111,13 +115,13 @@ impl QueryInternTable { if *candidate_hash != hash { break; } - if &self.keys[id.index()] == query { + if &*self.keys[id.index()] == query { return *id; } } let id = QueryId(self.keys.len() as u32); - self.keys.push(query.clone()); + self.keys.push(Rc::new(query.clone())); self.by_hash.insert(start, (hash, id)); id } @@ -129,12 +133,12 @@ impl QueryInternTable { self.by_hash[start..] .iter() .take_while(|(candidate_hash, _)| *candidate_hash == hash) - .find(|(_, id)| &self.keys[id.index()] == query) + .find(|(_, id)| &*self.keys[id.index()] == query) .map(|(_, id)| *id) } - /// The key an id stands for. Cold path: error formatting and tracing. - pub fn key(&self, id: QueryId) -> Option<&QueryKey> { + /// The key an id stands for. + pub fn key(&self, id: QueryId) -> Option<&Rc> { self.keys.get(id.index()) } @@ -179,7 +183,7 @@ mod tests { assert_eq!(a, a_again, "the same key must intern to the same id"); assert_ne!(a, b); assert_eq!(table.len(), 2, "re-interning must not grow the table"); - assert_eq!(table.key(a), Some(&bus("video"))); + assert_eq!(table.key(a).map(|k| &**k), Some(&bus("video"))); } #[test] diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs index eee2a3297..29ad7cbd6 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolve_session.rs @@ -14,7 +14,7 @@ use crate::dataflow::resolver::resolve_host::ResolveHost; use crate::dataflow::resolver::resolve_trace::ResolveTrace; use crate::dataflow::resolver::resolver::Resolver; use crate::dataflow::resolver::resolver::materialize_literal_product; -use crate::dataflow::resolver::route::ResolvedRoute; +use crate::dataflow::resolver::route::{ResolvedRoute, RouteTarget}; use lpc_model::{ChannelName, NodeId, Revision, SlotData, SlotMapDyn, SlotMerge, SlotPath}; /// Active engine session for one frame (or nested test scope). @@ -60,13 +60,48 @@ impl<'a> EngineSession<'a> { } /// Demand-resolve `query` (cache + cycle stack + host-owned bindings). + /// + /// Takes a reference so a caller that resolves the same slot every frame + /// can hold one key and lend it, rather than rebuilding a heap-backed key + /// per call. pub fn resolve( &mut self, host: &mut H, - query: QueryKey, + query: &QueryKey, ) -> Result { - let id = self.resolver.intern_query(&query); - self.resolve_interned(host, id, &query) + let id = self.resolver.intern_query(query); + self.resolve_interned(host, id, query) + } + + /// Resolve `node`'s consumed slot at a compile-time-constant path, + /// parsing that path at most once per structural epoch. + pub fn resolve_static_consumed( + &mut self, + host: &mut H, + node: NodeId, + path: &'static str, + ) -> Result { + let id = self + .resolver + .intern_static_consumed(node, path) + .map_err(|e| { + SessionResolveError::other(format!("invalid authored path {path:?}: {e}")) + })?; + self.resolve_id(host, id) + } + + /// Resolve an already-interned query, fetching its key from the intern + /// table. Used by route following, where the id is all that was stored. + fn resolve_id( + &mut self, + host: &mut H, + id: QueryId, + ) -> Result { + let key = + self.resolver.intern().key(id).cloned().ok_or_else(|| { + SessionResolveError::other("route referenced an unknown query id") + })?; + self.resolve_interned(host, id, &key) } fn resolve_interned( @@ -158,9 +193,10 @@ impl<'a> EngineSession<'a> { self.resolver.counters_mut().binding_lookups += 1; let candidates = host.providers_for_bus(channel); let (binding_ref, entry) = select_highest_priority_bus_provider(channel, &candidates)?; + let target = self.route_target(&entry.source); Ok(ResolvedRoute::Binding { binding_ref, - source: entry.source, + target, }) } @@ -210,12 +246,29 @@ impl<'a> EngineSession<'a> { Ok(match host.binding_for_consumed_slot(node, slot) { Some((binding_ref, entry)) => ResolvedRoute::Binding { binding_ref, - source: entry.source, + target: self.route_target(&entry.source), }, None => ResolvedRoute::Produce, }) } + /// Reduce a binding source to what the frame needs: a literal to + /// materialize, or the id of the query to resolve. + fn route_target(&mut self, source: &BindingSource) -> RouteTarget { + match source { + BindingSource::Literal(spec) => RouteTarget::Literal(spec.clone()), + BindingSource::ProducedSlot { node, slot } => { + RouteTarget::Query(self.resolver.intern_query(&QueryKey::ProducedSlot { + node: *node, + slot: slot.clone(), + })) + } + BindingSource::BusChannel(channel) => { + RouteTarget::Query(self.resolver.intern_query(&QueryKey::Bus(channel.clone()))) + } + } + } + /// Flatten a mergeable receiver's bindings into leaf sources. /// /// A bus source contributes all of its providers, in priority order, and @@ -226,10 +279,11 @@ impl<'a> EngineSession<'a> { host: &mut H, binding_ref: BindingRef, source: &BindingSource, - out: &mut Vec<(BindingRef, BindingSource)>, + out: &mut Vec<(BindingRef, RouteTarget)>, ) -> Result<(), SessionResolveError> { let BindingSource::BusChannel(channel) = source else { - out.push((binding_ref, source.clone())); + let target = self.route_target(source); + out.push((binding_ref, target)); return Ok(()); }; @@ -260,17 +314,17 @@ impl<'a> EngineSession<'a> { match route { ResolvedRoute::Binding { binding_ref, - source, + target, } => { self.trace.record_select_binding(query, *binding_ref); - self.resolve_binding_source(host, *binding_ref, source) + self.resolve_route_target(host, *binding_ref, target) } ResolvedRoute::Produce => self.produce_through_host(host, query), ResolvedRoute::MergeByKey { inputs } => { let mut productions = Vec::with_capacity(inputs.len()); - for (binding_ref, source) in inputs.iter() { + for (binding_ref, target) in inputs.iter() { self.trace.record_merge_input(query, *binding_ref); - let mut production = self.resolve_binding_source(host, *binding_ref, source)?; + let mut production = self.resolve_route_target(host, *binding_ref, target)?; production.source = ProductionSource::BusBinding { binding: *binding_ref, }; @@ -306,32 +360,20 @@ impl<'a> EngineSession<'a> { r } - fn resolve_binding_source( + fn resolve_route_target( &mut self, host: &mut H, binding_ref: BindingRef, - source: &BindingSource, + target: &RouteTarget, ) -> Result { - match source { - BindingSource::Literal(spec) => { + match target { + RouteTarget::Literal(spec) => { self.resolver.counters_mut().literal_materializations += 1; let product = materialize_literal_product(spec, self.revision); Ok(Production::leaf(product, ProductionSource::Literal)) } - BindingSource::ProducedSlot { node, slot } => { - let key = QueryKey::ProducedSlot { - node: *node, - slot: slot.clone(), - }; - let mut pv = self.resolve(host, key)?; - pv.source = ProductionSource::BusBinding { - binding: binding_ref, - }; - Ok(pv) - } - BindingSource::BusChannel(other) => { - let key = QueryKey::Bus(other.clone()); - let mut pv = self.resolve(host, key)?; + RouteTarget::Query(id) => { + let mut pv = self.resolve_id(host, *id)?; pv.source = ProductionSource::BusBinding { binding: binding_ref, }; @@ -561,8 +603,8 @@ mod tests { &mut resolver, ResolveTrace::new(ResolveLogLevel::Off), ); - let a = session.resolve(&mut host, key.clone()).unwrap(); - let b = session.resolve(&mut host, key).unwrap(); + let a = session.resolve(&mut host, &key).unwrap(); + let b = session.resolve(&mut host, &key).unwrap(); assert!(a.as_value().expect("value").eq(&LpsValueF32::F32(42.0))); assert!(b.as_value().expect("value").eq(&LpsValueF32::F32(42.0))); assert!( @@ -613,7 +655,7 @@ mod tests { ResolveTrace::new(ResolveLogLevel::Off), ); let pv = session - .resolve(&mut host, QueryKey::Bus(c)) + .resolve(&mut host, &QueryKey::Bus(c)) .expect("resolve bus"); assert!(pv.as_value().expect("value").eq(&LpsValueF32::F32(9.0))); assert_eq!(host.produce_calls, 0); @@ -687,7 +729,7 @@ mod tests { ResolveTrace::new(ResolveLogLevel::Off), ); let pv = session - .resolve(&mut host, QueryKey::Bus(outer)) + .resolve(&mut host, &QueryKey::Bus(outer)) .expect("bus chain"); assert!(pv.as_value().expect("value").eq(&LpsValueF32::F32(3.25))); } @@ -747,7 +789,7 @@ mod tests { ResolveTrace::new(ResolveLogLevel::Off), ); let err = session - .resolve(&mut host, QueryKey::Bus(a)) + .resolve(&mut host, &QueryKey::Bus(a)) .expect_err("cycle"); assert!(matches!(err, SessionResolveError::Cycle { .. })); } @@ -880,7 +922,7 @@ mod tests { let production = session .resolve( &mut host, - QueryKey::ConsumedSlot { + &QueryKey::ConsumedSlot { node: receiver, slot: receiver_slot, }, @@ -953,7 +995,7 @@ mod tests { let production = session .resolve( &mut host, - QueryKey::ConsumedSlot { + &QueryKey::ConsumedSlot { node: receiver, slot: receiver_slot, }, @@ -1021,10 +1063,10 @@ mod tests { let mut host = TraceHost { node, bindings }; let mut session = ResolveSession::new(frame, &mut resolver, trace); session - .resolve(&mut host, QueryKey::Bus(bus.clone())) + .resolve(&mut host, &QueryKey::Bus(bus.clone())) .unwrap(); // Second resolve — cache hit on bus - session.resolve(&mut host, QueryKey::Bus(bus)).unwrap(); + session.resolve(&mut host, &QueryKey::Bus(bus)).unwrap(); let evs = session.trace().events(); assert!(evs.iter().any(|e| { diff --git a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs index af8f2d4f0..e505ff3ee 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/resolver.rs @@ -27,8 +27,12 @@ use crate::dataflow::resolver::query_intern::{QueryId, QueryInternTable}; use crate::dataflow::resolver::query_key::QueryKey; use crate::dataflow::resolver::resolve_error::ResolveError; use crate::dataflow::resolver::resolver_cache::ResolverCache; +use lp_collection::VecMap; use lpc_model::LpValue; +use lpc_model::NodeId; use lpc_model::Revision; +use lpc_model::SlotPath; +use lpc_model::SlotPathError; use lpc_model::WithRevision; use lps_shared::LpsValueF32; @@ -75,6 +79,15 @@ pub struct Resolver { structure_epoch: u64, frame_counters: ResolveFrameCounters, force_invalidate_per_frame: bool, + /// `(node, authored path)` → the consumed-slot query it names. + /// + /// Nodes read a handful of authored slots by constant path every frame + /// (`power.some`, `diagnostic_mode`, each shader input). Parsing those + /// strings per frame allocated a `SlotPath` per read, only to drop it + /// again — one of the last per-frame allocation sources after routes and + /// values stopped being rebuilt. Lives here because it shares the intern + /// table's lifetime exactly. + static_paths: VecMap<(NodeId, &'static str), QueryId>, } impl Resolver { @@ -85,6 +98,7 @@ impl Resolver { structure_epoch: 0, frame_counters: ResolveFrameCounters::default(), force_invalidate_per_frame: false, + static_paths: VecMap::new(), } } @@ -105,6 +119,22 @@ impl Resolver { self.intern.intern(query) } + /// Id for `node`'s consumed slot at the compile-time-constant `path`, + /// parsing the path at most once per epoch. + pub fn intern_static_consumed( + &mut self, + node: NodeId, + path: &'static str, + ) -> Result { + if let Some(id) = self.static_paths.get(&(node, path)) { + return Ok(*id); + } + let slot = SlotPath::parse(path)?; + let id = self.intern.intern(&QueryKey::ConsumedSlot { node, slot }); + self.static_paths.insert((node, path), id); + Ok(id) + } + /// Start a new frame: per-frame resolved values are discarded. pub fn begin_frame(&mut self) { self.frame_counters = ResolveFrameCounters::default(); @@ -120,6 +150,7 @@ impl Resolver { self.structure_epoch = self.structure_epoch.wrapping_add(1); self.cache.invalidate_structure(); self.intern.clear(); + self.static_paths.clear(); } /// Throw away *all* resolution every frame, as if the graph changed shape diff --git a/lp-core/lpc-engine/src/dataflow/resolver/route.rs b/lp-core/lpc-engine/src/dataflow/resolver/route.rs index b964d1e54..2a676088f 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/route.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/route.rs @@ -11,16 +11,34 @@ //! answer, cached until the graph changes shape. use alloc::vec::Vec; +use lpc_model::LpValue; -use crate::dataflow::binding::{BindingRef, BindingSource}; +use crate::dataflow::binding::BindingRef; +use crate::dataflow::resolver::query_intern::QueryId; + +/// Where a binding's value comes from, resolved down to something the frame +/// can act on without touching the binding graph again. +/// +/// A binding that reads another slot or a bus channel is stored as the +/// [`QueryId`] of that query rather than as a `BindingSource`: the id is what +/// resolution needs, and deriving it means hashing a `SlotPath`. Ids and +/// routes share the same lifetime — both die when the graph changes — so the +/// id can simply be part of the decision. +#[derive(Clone, Debug)] +pub enum RouteTarget { + /// A literal written into the binding; materialize it directly. + Literal(LpValue), + /// Resolve this query and adopt its value. + Query(QueryId), +} /// The decision about how one query is answered. #[derive(Clone, Debug)] pub enum ResolvedRoute { - /// Resolve through this binding's source. + /// Resolve through this binding. Binding { binding_ref: BindingRef, - source: BindingSource, + target: RouteTarget, }, /// No binding answers this query; the host produces it. Produce, @@ -30,6 +48,6 @@ pub enum ResolvedRoute { /// walk that flattens them reads only the binding graph, so it is part of /// the decision rather than part of the frame. MergeByKey { - inputs: Vec<(BindingRef, BindingSource)>, + inputs: Vec<(BindingRef, RouteTarget)>, }, } diff --git a/lp-core/lpc-engine/src/dataflow/resolver/tick_resolver.rs b/lp-core/lpc-engine/src/dataflow/resolver/tick_resolver.rs index bcfb32b0b..0df13b090 100644 --- a/lp-core/lpc-engine/src/dataflow/resolver/tick_resolver.rs +++ b/lp-core/lpc-engine/src/dataflow/resolver/tick_resolver.rs @@ -14,7 +14,17 @@ use lpc_model::{NodeId, Revision, SlotPath}; /// Narrow API for [`crate::node::TickContext`] demand reads (`QueryKey` → [`Production`]). pub trait TickResolver { - fn resolve(&mut self, query: QueryKey) -> Result; + /// Resolve `query`. Borrowed rather than owned so a node that reads the + /// same slot every frame can keep one key instead of rebuilding it. + fn resolve(&mut self, query: &QueryKey) -> Result; + + /// Resolve one of `node`'s consumed slots named by a constant path, + /// without rebuilding the query each frame. + fn resolve_static_consumed( + &mut self, + node: NodeId, + path: &'static str, + ) -> Result; fn publish_produced_slot( &mut self, @@ -54,12 +64,22 @@ pub struct SessionHostResolver<'sess, 'resolver, 'host> { } impl<'sess, 'resolver, 'host> TickResolver for SessionHostResolver<'sess, 'resolver, 'host> { - fn resolve(&mut self, query: QueryKey) -> Result { + fn resolve(&mut self, query: &QueryKey) -> Result { self.session .resolve(self.host, query) .map_err(|e: SessionResolveError| ResolveError::new(alloc::format!("{e}"))) } + fn resolve_static_consumed( + &mut self, + node: NodeId, + path: &'static str, + ) -> Result { + self.session + .resolve_static_consumed(self.host, node, path) + .map_err(|e: SessionResolveError| ResolveError::new(alloc::format!("{e}"))) + } + fn publish_produced_slot( &mut self, node: NodeId, diff --git a/lp-core/lpc-engine/src/engine/engine.rs b/lp-core/lpc-engine/src/engine/engine.rs index bb29b0242..72824cc6b 100644 --- a/lp-core/lpc-engine/src/engine/engine.rs +++ b/lp-core/lpc-engine/src/engine/engine.rs @@ -541,7 +541,7 @@ impl Engine { radio_service, frame_time_seconds: time_s, }; - let result = session.resolve(&mut host, key); + let result = session.resolve(&mut host, &key); self.resolver = resolver_tmp; let production = result?; let Some(leaf) = production.value_leaf() else { @@ -629,7 +629,7 @@ impl Engine { radio_service, frame_time_seconds: time_s, }; - let result = session.resolve(&mut host, QueryKey::Bus(channel.clone())); + let result = session.resolve(&mut host, &QueryKey::Bus(channel.clone())); self.resolver = resolver; result } @@ -1882,7 +1882,7 @@ pub(crate) fn resolve_with_engine_host( frame_time_seconds: time_s, }; let result = session - .resolve(&mut host, key) + .resolve(&mut host, &key) .map(|pv| (pv, session.trace().clone())); eng.resolver = resolver_tmp; result @@ -1919,9 +1919,9 @@ pub(super) fn resolve_twice_same_frame_with_engine_host( radio_service, frame_time_seconds: time_s, }; - let result = session.resolve(&mut host, key.clone()).and_then(|first| { + let result = session.resolve(&mut host, &key).and_then(|first| { session - .resolve(&mut host, key) + .resolve(&mut host, &key) .map(|second| (first, second)) }); eng.resolver = resolver_tmp; diff --git a/lp-core/lpc-engine/src/engine/test_support.rs b/lp-core/lpc-engine/src/engine/test_support.rs index fcd3f86d9..01d91077b 100644 --- a/lp-core/lpc-engine/src/engine/test_support.rs +++ b/lp-core/lpc-engine/src/engine/test_support.rs @@ -495,7 +495,7 @@ impl DummyFixtureNode { impl NodeRuntime for DummyFixtureNode { fn consume(&mut self, ctx: &mut TickContext<'_>) -> Result<(), NodeError> { let pv = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: self.slot.clone(), }) @@ -531,7 +531,7 @@ impl DummyOutputNode { impl NodeRuntime for DummyOutputNode { fn consume(&mut self, ctx: &mut TickContext<'_>) -> Result<(), NodeError> { let pv = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: self.slot.clone(), }) @@ -585,7 +585,7 @@ impl NodeRuntime for DummySelectorNode { let index = self.selected.load(Ordering::Relaxed) as usize % self.targets.len(); let (node, slot) = self.targets[index].clone(); let pv = ctx - .resolve(QueryKey::ProducedSlot { node, slot }) + .resolve(&QueryKey::ProducedSlot { node, slot }) .map_err(|e| NodeError::msg(format!("selector resolve failed: {}", e.message)))?; self.record.record(&pv.as_value().expect("value")); Ok(()) diff --git a/lp-core/lpc-engine/src/node/contexts.rs b/lp-core/lpc-engine/src/node/contexts.rs index 5ba875085..b127ba648 100644 --- a/lp-core/lpc-engine/src/node/contexts.rs +++ b/lp-core/lpc-engine/src/node/contexts.rs @@ -195,10 +195,23 @@ impl<'r> TickContext<'r> { } /// Resolve a [`QueryKey`] for this frame (cache, bindings, optional host production). - pub fn resolve(&mut self, query: QueryKey) -> Result { + pub fn resolve(&mut self, query: &QueryKey) -> Result { self.resolver.resolve(query) } + /// Resolve one of this node's consumed slots named by a constant path. + /// + /// Prefer this to building a [`QueryKey`] from a parsed path when the path + /// is a literal: the parse and the key are memoized for as long as they + /// stay valid, instead of being rebuilt and dropped every frame. + pub fn resolve_static_consumed( + &mut self, + path: &'static str, + ) -> Result { + let node = self.node_id; + self.resolver.resolve_static_consumed(node, path) + } + pub fn publish_runtime_slot( &mut self, state_root: &dyn SlotAccess, @@ -225,7 +238,7 @@ impl<'r> TickContext<'r> { T: FromLpValue, { let production = self - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: self.node_id, slot: slot.clone(), }) @@ -249,7 +262,7 @@ impl<'r> TickContext<'r> { T: FromLpValue, { let production = self - .resolve(QueryKey::ConsumedSlotAccessor { + .resolve(&QueryKey::ConsumedSlotAccessor { node: self.node_id, accessor: accessor.clone(), }) @@ -814,7 +827,7 @@ mod tests { &slot_shapes, ); let pv = ctx - .resolve(QueryKey::Bus(channel.clone())) + .resolve(&QueryKey::Bus(channel.clone())) .expect("resolve bus"); assert!(pv.as_value().expect("value").eq(&LpsValueF32::F32(7.8))); } @@ -855,7 +868,7 @@ mod tests { ); let pv = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node, slot: input.clone(), }) @@ -890,7 +903,7 @@ mod tests { ctx.publish_runtime_slot(&state, slot.clone()) .expect("publish"); let production = ctx - .resolve(QueryKey::ProducedSlot { node, slot }) + .resolve(&QueryKey::ProducedSlot { node, slot }) .expect("resolve published slot"); assert_eq!( @@ -938,7 +951,7 @@ mod tests { _slot: &SlotPath, ctx: &mut TickContext<'_>, ) -> Result { - let pv = ctx.resolve(self.query.clone()).map_err(|e| { + let pv = ctx.resolve(&self.query).map_err(|e| { crate::node::NodeError::msg(alloc::format!("resolve failed: {}", e.message)) })?; if let LpsValueF32::F32(v) = pv.as_value().expect("value") { diff --git a/lp-core/lpc-engine/src/nodes/fixture/fixture_node.rs b/lp-core/lpc-engine/src/nodes/fixture/fixture_node.rs index e941de15d..243931508 100644 --- a/lp-core/lpc-engine/src/nodes/fixture/fixture_node.rs +++ b/lp-core/lpc-engine/src/nodes/fixture/fixture_node.rs @@ -23,7 +23,6 @@ use crate::nodes::fixture::mapping::{ use lp_gfx::{SampleOutHandle, SamplePointsHandle, TextureData, TextureHandle}; use lpc_model::nodes::texture::TextureFormat; -use crate::dataflow::resolver::QueryKey; use crate::node::{ AssetRefreshContext, AssetRefreshResult, ControlNode, ControlRenderContext, DestroyCtx, MemPressureCtx, NodeError, NodeRuntime, PressureLevel, ProduceResult, RuntimeStateShape, @@ -254,8 +253,12 @@ struct FixtureDisplayLayoutKey { height: u32, } +/// The fixture's authored input slot. Resolution takes it as a constant so +/// the path is parsed once rather than per frame. +pub(crate) const FIXTURE_INPUT_PATH: &str = "input"; + pub fn fixture_input_path() -> SlotPath { - SlotPath::parse("input").expect("fixture input path") + SlotPath::parse(FIXTURE_INPUT_PATH).expect("fixture input path") } pub fn fixture_output_path() -> SlotPath { @@ -302,10 +305,7 @@ impl NodeRuntime for FixtureNode { // stays viewable/editable, with the cause surfaced as runtime // status. A resolved input carrying the wrong shape keeps // failing loudly — that is authored misconfiguration. - match ctx.resolve(QueryKey::ConsumedSlot { - node: ctx.node_id(), - slot: fixture_input_path(), - }) { + match ctx.resolve_static_consumed(FIXTURE_INPUT_PATH) { Ok(prod) => { let visual_product = match prod .value_leaf() @@ -500,22 +500,19 @@ fn sync_mapping_config_from_def( fn try_read_def_value( ctx: &mut TickContext<'_>, - path: &str, + path: &'static str, ) -> Result, NodeError> { - let slot = SlotPath::parse(path).map_err(|e| { - NodeError::msg(alloc::format!( - "invalid authored fixture path {path:?}: {e}" - )) - })?; - let production = match ctx.resolve(QueryKey::ConsumedSlot { - node: ctx.node_id(), - slot: slot.clone(), - }) { + let production = match ctx.resolve_static_consumed(path) { Ok(production) => production, Err(e) => { // "Absent" (no def loaded, inactive enum variant, option none) is // expected and reads as None; a path that cannot exist in the // FixtureDef shape is a code bug and must not be swallowed. + let slot = SlotPath::parse(path).map_err(|e| { + NodeError::msg(alloc::format!( + "invalid authored fixture path {path:?}: {e}" + )) + })?; ensure_path_exists_in_fixture_def_shape(ctx.slot_shapes(), &slot)?; log::debug!("[fixture] def path {path} unavailable: {}", e.message); return Ok(None); diff --git a/lp-core/lpc-engine/src/nodes/fluid/fluid_node.rs b/lp-core/lpc-engine/src/nodes/fluid/fluid_node.rs index 24587f90c..35ee8a1e6 100644 --- a/lp-core/lpc-engine/src/nodes/fluid/fluid_node.rs +++ b/lp-core/lpc-engine/src/nodes/fluid/fluid_node.rs @@ -234,7 +234,7 @@ impl RenderNode for FluidNode { fn resolve_emitters(ctx: &mut TickContext<'_>) -> Result, NodeError> { let production = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: fluid_emitters_path(), }) diff --git a/lp-core/lpc-engine/src/nodes/output/output_node.rs b/lp-core/lpc-engine/src/nodes/output/output_node.rs index 75587dacd..fbf42b16d 100644 --- a/lp-core/lpc-engine/src/nodes/output/output_node.rs +++ b/lp-core/lpc-engine/src/nodes/output/output_node.rs @@ -59,7 +59,7 @@ impl NodeRuntime for OutputNode { fn consume(&mut self, ctx: &mut TickContext<'_>) -> Result<(), NodeError> { let prod = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: output_input_path(), }) diff --git a/lp-core/lpc-engine/src/nodes/playlist/playlist_node.rs b/lp-core/lpc-engine/src/nodes/playlist/playlist_node.rs index d21c4f353..58b8e53b1 100644 --- a/lp-core/lpc-engine/src/nodes/playlist/playlist_node.rs +++ b/lp-core/lpc-engine/src/nodes/playlist/playlist_node.rs @@ -434,7 +434,7 @@ fn detect_triggered_entry( last_seen: &mut VecMap, ) -> Result, NodeError> { let production = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: SlotPath::parse("trigger").expect("playlist trigger slot"), }) @@ -483,7 +483,7 @@ fn resolve_entry_product( entry: &PlaylistRuntimeEntry, ) -> Result { let production = ctx - .resolve(QueryKey::ProducedSlot { + .resolve(&QueryKey::ProducedSlot { node: entry.child, slot: entry.output_slot.clone(), }) diff --git a/lp-core/lpc-engine/src/nodes/radio/control_radio_node.rs b/lp-core/lpc-engine/src/nodes/radio/control_radio_node.rs index fb270d96d..835ce4850 100644 --- a/lp-core/lpc-engine/src/nodes/radio/control_radio_node.rs +++ b/lp-core/lpc-engine/src/nodes/radio/control_radio_node.rs @@ -304,7 +304,7 @@ impl NodeRuntime for ControlRadioNode { fn resolve_input_messages(ctx: &mut TickContext<'_>) -> Result, NodeError> { let production = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: control_radio_input_path(), }) diff --git a/lp-core/lpc-engine/src/nodes/shader/compute_shader_node.rs b/lp-core/lpc-engine/src/nodes/shader/compute_shader_node.rs index a2e78d418..8789bb938 100644 --- a/lp-core/lpc-engine/src/nodes/shader/compute_shader_node.rs +++ b/lp-core/lpc-engine/src/nodes/shader/compute_shader_node.rs @@ -444,7 +444,7 @@ fn resolve_or_default_input( ) -> Result { let slot_path = SlotPath::parse(name) .map_err(|e| NodeError::msg(format!("invalid compute consumed slot {name:?}: {e}")))?; - let production = match ctx.resolve(QueryKey::ConsumedSlot { + let production = match ctx.resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: slot_path, }) { diff --git a/lp-core/lpc-engine/src/nodes/shader/shader_node.rs b/lp-core/lpc-engine/src/nodes/shader/shader_node.rs index 03cfe07a9..39e4cc14a 100644 --- a/lp-core/lpc-engine/src/nodes/shader/shader_node.rs +++ b/lp-core/lpc-engine/src/nodes/shader/shader_node.rs @@ -476,7 +476,7 @@ pub(super) fn read_authored_value( /// defs) — the runtime key set is then left as loaded. fn try_read_authored_consumed_keys(ctx: &mut TickContext<'_>) -> Option> { let production = ctx - .resolve(QueryKey::ConsumedSlot { + .resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: SlotPath::parse("consumed").expect("static path"), }) @@ -502,7 +502,7 @@ fn try_read_authored_value( let slot = SlotPath::parse(path).map_err(|e| { NodeError::msg(alloc::format!("invalid authored shader path {path:?}: {e}")) })?; - let production = match ctx.resolve(QueryKey::ConsumedSlot { + let production = match ctx.resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot, }) { @@ -769,7 +769,7 @@ fn resolve_or_default_input( ) -> Result { let slot_path = SlotPath::parse(name) .map_err(|e| NodeError::msg(format!("invalid visual consumed slot {name:?}: {e}")))?; - let production = match ctx.resolve(QueryKey::ConsumedSlot { + let production = match ctx.resolve(&QueryKey::ConsumedSlot { node: ctx.node_id(), slot: slot_path, }) { diff --git a/lp-core/lpc-engine/tests/runtime_spine.rs b/lp-core/lpc-engine/tests/runtime_spine.rs index 1c53a20b4..d7e5c1c72 100644 --- a/lp-core/lpc-engine/tests/runtime_spine.rs +++ b/lp-core/lpc-engine/tests/runtime_spine.rs @@ -557,7 +557,7 @@ impl NodeRuntime for ProduceProbeNode { ctx: &mut TickContext<'_>, ) -> Result { let pv = ctx - .resolve(self.query.clone()) + .resolve(&self.query) .map_err(|e| NodeError::msg(format!("resolve: {}", e.message)))?; if let LpsValueF32::F32(v) = pv.as_value().expect("value") { self.last = Some(v); From edc435a9f877fa9c3762a4233468c91f1770055f Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Fri, 31 Jul 2026 23:46:14 -0700 Subject: [PATCH 5/9] docs: record the resolver persistence decision and what it measured MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ADR states the invalidation contract, because that is the part that does not fail loudly: a mutation site that forgets to invalidate serves a stale answer that looks entirely plausible. It names the current sites, including the two that were missing before this work, and says why a playlist switch is deliberately not one of them. The debt entry gets the numbers rather than a claim: −54% on the 1-fixture oracle, but only −1.9% on the four-fixture project, because endpoint status is 46.7% of that profile and belongs to the other half of the exit criteria. The desk-S3 rows stay unrefreshed and are marked so — the board did not enumerate this session. Plan: 2026-07-31-2225-persist-dataflow-resolution --- ...26-07-31-resolver-persistent-resolution.md | 158 ++++++++++++++++++ docs/debt/s3-frame-cost-scales-per-fixture.md | 40 +++++ 2 files changed, 198 insertions(+) create mode 100644 docs/adr/2026-07-31-resolver-persistent-resolution.md diff --git a/docs/adr/2026-07-31-resolver-persistent-resolution.md b/docs/adr/2026-07-31-resolver-persistent-resolution.md new file mode 100644 index 000000000..2679d3208 --- /dev/null +++ b/docs/adr/2026-07-31-resolver-persistent-resolution.md @@ -0,0 +1,158 @@ +# ADR: The dataflow resolver keeps its resolution across frames + +- **Status:** Accepted +- **Date:** 2026-07-31 +- **Deciders:** Photomancer +- **Supersedes:** None +- **Superseded by:** None + +## Context + +Frame cost on the desk ESP32-S3 was measured as a flat ~8.4 ms per +fixture+output chain — the same whether a fixture rendered 30×1 or 90×90, and +the same for 10 mapped LEDs or 120. Authors could not buy the cost down, which +made multi-fixture projects degrade linearly for no reason they could see (4 +fixtures = 20 fps; ~10 would be single digits). + +`lp-cli profile` attributed it. The shader — the thing the product is named +for, JIT-compiled to native code on the device — was **1.1%** of self cycles. +The frame was dominated by the dataflow resolver re-resolving the binding +graph from cold every tick: `Resolver::clear_frame_cache` ran each frame, so +each frame re-walked `resolve → resolve_binding_source → resolve` with +`SlotPath::parse` per read, `QueryKey` allocation, equality and drop, and the +allocator/`memcpy` pair at ~44% of self cycles combined. + +The resolver already had a cache that outlived nothing. It was cleared per +tick because one method, `clear_frame_cache()`, served two unrelated callers: +"a new frame started" and "the graph changed shape". While both said the same +thing, no caller could persist anything. + +## Decision + +**Resolution persists across frames and is invalidated by structural change, +not by the passage of a frame.** + +The resolver distinguishes two operations: + +- `begin_frame()` — values resolved for the previous frame are stale, because + producers tick and time moves. The graph is unchanged. +- `invalidate_structure()` — bindings, tree topology, node definitions, or + project state changed. Everything cached is void. + +Three things persist until `invalidate_structure()`: + +1. **Routes** (`ResolvedRoute`) — the decision about *how* a query is + answered: which binding wins (owner depth for consumed slots, priority for + bus providers), the merge policy, and the fully expanded provider list for + mergeable receivers. Computing this reads the binding index and introspects + authored definitions; none of it can change without a structural change. +2. **Def-sourced values** — `ProductionSource::Literal` (binding literals) and + `::Default` (authored-def reads, including their deep `snapshot_slot_shape` + copies). Both are functions of the project, not of the frame. +3. **Interned queries** — `QueryKey → QueryId`, plus a memo from + `(node, constant path)` to the query it names. + +Everything else stays per-frame: producer values, merges, and anything reached +*through* a binding. A cached route still runs the producer behind it, once +per frame. + +Failures are never cached. An ambiguous bus channel or a cyclic binding graph +keeps reporting itself rather than being remembered as a decision. + +## The invalidation contract + +**Any engine mutation that can change what a query resolves *through* — as +opposed to what it resolves *to* this frame — must call +`Resolver::invalidate_structure()`.** + +This is the load-bearing rule, and it does not fail loudly. A missed call +serves a stale but entirely plausible answer. The current sites: + +| Site | Why | +|---|---| +| `Engine::apply_project_changes` | Every registry-driven change: deploy, reload, overlay mutation, node add/remove/replace, def changes, asset refresh. Also rebuilds all bindings. | +| `Engine::remove_runtime_subtree` | Tree topology | +| `Engine::reattach_runtime_node` | A node's runtime is replaced under its consumers | +| `Engine::add_binding` | A new binding can win a slot that already resolved | +| `Engine::clear_bindings` | The binding graph is emptied | + +Two of those were found while writing this down. `add_binding` never +invalidated anything — harmless while every frame cleared regardless, a +stale-value bug the moment it did not. `clear_bindings` ran on the tree, so +emptying the binding graph left the resolver's knowledge of it intact and +depended on whatever ran next to invalidate; it now goes through the engine. + +A playlist entry switch is deliberately **not** structural. It changes which +child a node demands, not what any query resolves through, and the resolver +handles it by simply being asked a different question. + +## Consequences + +### Measured + +1-fixture workload (`projects/test/quad-strips-1fix`), steady render, esp32c6 +cycle model: + +| | Baseline | After | +|---|---|---| +| Total attributed cycles | 2,468,427 | **1,146,110** (−54%) | +| allocator + memcpy | 44.0% | 34.4% | +| `[jit] render` | 1.1% | 2.4% (same cycles, less than half the frame) | + +`QueryKey::eq`, `merge_policy_for_consumed_slot`, +`bindings_for_consumed_slot`, `slot_lookup` and `Vec::clone` +no longer appear in the top twenty. + +The 4-fixture workload moved only −1.9%, because +`HwRegistry::endpoint_status_for` is 46.7% of it and is a separate per-frame +recomputation with its own owner. See +`docs/debt/s3-frame-cost-scales-per-fixture.md`. + +### Revision stamps age + +A cached literal or def read keeps the revision it was first stamped with, +rather than being restamped each frame. This is the more truthful reading — +the value genuinely has not changed since then — and an audit of every +`changed_at` consumer on the resolve path found none that compares a +production's revision against the current frame. Playlist triggers compare +message sequence numbers; wire delta-sync treats an aged stamp as "unchanged, +do not resend", which is correct for a value cached *because* it is unchanged. + +### Ids are epoch-scoped + +`QueryId`s are indices into a table that is cleared on every structural +change. They must never be stored anywhere that outlives the resolver's +epoch — which is why routes, themselves epoch-scoped, are the one place that +holds them. + +## Alternatives considered + +**A compiled resolution plan** — build an explicit schedule of producer ticks +and routes at load time and execute it per frame. Higher ceiling: no +per-query overhead at all. Rejected because nodes issue queries that are not +knowable ahead of the tick (a fixture reads `power.some` only if the def has +it; a shader reads one query per declared input; a playlist demands whichever +child is current), so the plan would need a discovery pass per frame anyway — +which is lazy memoization with extra steps. This decision does not foreclose +it: routes are already the per-query half of such a plan. + +**Leaving the cache per-frame and optimizing the constants** — cheaper key +comparison, fewer clones. Rejected on the measurement: the work itself was +redundant, not merely expensive. + +## How this is kept + +Two mechanisms, because the failure mode is silence: + +- **Counters** (`ResolveFrameCounters`) make "did this frame re-derive the + graph?" a testable question. `steady_state_frame_does_zero_structural_work` + asserts a steady frame performs no binding lookups, no merge-policy reads + and no def reads. +- **A differential test** runs the same scene twice — once caching, once with + `set_force_invalidate_per_frame(true)`, which reproduces the old + clear-everything-per-tick behaviour — through binding replacement, a + producer switch and a node reattach, and demands the two agree frame for + frame. This is the check that does not require someone to have first + imagined the particular way a cache could go stale, which matters because a + wrong cached answer is a plausible answer and survives assertions written by + whoever wrote the cache. diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index b6c64395c..02a0c31da 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -68,6 +68,37 @@ nothing else helps, by measurement. resolution machinery (dataflow resolver + endpoint status) instead. The suspicious shapes: `clear_frame_cache` discarding all resolution work every tick, and endpoint status recomputed per frame per channel. +- **2026-07-31** — **Resolver half addressed** (PR #243, + `docs/adr/2026-07-31-resolver-persistent-resolution.md`). Routes, binding + literals and authored-def reads now persist across frames and are dropped + on structural change instead of per tick. Measured on the 1-fixture + workload (`projects/test/quad-strips-1fix`, committed as the reproducible + oracle), steady render, esp32c6 cycle model: + + | | Before | After | + |---|---|---| + | Total attributed cycles | 2,468,427 | 1,146,110 (−54%) | + | allocator + memcpy | 44.0% | 34.4% | + | `[jit] render` | 1.1% | 2.4% (same cycles) | + + `QueryKey::eq`, `merge_policy_for_consumed_slot`, + `bindings_for_consumed_slot`, `slot_lookup` and `Vec::clone` + are gone from the top twenty. + + The **4-fixture workload moved only −1.9%** (65,811,630 → 64,531,359), + because `HwRegistry::endpoint_status_for` is 46.7% of that profile and is + untouched by this work. The endpoint-status half is what now caps + multi-fixture fps. + + Two smaller per-frame re-derivations surfaced while measuring, both the same + shape and neither addressed: the shader node re-reads its consumed-slot + *definitions* every frame through `format!`-built paths (now the largest + single allocation source in the 1-fixture profile), and a resolver cache hit + still clones a `ProductionSource` carrying a `SlotPath`. + + ⚠️ Desk-S3 re-measurement is **still pending** — the board did not enumerate + for `espflash board-info` during the session (needs a physical replug). The + fps matrix in the table above has not been refreshed on silicon. **Exit criteria** — A profiled optimization pass that makes resolved bindings and endpoint status persist across frames (invalidate on tree/binding/ @@ -75,3 +106,12 @@ hardware change, not per tick), after which frame cost is dominated by actual rendering work and the profile's top self-cycle entries are no longer allocator/memcpy/string machinery. Re-measure the same quad-strips matrix on the desk S3 as the oracle. + +- [x] **Dataflow resolver** — done 2026-07-31, see the incident log above. +- [ ] **`HwRegistry::endpoint_status_for`** — per-frame re-enumeration of + hardware endpoints with per-endpoint status recomputation and string + spec formatting. 46.7% of the 4-fixture profile; the remaining cap on + multi-fixture fps. +- [ ] **Desk-S3 re-measurement** of the quad-strips matrix. Not done for the + resolver work (board unreachable); worth doing once both halves land, + rather than twice. From 2e3cf3788a585dd29f8fa9877863cacaacda931b Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Sat, 1 Aug 2026 07:48:49 -0700 Subject: [PATCH 6/9] feat(lpc-engine): make the invalidation contract check itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rule that keeps persisted resolution correct — any mutation that changes what a query resolves *through* must call `invalidate_structure()` — was stated only in an ADR, and breaking it is silent: the resolver keeps serving an answer that is stale but entirely plausible. Prose does not survive contact with the next mutation site. So `tick_nodes` now compares a cheap fingerprint of the tree's shape (node count, binding count, newest binding revision) against the previous frame's and asserts that any change came with an epoch bump. Debug builds only — release firmware pays nothing, and the guard's job is to fire during development, long before a device is involved. Verified by reintroducing the bug: dropping the `invalidate_structure()` from `add_binding` trips it with the shape delta and a pointer to the ADR. Worth noting that the differential test *passed* under that same sabotage — the scenario reaches `add_binding` via `clear_bindings`, which does invalidate. Two checks, catching different things. Also records the desk-S3 numbers, measured today on d8:3b:da:47:29:70: 4 fixtures 20 fps / 48 ms -> 25 fps / 37.5 ms 1 fixture 50 fps / 19 ms -> 67 fps / 13.5 ms Hardware beat the emulator's +2% prediction for four fixtures because the profile runs the virtual WS281x driver, whose endpoint enumeration is dearer than the real RMT one — the emulator attributes cost well but does not predict fps. And the honest read on the filed shape: per-additional-chain cost only fell ~9.7 -> ~8.0 ms. Most of this win is fixed per-frame overhead, not the per-chain scaling the debt entry is named for. That still belongs to endpoint status. Plan: 2026-07-31-2225-persist-dataflow-resolution --- ...26-07-31-resolver-persistent-resolution.md | 42 +++++++++++++++++-- docs/debt/s3-frame-cost-scales-per-fixture.md | 28 +++++++++---- lp-core/lpc-engine/src/engine/engine.rs | 37 ++++++++++++++++ lp-core/lpc-engine/src/node/node_tree.rs | 21 ++++++++++ 4 files changed, 118 insertions(+), 10 deletions(-) diff --git a/docs/adr/2026-07-31-resolver-persistent-resolution.md b/docs/adr/2026-07-31-resolver-persistent-resolution.md index 2679d3208..c8f062534 100644 --- a/docs/adr/2026-07-31-resolver-persistent-resolution.md +++ b/docs/adr/2026-07-31-resolver-persistent-resolution.md @@ -82,6 +82,25 @@ stale-value bug the moment it did not. `clear_bindings` ran on the tree, so emptying the binding graph left the resolver's knowledge of it intact and depended on whatever ran next to invalidate; it now goes through the engine. +**The rule checks itself.** A contract that lives only in this document will +not survive the next mutation site, so `Engine::tick_nodes` compares a cheap +fingerprint of the tree's shape (node count, binding count, newest binding +revision) against the previous frame's, and asserts that a change was +accompanied by an epoch bump. Debug builds only — release firmware pays +nothing, and the guard's whole job is to fire during development and tests, +long before a device is involved. Verified by reintroducing the bug: removing +the `invalidate_structure()` from `add_binding` produces + +``` +the node tree changed shape ((2, 2, Revision(1)) -> (2, 3, Revision(2))) +without Resolver::invalidate_structure(); resolution cached against the old +graph is now being served. +``` + +That case is worth noting: the differential test **passed** under that +sabotage, because the scenario reached `add_binding` through `clear_bindings`, +which does invalidate. Two independent checks, catching different things. + A playlist entry switch is deliberately **not** structural. It changes which child a node demands, not what any query resolves through, and the resolver handles it by simply being asked a different question. @@ -103,9 +122,26 @@ cycle model: `bindings_for_consumed_slot`, `slot_lookup` and `Vec::clone` no longer appear in the top twenty. -The 4-fixture workload moved only −1.9%, because -`HwRegistry::endpoint_status_for` is 46.7% of it and is a separate per-frame -recomputation with its own owner. See +The 4-fixture workload moved only −1.9% in the emulator, because +`HwRegistry::endpoint_status_for` is 46.7% of that profile. + +On the **desk ESP32-S3** (2026-08-01, same board and projects as the original +measurement): + +| Config | Before | After | | +|---|---|---|---| +| 4 fixtures (`quad-strips`) | 20 fps / 48 ms | **25 fps / 37.5 ms** | +25% fps | +| 1 fixture (`quad-strips-1fix`) | 50 fps / 19 ms | **67 fps / 13.5 ms** | +34% fps | + +Hardware improved *more* than the emulator predicted for the 4-fixture case +(+25% vs +2%), because the emulator profiles the **virtual** WS281x driver, +whose endpoint enumeration is more expensive than the real RMT driver's — so +`endpoint_status_for`'s 46.7% share is inflated there. Treat the emulator as +an attribution tool, not an fps predictor. + +The per-additional-chain cost fell from ~9.7 ms to ~8.0 ms (−17%): most of the +win is in fixed per-frame overhead, not in the per-chain scaling that made +this a filed debt. That scaling is still owned by endpoint status. See `docs/debt/s3-frame-cost-scales-per-fixture.md`. ### Revision stamps age diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index 02a0c31da..720c7a0b7 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -13,7 +13,8 @@ related: **Shape** — Measured on the desk ESP32-S3 (2026-07-31, all eight node gates, quad-strips variants, 30-LED strips), then attributed with `lp-cli profile` -(emulator, esp32-c6 cycle model): +(emulator, esp32-c6 cycle model). The table below is the ORIGINAL measurement; +see the 2026-08-01 incident-log entry for the post-resolver numbers: | Config | fps | tick | |---|---|---| @@ -96,9 +97,23 @@ nothing else helps, by measurement. single allocation source in the 1-fixture profile), and a resolver cache hit still clones a `ProductionSource` carrying a `SlotPath`. - ⚠️ Desk-S3 re-measurement is **still pending** — the board did not enumerate - for `espflash board-info` during the session (needs a physical replug). The - fps matrix in the table above has not been refreshed on silicon. + **Desk-S3 re-measured 2026-08-01** (same board `d8:3b:da:47:29:70`, same + projects): + + | Config | Before | After | | + |---|---|---|---| + | 4 fixtures | 20 fps / 48 ms | **25 fps / 37.5 ms** | +25% fps | + | 1 fixture | 50 fps / 19 ms | **67 fps / 13.5 ms** | +34% fps | + + Hardware beat the emulator's prediction for 4 fixtures (+25% vs +2%): the + profile uses the **virtual** WS281x driver, whose endpoint enumeration is + dearer than the real RMT driver's, so `endpoint_status_for`'s 46.7% share is + inflated there. The emulator attributes cost well; it does not predict fps. + + **The filed shape is only partly fixed.** Per-additional-chain cost went + ~9.7 ms → ~8.0 ms (−17%) — most of the win is fixed per-frame overhead, not + the per-chain scaling this entry is named for. A 10-fixture project would + still be in the low teens. Endpoint status owns that scaling. **Exit criteria** — A profiled optimization pass that makes resolved bindings and endpoint status persist across frames (invalidate on tree/binding/ @@ -112,6 +127,5 @@ the desk S3 as the oracle. hardware endpoints with per-endpoint status recomputation and string spec formatting. 46.7% of the 4-fixture profile; the remaining cap on multi-fixture fps. -- [ ] **Desk-S3 re-measurement** of the quad-strips matrix. Not done for the - resolver work (board unreachable); worth doing once both halves land, - rather than twice. +- [x] **Desk-S3 re-measurement** — done 2026-08-01 for the resolver half + (20 → 25 fps at 4 fixtures). Re-measure again after endpoint status. diff --git a/lp-core/lpc-engine/src/engine/engine.rs b/lp-core/lpc-engine/src/engine/engine.rs index 72824cc6b..1b2ca5eb2 100644 --- a/lp-core/lpc-engine/src/engine/engine.rs +++ b/lp-core/lpc-engine/src/engine/engine.rs @@ -61,6 +61,11 @@ pub struct Engine { services: EngineServices, demand_roots: Vec, graphics: Option>, + /// The tree shape and resolver epoch as of the last tick, so that a + /// structural change that forgot to invalidate resolution is caught here + /// rather than by someone noticing a stale value on a device. + #[cfg(debug_assertions)] + last_structural_check: Option<((usize, usize, Revision), u64)>, } impl Engine { @@ -83,6 +88,8 @@ impl Engine { services, demand_roots: Vec::new(), graphics: None, + #[cfg(debug_assertions)] + last_structural_check: None, } } @@ -430,6 +437,8 @@ impl Engine { } fn tick_nodes(&mut self, registry: &ProjectRegistry, delta_ms: u32) -> Result<(), EngineError> { + #[cfg(debug_assertions)] + self.assert_structural_changes_were_announced(); self.resolver.begin_frame(); self.frame_num = self.frame_num.next(); self.revision = advance_revision(); @@ -468,6 +477,34 @@ impl Engine { Ok(()) } + /// Fail loudly when the graph changed shape without anyone calling + /// [`Resolver::invalidate_structure`]. + /// + /// The invalidation contract is the load-bearing rule behind persisting + /// resolution across frames, and breaking it is silent by nature: the + /// resolver keeps serving an answer that is stale but entirely plausible. + /// Prose in an ADR does not survive contact with a new mutation site, so + /// the rule checks itself. + /// + /// Debug builds only — release firmware pays nothing. That is the right + /// trade for a guard whose whole job is to fire during development and + /// tests, long before a device is involved. + #[cfg(debug_assertions)] + fn assert_structural_changes_were_announced(&mut self) { + let fingerprint = self.tree.structural_fingerprint(); + let epoch = self.resolver.structure_epoch(); + if let Some((previous_fingerprint, previous_epoch)) = self.last_structural_check { + debug_assert!( + fingerprint == previous_fingerprint || epoch != previous_epoch, + "the node tree changed shape ({previous_fingerprint:?} -> {fingerprint:?}) \ + without Resolver::invalidate_structure(); resolution cached against the old \ + graph is now being served. Whatever mutated the tree or its bindings must \ + announce it — see docs/adr/2026-07-31-resolver-persistent-resolution.md" + ); + } + self.last_structural_check = Some((fingerprint, epoch)); + } + /// Materialize a visual product handle into a CPU texture. /// /// This is the same materialization the wire render-product probe uses; diff --git a/lp-core/lpc-engine/src/node/node_tree.rs b/lp-core/lpc-engine/src/node/node_tree.rs index 43b7e378a..75566ac81 100644 --- a/lp-core/lpc-engine/src/node/node_tree.rs +++ b/lp-core/lpc-engine/src/node/node_tree.rs @@ -372,6 +372,27 @@ impl RuntimeNodeTree { Ok(()) } + /// A cheap summary of the tree's *shape*, for catching structural changes + /// that forgot to invalidate resolution. + /// + /// Node count, binding count and the newest binding revision together move + /// whenever the binding graph or the topology does. This is not a hash and + /// does not need to be: it is compared against its own previous value one + /// frame later, by a debug-only assertion, to answer "did the graph change + /// without saying so?". + #[cfg(debug_assertions)] + pub fn structural_fingerprint(&self) -> (usize, usize, Revision) { + let mut nodes = 0; + let mut bindings = 0; + let mut newest = Revision::default(); + for entry in self.entries() { + nodes += 1; + bindings += entry.bindings.value().len(); + newest = core::cmp::max(newest, entry.bindings.changed_at()); + } + (nodes, bindings, newest) + } + /// Get the number of live entries (excludes tombstones). pub fn len(&self) -> usize { self.nodes.iter().filter(|opt| opt.is_some()).count() From bb157ce5756886078fbadd190a89b8b11c37a673 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Sat, 1 Aug 2026 07:57:22 -0700 Subject: [PATCH 7/9] docs: record what this cost in flash, not just what it saved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An engine change that ships on a 3 MB partition should say what it spends. Measured with `just fw-esp32c6-size-check` against origin/main: image 2,863,296 -> 2,873,504 B (+10,208 B, +0.36%) headroom 282,432 -> 272,224 B (CI gate 65,536) Ten kilobytes for the intern table, the route and structural caches, and the static-path memo — headroom stays four times the pre-merge gate. The invalidation guard adds nothing to shipped firmware: it is cfg(debug_assertions) and release-esp32 inherits release. Checked on the linked ELF rather than assumed — the assertion's message string is absent while other resolver strings are present. Plan: 2026-07-31-2225-persist-dataflow-resolution --- ...26-07-31-resolver-persistent-resolution.md | 20 +++++++++++++++++++ docs/debt/s3-frame-cost-scales-per-fixture.md | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/docs/adr/2026-07-31-resolver-persistent-resolution.md b/docs/adr/2026-07-31-resolver-persistent-resolution.md index c8f062534..f8aae3c8b 100644 --- a/docs/adr/2026-07-31-resolver-persistent-resolution.md +++ b/docs/adr/2026-07-31-resolver-persistent-resolution.md @@ -144,6 +144,26 @@ win is in fixed per-frame overhead, not in the per-chain scaling that made this a filed debt. That scaling is still owned by endpoint status. See `docs/debt/s3-frame-cost-scales-per-fixture.md`. +### Flash cost + +The ESP32-C6 is the tight budget (3 MB app partition; see +`docs/adr/2026-07-28-esp32c6-flash-budget.md`), so that is where the cost was +measured — `just fw-esp32c6-size-check`, this branch against `origin/main`: + +| | Image | Headroom | +|---|---|---| +| Baseline | 2,863,296 B | 282,432 B | +| After | 2,873,504 B | 272,224 B | +| | **+10,208 B (+0.36%)** | −10,208 B | + +Ten kilobytes for the intern table, the route and structural caches, and the +static-path memo. Headroom stays four times the 64 KB pre-merge gate. + +The self-checking invalidation guard costs **nothing** in shipped firmware: +it is `#[cfg(debug_assertions)]`, and `release-esp32` inherits `release`. +Confirmed on the linked ELF rather than by inference — the assertion's message +string is absent from it, while other resolver strings are present. + ### Revision stamps age A cached literal or def read keeps the revision it was first stamped with, diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index 720c7a0b7..b5e32c3df 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -110,6 +110,10 @@ nothing else helps, by measurement. dearer than the real RMT driver's, so `endpoint_status_for`'s 46.7% share is inflated there. The emulator attributes cost well; it does not predict fps. + Flash cost on the C6 (the tight budget): **+10,208 B (+0.36%)**, headroom + 282,432 → 272,224 B, still 4× the 64 KB CI gate. The debug-only invalidation + guard is absent from release firmware (verified on the linked ELF). + **The filed shape is only partly fixed.** Per-additional-chain cost went ~9.7 ms → ~8.0 ms (−17%) — most of the win is fixed per-frame overhead, not the per-chain scaling this entry is named for. A 10-fixture project would From bb573201a88cad288a17e550fef180304692ab57 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Sat, 1 Aug 2026 08:02:40 -0700 Subject: [PATCH 8/9] docs(adr): the flash budget gets a running spend ledger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reversible spend is only remembered at one moment — when someone is hunting for flash — and that moment has a forced entry point: the size gate's error message names the budget ADR, and AGENTS.md orders size work to read it first. So that is where deliberate spends get registered, with amounts and levers, rather than in PR threads where they go to die. First entry is the resolver's +10,208 B, recorded honestly: the spend IS the feature, the only cheap slice is the intern reverse-lookup's error-formatting paths (unmeasured, likely single-digit KB — its real holding is heap, not flash), and nobody should spend an afternoon there expecting ten. Plan: 2026-07-31-2225-persist-dataflow-resolution --- docs/adr/2026-07-28-esp32c6-flash-budget.md | 12 ++++++++++++ .../adr/2026-07-31-resolver-persistent-resolution.md | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/docs/adr/2026-07-28-esp32c6-flash-budget.md b/docs/adr/2026-07-28-esp32c6-flash-budget.md index 4a1299b54..c857f4d0d 100644 --- a/docs/adr/2026-07-28-esp32c6-flash-budget.md +++ b/docs/adr/2026-07-28-esp32c6-flash-budget.md @@ -179,6 +179,18 @@ rather than a cliff discovered by a deploy job. - Growth discipline moves to the CI gate. Features that need more than 64 KB of headroom must find their own savings or make an explicit budget case. +## Spend ledger (running) + +Deliberate spends since this ADR landed, so that future size work finds them +here — at the entry point the size gate's error message names — rather than +re-attributing them from a bloat diff. One line per spend: what it bought, +what (if anything) is clawback-able, and what clawing it back would cost. +Append; do not editorialize old entries. + +| Date | Spend | Bought | Clawback lever | +|---|---|---|---| +| 2026-08-01 | **+10,208 B** — resolver persistent resolution (PR #243, `docs/adr/2026-07-31-resolver-persistent-resolution.md`) | −54% engine cycles on the 1-fixture oracle; S3 quad-strips 20→25 fps | **Mostly none** — the spend is the feature; reverting costs the perf win back. The only cheap slice is the intern table's reverse-lookup + error-formatting paths (cycle errors would report ids instead of names): unmeasured, likely single-digit KB flash — its real holding is a few KB of *heap*, not flash. Do not spend an afternoon here expecting 10 KB. | + ## Alternatives Considered - **Swap ESP-NOW for raw IEEE 802.15.4** (~460 KB). Rejected for now — see diff --git a/docs/adr/2026-07-31-resolver-persistent-resolution.md b/docs/adr/2026-07-31-resolver-persistent-resolution.md index f8aae3c8b..3d9f388b7 100644 --- a/docs/adr/2026-07-31-resolver-persistent-resolution.md +++ b/docs/adr/2026-07-31-resolver-persistent-resolution.md @@ -164,6 +164,10 @@ it is `#[cfg(debug_assertions)]`, and `release-esp32` inherits `release`. Confirmed on the linked ELF rather than by inference — the assertion's message string is absent from it, while other resolver strings are present. +The spend and its (mostly absent) clawback lever are registered in the flash +budget's running ledger — `docs/adr/2026-07-28-esp32c6-flash-budget.md` — which +is where size work is directed when the gate trips. + ### Revision stamps age A cached literal or def read keeps the revision it was first stamped with, From 24501ffe10794f0b4430396542ca1bfc85eb6ff5 Mon Sep 17 00:00:00 2001 From: Yona Appletree Date: Sat, 1 Aug 2026 08:49:36 -0700 Subject: [PATCH 9/9] docs(debt): both mechanisms fixed, the shape they were blamed for is not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With #243 and #244 on one image, measured together on d8:3b:da:47:29:70: 4 fixtures 25 fps / 37 ms, 1 fixture 68 fps / 13 ms. The emulator says 21.9×; silicon says +25%, because the endpoint-status half was a virtual-board retry storm the S3 never entered — it opens all four channels on frame one. Decomposed, that is the uncomfortable part: per fixture+output chain 9.7 ms -> 8.0 ms (-17%) fixed per-frame overhead 9.3 ms -> 5.0 ms (-46%) This entry is named for the per-chain term, and the per-chain term barely moved. Ten fixtures goes from 9 fps to 12 — still linear, still unusable, still not something an author can buy down with resolution or LED count. So it stays open, now on the number rather than on either mechanism, and without a fresh guess at the cause: the honest state is that ~8 ms per chain is unattributed, with quad-strips-1fix vs quad-strips as the differential to attribute it. Plan: 2026-07-31-2225-persist-dataflow-resolution --- docs/debt/s3-frame-cost-scales-per-fixture.md | 68 ++++++++++++++++--- 1 file changed, 59 insertions(+), 9 deletions(-) diff --git a/docs/debt/s3-frame-cost-scales-per-fixture.md b/docs/debt/s3-frame-cost-scales-per-fixture.md index 967414eb3..d5784cfc6 100644 --- a/docs/debt/s3-frame-cost-scales-per-fixture.md +++ b/docs/debt/s3-frame-cost-scales-per-fixture.md @@ -11,6 +11,11 @@ related: --- # Frame cost is per-frame resolution machinery, not the shader: ~8.4 ms/fixture flat on the S3 +> **2026-08-01 status:** both mechanisms named below are fixed (PRs #243, +> #244). The per-chain cost they were blamed for moved only 9.7 → 8.0 ms, so +> the linear degradation this entry exists for **remains open** and is now +> unattributed. See the 2026-08-01 incident-log entry. + **Shape** — Measured on the desk ESP32-S3 (2026-07-31, all eight node gates, quad-strips variants, 30-LED strips), then attributed with `lp-cli profile` (emulator, esp32-c6 cycle model). The table below is the ORIGINAL measurement; @@ -52,11 +57,13 @@ frame; per-clock comparison with the C6 is resolver-bound on both chips. Profiles: `profiles/2026-07-31T18-02-07--…-1fix--steady-render--s3-gate-perf-1fix/` and `…18-02-44--…quad-strips--steady-render--s3-gate-perf-4fix/` (report.txt). -**Carrying cost** — Multi-fixture projects degrade linearly (~8.4 ms per -fixture+output chain): 4 fixtures = 20 fps today; ~10 fixtures ≈ single-digit -fps, regardless of how small the fixtures are. Authors cannot buy the cost -down with lower resolution or fewer LEDs, which makes the scaling feel -arbitrary from the outside. +**Carrying cost** — Multi-fixture projects degrade linearly. As filed: +~8.4 ms per fixture+output chain, 4 fixtures = 20 fps, ~10 fixtures ≈ +single-digit fps. **After both fixes (2026-08-01): ~8.0 ms per chain, 4 +fixtures = 25 fps, ~10 fixtures ≈ 12 fps.** The linear term is essentially +unchanged; what improved was fixed per-frame overhead. Authors still cannot +buy the cost down with lower resolution or fewer LEDs, which is what makes +the scaling feel arbitrary from the outside. **Workarounds** — Fewer fixture+output chains (one fixture spanning strips); nothing else helps, by measurement. @@ -173,6 +180,45 @@ nothing else helps, by measurement. the per-chain scaling this entry is named for. A 10-fixture project would still be in the low teens. Endpoint status owns that scaling. +- **2026-08-01** — **Both halves on one image, measured together.** Emulator + (`quad-strips`, steady render): **65,811,630 → 3,011,467 cycles, 21.9×**, and + `[jit] render` is now the largest engine entry (3.6%). Desk S3 + (d8:3b:da:47:29:70, both fixes flashed, project confirmed from the device + heartbeat as `/projects/Quad strips`): + + | Project | Original | Resolver only | Both halves | + |---|---|---|---| + | 4 fixtures | 20 fps / 48 ms | 25 fps / 37.5 ms | **25 fps / 37 ms** | + | 1 fixture | 50 fps / 19 ms | 67 fps / 13.5 ms | **68 fps / 13 ms** | + + The endpoint-status half contributes **nothing on silicon**, exactly as its + own ADR predicted: the S3 opens all four channels on frame one and never + entered the retry storm. The 21.9× is real but is an artifact of the + emulator's virtual board declaring one WS281x channel — worth remembering + before quoting emulator ratios as device wins. + + **⚠️ The shape this entry is named for is NOT fixed.** Decomposing the two + measurements into fixed overhead plus per-chain cost: + + | | per fixture+output chain | fixed per-frame | + |---|---|---| + | Original | 9.7 ms | 9.3 ms | + | Both halves | **8.0 ms** | 5.0 ms | + + Per-chain cost fell only **−17%**; the fixed overhead nearly halved. That is + why 1 fixture improved 36% and 4 fixtures only 25%. Projected 10 fixtures: + **9 fps → 12 fps** — still unusable, still linear. Authors still cannot buy + the cost down with resolution or LED count. + + So both *named mechanisms* are fixed and the carrying cost below is not. + Whatever owns the remaining ~8 ms per chain has not been attributed: the + 1-fixture profile's top entries are memcpy and the allocator (30% combined), + with measured candidates being the shader node's per-frame re-read of its + consumed-slot definitions via `format!`-built paths, per-hit + `ProductionSource` clones, `FixtureNode::render_control`, and the 1.3 ms/ + channel blocking RMT send. **This entry stays open on that basis** — a third + attribution pass on the 8 ms, not a third guess. + **Exit criteria** — A profiled optimization pass that makes resolved bindings and endpoint status persist across frames (invalidate on tree/binding/ hardware change, not per tick), after which frame cost is dominated by actual @@ -185,7 +231,11 @@ the desk S3 as the oracle. failed-open retry storm, fixed by parking sinks on `HwRegistry::generation()` rather than by caching status. Emulator-only in steady state; the S3 never paid it. -- [x] **Desk-S3 re-measurement** — each half measured separately 2026-08-01 - (resolver: 20 → 25 fps at 4 fixtures; endpoint status: flat, as its fix - predicted for silicon). Joint measurement with both halves on one image: - see the closing incident-log entry. +- [x] **Desk-S3 re-measurement** — done 2026-08-01, separately and jointly. + Both halves on one image: 25 fps at 4 fixtures, 68 fps at 1. +- [ ] **The linear term itself** — the two named mechanisms are fixed and + per-chain cost still sits at ~8.0 ms (was ~9.7). This entry stays open + on that number, not on either mechanism. Next step is attribution of + that 8 ms, not another guess; `projects/test/quad-strips-1fix` vs + `quad-strips` under `lp-cli profile` is the differential that isolates + it.