perf(lpc-engine): persist dataflow resolution across frames - #243
Draft
Yona-Appletree wants to merge 11 commits into
Draft
perf(lpc-engine): persist dataflow resolution across frames#243Yona-Appletree wants to merge 11 commits into
Yona-Appletree wants to merge 11 commits into
Conversation
…ing frame cost 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
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
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<SlotPathSegment>::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
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
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
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
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
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
…-1fd675 # Conflicts: # docs/debt/s3-frame-cost-scales-per-fixture.md
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Plan: lp2025/2026-07-31-2225-persist-dataflow-resolution
Path: ~/.photomancer/planning/lp2025/2026-07-31-2225-persist-dataflow-resolution/plan.md
Why
Frame cost on the desk ESP32-S3 was flat ~8.4 ms per fixture+output chain —
the same at 30×1 and 90×90, the same for 10 LEDs and 120.
lp-cli profilesaid the shader was 1.1% of self cycles; the frame was the dataflow
resolver re-resolving the binding graph from cold every tick, with
allocator+memcpy at ~44%.
See
docs/debt/s3-frame-cost-scales-per-fixture.mdanddocs/adr/2026-07-31-resolver-persistent-resolution.md.What changed
Resolution decisions — which binding wins, the merge policy, the expanded
provider list — and def-sourced values (binding literals, authored-def reads)
now persist across frames and are dropped when the graph changes shape, not
when a frame ends. Producer values stay strictly per-frame.
Queries are interned to ids, which is what makes the rest cheap: array-indexed
cache lookups instead of
SlotPathstring compares, aCopycycle stack, anddiscarding a frame's values as a counter bump rather than a drop-and-realloc
storm.
Two invalidation gaps were found and closed on the way:
Engine::add_bindingnever invalidated, and
clear_bindingsleft the resolver holding a bindinggraph that no longer existed.
Measured (1-fixture oracle, steady render, esp32c6 model)
[jit] renderQueryKey::eq,merge_policy_for_consumed_slot,bindings_for_consumed_slot,slot_lookupandVec<SlotPathSegment>::cloneare gone from the top twenty.The 4-fixture project moved only −1.9%, because
HwRegistry::endpoint_status_foris 46.7% of that profile and belongs to thesibling plan. That is the remaining cap on multi-fixture fps.
Scope
This PR owns the dataflow resolver half of the debt entry's exit criteria.
Endpoint status is explicitly out of scope and still open.
Phases
projects/test/quad-strips-1fixoraclebegin_framevsinvalidate_structure),counters, a test pin per invalidation source
oracle test
Status
Draft, pending gate G1 (see the gate message). One acceptance item is
unverified: the desk S3 did not enumerate for
espflash board-info, sothe fps matrix has not been refreshed on silicon.