From d3576117da2b823cd88e6759cc057a9c3cafa774 Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Mon, 3 Aug 2026 17:05:56 +0300 Subject: [PATCH] docs(skills): record the measurement traps this investigation hit Every wrong turn in this work came from trusting a number whose validity had not been established: a throttled renderer reporting zero rendering work, a trace category that was never enabled reporting zero long tasks, a scenario that rendered nothing reporting a perfectly quiet profile, and an 'after' measured without a matching 'before', which made a no-op change look like a fix. The performance skill now puts measurement validity ahead of measurement, requires a baseline from the unchanged build through the identical scenario, directs native work to the timeline trace rather than the sampler, requires unvalidated changes to be reverted and recorded as rejected, and says when to stop optimising a path that is already inside budget. It also points at the repository's capture commands. The sync skill gains the invariant behind the cache-thrash loop: an entry acquired during render but protected only after commit is unprotected for the whole render pass, capacity should be a soft target, eviction must not run on the acquisition path, and raising a limit relocates a cliff instead of removing it. --- .../skills/performance-engineering/SKILL.md | 99 +++++++++++++++++++ .agents/skills/sync-state-invariants/SKILL.md | 23 +++++ 2 files changed, 122 insertions(+) diff --git a/.agents/skills/performance-engineering/SKILL.md b/.agents/skills/performance-engineering/SKILL.md index 6bcd7123..eeef8359 100644 --- a/.agents/skills/performance-engineering/SKILL.md +++ b/.agents/skills/performance-engineering/SKILL.md @@ -27,6 +27,34 @@ Do not optimize against a toy fixture when the report provides production scale. ## Workflow +### 0. Trust The Measurement Before Trusting The Number + +A measurement setup that is wrong produces clean, confident, wrong numbers, and +a clean number ends an investigation. Establish validity first. + +**Prove the environment is not throttled.** Chrome stops producing frames and +throttles timers for windows it considers backgrounded or occluded, headless or +not. A capture taken that way reports near-zero rendering work no matter what +the page does. Disable background/occlusion throttling at launch and measure +frame liveness inside the capture. The same applies to any environment that +idles when unobserved. + +**Prove zero is a measurement.** A metric reading zero, absent, or perfectly +quiet is a claim that requires evidence, because a disabled instrument reports +exactly the same thing. `RunTask` only appears under the disabled-by-default +timeline category; a scenario opened for the wrong directory renders nothing at +all. Before believing a quiet result, confirm the instrument fired and the +workload actually ran: assert on an independent signal, such as DOM growth +alongside the application's own render counters. + +**Prove the workload is comparable.** When the stimulus varies in size between +runs, per-second and total figures are not comparable. Normalise by units of +work delivered, and check run-to-run spread on an unchanged build before +attributing any difference to a change. + +Do not report a number whose validity you have not established. State which +validity checks ran. + ### 1. Reproduce And Measure - Reproduce the exact interaction, not a nearby helper in isolation. @@ -37,6 +65,24 @@ Do not optimize against a toy fixture when the report provides production scale. Do not infer a bottleneck from code appearance when a trace or counter can identify it. +**Never accept an "after" without a "before" on the identical scenario and +build.** Measuring a fixed build against a remembered number, a different +scenario, or a nearby baseline proves nothing: the mechanism you changed may +not even execute in the path you measured. Re-run the unchanged build through +the same scenario, however inconvenient the rebuild. Expect to discover that a +plausible fix changes nothing. + +**A sampling profiler cannot explain native work.** Self time attributed to +`(program)` says only that the time was not in interpreted JavaScript. Use the +timeline trace, which names parsing, style recalculation, layout, layerization, +paint, and raster, and reserve the sampler for attributing application code. + +**Reproduction may require production scale you do not have.** A threshold +effect is invisible below its threshold, and a development workspace is usually +below it. When a report will not reproduce, compare the reporter's scale +against yours on the specific dimension the code keys on before concluding the +bug is absent. + Profiling identifies where time is spent; it does not prove behavioral equivalence. Separately verify the applicable state, identity, layout, and lifecycle transitions for every structural optimization. ### 2. Write The Cost Equation @@ -149,6 +195,24 @@ Add a cache only when all are explicit: A cache inside an `O(consumers × entities × candidates)` loop is a mitigation, not automatically a complete fix. +## Repository Tooling + +Three unattended capture commands exist; prefer them over ad-hoc timing code, +and extend them when a scenario is missing rather than measuring by hand. + +| Command | Answers | +|---|---| +| `bun run profile:idle` | What the app does while nobody interacts with it. Supports `--session`, `--tab`, `--then-tab`, `--panel`, `--expand-projects` to reach a specific mounted state, plus `--baseline` and `--budget-*` for regression gating. | +| `bun run profile:session` | What a streaming assistant response costs. Creates a session, dispatches a prompt through the `openchamber session` CLI, and records until the session reports idle. Reports the long-task distribution, a timeline-trace breakdown, running animations, and output-normalised metrics. | +| `bun run profile:browser` | A manually driven capture when the interaction cannot be scripted. | + +Both automated commands fail loudly rather than reporting a clean result when +the renderer was throttled, the trace collected no tasks, or the scenario never +rendered. Keep that property when extending them. + +Measure a production build. A development build's render and bundle behaviour +does not represent what users run. + ## Verification Require both correctness and performance guards: @@ -169,6 +233,32 @@ Require both correctness and performance guards: State what was not measured. Never claim a freeze is fixed from type-check and unit tests alone. +## Revert What You Cannot Measure + +A change that does not move its target metric is not a small win, a safety +improvement, or a cleanup. It is unvalidated complexity, and shipping it under +a performance rationale makes the next investigation harder by implying the +path was already optimised. Revert it and record the hypothesis as rejected. + +This applies to a change whose benefit appears only in reasoning, one measured +against the wrong baseline, and one whose measured scenario turns out to behave +identically without it. + +Report negative results explicitly. "Disabling this removed 40% of the +layerization, and the fix that preserved the visuals did not" is a finding, and +the next person needs it. + +## Know When To Stop + +Compare the remaining cost against the user-facing budget, not against zero. +When the interaction already sits far inside budget, further optimisation of +that path trades real regression risk for an invisible gain, and it displaces +work on the path the user actually reported. Say so and move on. + +Cost that comes from intentional, user-visible behaviour is not waste. Removing +it is a product decision, not a performance fix, and it needs the owner's +agreement rather than a quiet commit. + ## Hotfix Policy Ship a bounded cache-only or local mitigation under deadline pressure only when: @@ -192,9 +282,16 @@ If the interaction remains above budget, do not call the mitigation the complete | "Move it to a worker" | Moving waste changes responsiveness, not total cost or data correctness. | | "Empty means nothing exists" | Empty after failure or partial loading is not authoritative absence. | | "We can optimize later" | Add a scale regression now or the multiplier will return. | +| "The profile is clean" | Prove the instrument fired and the renderer was not throttled. A disabled instrument looks identical to a fast app. | +| "It is much faster now" | Against which baseline, on which build, in which scenario? Re-run the unchanged build. | +| "Most of the time is `(program)`" | The sampler cannot see native work. Read the timeline trace. | +| "It does not reproduce here" | Compare your scale to the reporter's on the dimension the code keys on. | +| "It cannot hurt to keep the change" | An unmeasured change is unvalidated complexity that hides the path from the next investigation. | ## Exit Checklist +- [ ] Measurement validity established: no throttling, instruments confirmed firing, workload comparable. +- [ ] Baseline captured from the unchanged build through the identical scenario. - [ ] Exact interaction and production scale reproduced. - [ ] Cost equation written and dominant multipliers removed. - [ ] Sources of truth, completeness, and invalidation explicit. @@ -205,4 +302,6 @@ If the interaction remains above budget, do not call the mitigation the complete - [ ] Operation-count or repeated-event regression test prevents recurrence. - [ ] Structural optimizations have transition-focused correctness coverage independent of performance measurements. - [ ] When mount topology or activation boundaries change, instrumentation distinguishes those transitions from steady state. +- [ ] Every change retained is justified by a measured difference; unvalidated ones reverted and recorded as rejected. +- [ ] Remaining cost compared against the budget, and stopping justified when inside it. - [ ] Correctness, type, lint, and relevant runtime validations pass. diff --git a/.agents/skills/sync-state-invariants/SKILL.md b/.agents/skills/sync-state-invariants/SKILL.md index 9aa3fec0..c6cd720a 100644 --- a/.agents/skills/sync-state-invariants/SKILL.md +++ b/.agents/skills/sync-state-invariants/SKILL.md @@ -97,6 +97,28 @@ For streaming-frequency work, also load `performance-engineering`. - Key runtime-scoped caches by runtime identity when IDs or paths can collide. - Clean optimistic and local cache state after partial failures. +### Never Evict What Is In Use + +An entry acquired during render but protected only after commit is unprotected +for the whole render pass. Eviction that runs on acquisition therefore disposes +entries that are actively mounting; the next render recreates them in a loading +state, which issues another fetch, which repeats forever. The symptom is an +endless request loop and sawtoothing listeners, heap, and CPU, and it appears +only once live entries outnumber the limit, so it never reproduces on a small +workspace. + +- Define what protects an entry from eviction, and prove that protection is in + place before eviction can observe the entry, not one commit later. +- Treat capacity as a soft target. Overflowing briefly is always cheaper than + evict/recreate cycles; bound the cache with idle-time eviction instead. +- Never run an eviction scan on the acquisition path. Coalesce it into one + deferred pass so a render mounting many entries scans once, not once per + entry. +- Keep explicit lifecycle edges, such as the last consumer releasing an entry, + synchronous. Deferring those changes an observable contract. +- Raising a limit is a workaround, not a fix. It relocates the cliff and hides + the loop from everyone whose workload is smaller than the new number. + ## Persisted Snapshot Ordering When state exists in memory and one or more persistent stores, define an explicit authority and ordering protocol: @@ -136,5 +158,6 @@ Cover the relevant lifecycle, not only static state: - New session lookup assumes SSE already indexed it. - Optimistic data has no shadow entry or rollback. - Snapshot-difference cleanup treats its first startup snapshot as a disappearance event. +- Eviction runs on the acquisition path, or a cache limit is raised in response to a request loop. - Missing or malformed persistence becomes authoritative empty state. - Debounced writes are canceled on owner/lifecycle change without completing against the captured owner or an explicit durability/data-loss contract.