From 1c6437524411b6a4415d63a2fcfe383a7febd3b3 Mon Sep 17 00:00:00 2001 From: Andrew Fuller Date: Tue, 28 Jul 2026 18:55:46 -0400 Subject: [PATCH 1/4] refactor: consolidate the candle-mode line-overlay presence gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The draw layer's "is the line overlay visible this frame" calculation and the engine's companion "should I build its points" check were separate inline expressions sharing a hardcoded 0.01 threshold — coupling that rots silently, since a change to one without the other makes the builder skip frames the drawer still wants and the overlay vanishes mid-transition with no crash and no failing test. Both now live in draw/lineOverlay.ts, kept free of Skia imports so jest can exercise them directly (same reasoning as draw/pathCache.ts). The extracted lineOverlayPresence is bit-identical to the previous inline expression across 1,002,001 sampled input pairs. lineOverlay.test.ts asserts the load-bearing invariant over a dense grid of both inputs: the builder must return true wherever the drawer would draw. Verified the test actually fails when the gate stops being broader. --- src/draw/__tests__/lineOverlay.test.ts | 112 +++++++++++++++++++++++++ src/draw/index.ts | 18 ++-- src/draw/lineOverlay.ts | 70 ++++++++++++++++ 3 files changed, 190 insertions(+), 10 deletions(-) create mode 100644 src/draw/__tests__/lineOverlay.test.ts create mode 100644 src/draw/lineOverlay.ts diff --git a/src/draw/__tests__/lineOverlay.test.ts b/src/draw/__tests__/lineOverlay.test.ts new file mode 100644 index 0000000..8a7b04c --- /dev/null +++ b/src/draw/__tests__/lineOverlay.test.ts @@ -0,0 +1,112 @@ +import { + lineOverlayPresence, + shouldBuildLineOverlay, + LINE_OVERLAY_MIN_PRESENCE, +} from '../lineOverlay'; + +// A dense grid over both inputs, including every threshold either function +// mentions and the values immediately on each side of them. +const GRID = [ + 0, 0.0001, 0.005, 0.00999, 0.01, 0.01001, 0.02, 0.05, 0.1, 0.2, 0.2154, + 0.2155, 0.3, 0.5, 0.7, 0.78, 0.7846, 0.79, 0.9, 0.98, 0.98999, 0.99, 0.99001, + 0.999, 1, +]; + +describe('lineOverlayPresence', () => { + it('is 0 in steady candle mode (no line mode, reveal complete)', () => { + expect(lineOverlayPresence(0, 1)).toBe(0); + }); + + it('is 1 in full line mode', () => { + expect(lineOverlayPresence(1, 1)).toBe(1); + }); + + it('tracks lineModeProg once the reveal has finished', () => { + for (const p of GRID) expect(lineOverlayPresence(p, 1)).toBeCloseTo(p); + }); + + it('cubes the reveal contribution in candle mode', () => { + // reveal=0.5 => inv=0.5 => 0.5^3 = 0.125, not 0.5. + expect(lineOverlayPresence(0, 0.5)).toBeCloseTo(0.125); + }); + + it('uses a linear reveal contribution in full line mode', () => { + // At lineModeProg >= 0.99 the reveal term is 1 - reveal, but the max with + // lineModeProg dominates unless reveal is very low. + expect(lineOverlayPresence(0.99, 0)).toBeCloseTo(1); + expect(lineOverlayPresence(0.99, 1)).toBeCloseTo(0.99); + }); + + it('stays within [0, 1] across the grid', () => { + for (const p of GRID) { + for (const r of GRID) { + const lp = lineOverlayPresence(p, r); + expect(lp).toBeGreaterThanOrEqual(0); + expect(lp).toBeLessThanOrEqual(1); + } + } + }); + + it('never decreases as lineModeProg rises at a fixed reveal', () => { + for (const r of GRID) { + let prev = -Infinity; + for (const p of GRID) { + const lp = lineOverlayPresence(p, r); + expect(lp).toBeGreaterThanOrEqual(prev); + prev = lp; + } + } + }); +}); + +describe('shouldBuildLineOverlay', () => { + it('skips the build in steady candle mode', () => { + expect(shouldBuildLineOverlay(0, 1)).toBe(false); + }); + + it('builds while any line-mode transition is in progress', () => { + expect(shouldBuildLineOverlay(0.5, 1)).toBe(true); + expect(shouldBuildLineOverlay(1, 1)).toBe(true); + }); + + it('builds for the whole reveal, including its tail', () => { + expect(shouldBuildLineOverlay(0, 0)).toBe(true); + expect(shouldBuildLineOverlay(0, 0.5)).toBe(true); + expect(shouldBuildLineOverlay(0, 0.999)).toBe(true); + }); + + // THE load-bearing test. If either function is changed such that the + // engine could skip building an array the drawer still wants, the overlay + // silently disappears mid-transition — a bug with no test failure and no + // crash, visible only on a device, only during an animation. This asserts + // the one relationship that makes the optimization safe. + it('is true wherever the drawer would draw (never skips a wanted frame)', () => { + for (const p of GRID) { + for (const r of GRID) { + const drawerWouldDraw = + lineOverlayPresence(p, r) > LINE_OVERLAY_MIN_PRESENCE; + if (drawerWouldDraw) { + expect({ + lineModeProg: p, + chartReveal: r, + builds: shouldBuildLineOverlay(p, r), + }).toEqual({ lineModeProg: p, chartReveal: r, builds: true }); + } + } + } + }); + + // Guards the other direction: the gate has to actually skip something, or + // the optimization is a no-op and the complexity isn't paying for itself. + it('does skip real frames — steady candle mode is entirely excluded', () => { + const skipped = GRID.flatMap((p) => + GRID.filter((r) => !shouldBuildLineOverlay(p, r)).map((r) => [p, r]) + ); + expect(skipped.length).toBeGreaterThan(0); + // Everything skipped must be reveal-complete and out of line mode. + for (const [p, r] of skipped) { + expect(r).toBe(1); + expect(p).toBeLessThanOrEqual(LINE_OVERLAY_MIN_PRESENCE); + } + }); +}); diff --git a/src/draw/index.ts b/src/draw/index.ts index 0e63726..178365c 100644 --- a/src/draw/index.ts +++ b/src/draw/index.ts @@ -13,6 +13,7 @@ import type { Ctx2D } from './canvas2d'; import { drawGrid, type GridState } from './grid'; import type { GridLayerSlot } from './gridLayer'; import { drawLine } from './line'; +import { lineOverlayPresence, LINE_OVERLAY_MIN_PRESENCE } from './lineOverlay'; import { createLineCacheSlot, type LineCacheRef, @@ -727,13 +728,10 @@ export function drawCandleFrame( const fullLineMode = opts.lineModeProg >= 0.99; // Line presence (lp): during the reveal, the morph line smoothly - // transforms from the loading squiggly into data positions. In candle - // mode it fades much faster (cubed) so candles become dominant early - // and the morphing line never looks like a "line chart." - const revealLine = fullLineMode - ? 1 - reveal - : (1 - reveal) * (1 - reveal) * (1 - reveal); - const lp = Math.max(opts.lineModeProg, revealLine); + // transforms from the loading squiggly into data positions. See + // draw/lineOverlay.ts — `engineStep` gates *building* the overlay's point + // array on the companion predicate there, so the two must stay in step. + const lp = lineOverlayPresence(opts.lineModeProg, reveal); // colorBlend: when reveal drives lp, force grey (loading squiggly color). // When the user's lineModeProg drives lp, use accent color. @@ -755,7 +753,7 @@ export function drawCandleFrame( // 2. Line — morph line that transforms from loading squiggly into data. // Returns pts for dot position. let linePts: [number, number][] | undefined; - if (lp > 0.01 && opts.lineVisible.length >= 2) { + if (lp > LINE_OVERLAY_MIN_PRESENCE && opts.lineVisible.length >= 2) { const scrubX = opts.scrubAmount > 0.05 ? opts.hoverX : null; ctx.save(); ctx.globalAlpha = lp; @@ -800,7 +798,7 @@ export function drawCandleFrame( } // Accent-colored dash line (fades in with lineModeProg) // Skip when fully in line mode — drawLine draws its own morphing dash - if (lp > 0.01 && !fullLineMode) { + if (lp > LINE_OVERLAY_MIN_PRESENCE && !fullLineMode) { const dashY = layout.toY(closeSource.close); if (dashY >= pad.top && dashY <= h - pad.bottom) { ctx.save(); @@ -845,7 +843,7 @@ export function drawCandleFrame( ctx.save(); ctx.clipRect(pad.left - 1, pad.top, chartW + 2, chartH); - const accentCol = lp > 0.01 ? palette.line : undefined; + const accentCol = lp > LINE_OVERLAY_MIN_PRESENCE ? palette.line : undefined; if (opts.morphT >= 0 && revealOld.length > 0) { ctx.globalAlpha = (1 - opts.morphT) * candleAlpha; drawCandlesticks( diff --git a/src/draw/lineOverlay.ts b/src/draw/lineOverlay.ts new file mode 100644 index 0000000..b3d2b4f --- /dev/null +++ b/src/draw/lineOverlay.ts @@ -0,0 +1,70 @@ +/** + * The candle-mode line overlay's presence gate, in one place. + * + * Candle mode can draw a line on top of the candles in two situations: the + * user is transitioning to/from line mode (`lineModeProg`), or the reveal + * morph is still running and the line is what the loading squiggly morphs + * out of (`chartReveal`). Two different modules need to agree about this: + * + * - `drawCandleFrame` (draw/index.ts) decides whether to *draw* it, from + * `lineOverlayPresence` — which is also the alpha the overlay is drawn at. + * - `engineStep` (engine/step.ts) decides whether to *build* the point array + * the drawer would consume, from `shouldBuildLineOverlay`. + * + * They were previously separate inline expressions sharing a hardcoded + * `0.01`, which is exactly the kind of coupling that rots: if the drawer's + * threshold moved and the builder's didn't, the builder would start + * skipping frames the drawer still wanted, and the overlay would vanish + * during a transition. Both live here now, and `lineOverlay.test.ts` + * asserts the invariant that keeps them safe — see `shouldBuildLineOverlay`. + * + * Deliberately free of Skia imports so jest can exercise it directly: + * `draw/index.ts` pulls in the native binding at import time, which the + * draw-layer unit tests can't load (same reasoning as `draw/pathCache.ts`, + * and the local `DASH_4_4` in `draw/line.ts`). + */ + +/** Below this presence the overlay contributes nothing and isn't drawn. */ +export const LINE_OVERLAY_MIN_PRESENCE = 0.01; + +/** + * How present the line overlay is this frame, in [0, 1] — used directly as + * its draw alpha. The reveal contribution is cubed in candle mode so the + * candles become dominant early and the morphing line never reads as a + * "line chart"; in full line mode it's linear, because there the line *is* + * the chart. + */ +export function lineOverlayPresence( + lineModeProg: number, + chartReveal: number +): number { + 'worklet'; + const fullLineMode = lineModeProg >= 0.99; + const inv = 1 - chartReveal; + const revealLine = fullLineMode ? inv : inv * inv * inv; + return Math.max(lineModeProg, revealLine); +} + +/** + * Whether `engineStep` should build the overlay's point array this frame. + * + * **Invariant:** this must be true whenever `lineOverlayPresence(...) > + * LINE_OVERLAY_MIN_PRESENCE` — i.e. it may only ever skip work the drawer + * was going to ignore anyway, never the reverse. It is deliberately + * *broader* than the drawer's test rather than an exact mirror of it: + * `chartReveal < 1` covers the entire reveal, including the tail where the + * cubed reveal term has already decayed under the threshold. Building a + * handful of unnecessary frames at the end of a reveal is free; skipping + * one the drawer wanted is a visible dropout. + * + * `lineOverlay.test.ts` asserts this over a dense grid of both inputs, so a + * future change to either function that breaks the relationship fails a + * test instead of shipping. + */ +export function shouldBuildLineOverlay( + lineModeProg: number, + chartReveal: number +): boolean { + 'worklet'; + return lineModeProg > LINE_OVERLAY_MIN_PRESENCE || chartReveal < 1; +} From 4d9de69a2ea273206de4cf74b286433b826c8041 Mon Sep 17 00:00:00 2001 From: Andrew Fuller Date: Tue, 28 Jul 2026 18:55:59 -0400 Subject: [PATCH 2/4] perf: skip building the candle-mode line overlay when it isn't drawn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In steady candle mode — the common case, held for as long as the chart is on screen — drawCandleFrame discards the line overlay's point array, because the overlay is only drawn during a line-mode or reveal transition. The engine built it anyway, every frame at 60fps: one object per visible candle, or one per visible tick plus a closeRefs array on the density-blend branch. Gated on shouldBuildLineOverlay (draw/lineOverlay.ts), which is deliberately broader than the drawer's own test, so it can only ever skip frames the drawer was going to ignore. lineSmoothValue stays unconditional in both branches — it's a scalar, and leaving it there keeps the value's derivation byte-for-byte unchanged. Measured no CPU delta on a Debug simulator build (before 65.4%, after 66.9%, after-recheck 64.6% — the after arms bracket the before arm). This removes provably dead work rather than making live work faster; the win is not visible at that measurement floor. Verified on the iOS sim in steady candle mode, line-mode-within-candles, and the Line tab. --- src/engine/step.ts | 102 +++++++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 37 deletions(-) diff --git a/src/engine/step.ts b/src/engine/step.ts index 4127f0b..c71f7b5 100644 --- a/src/engine/step.ts +++ b/src/engine/step.ts @@ -20,6 +20,7 @@ import { drawEdgeFade, type MultiSeriesEntry, } from '../draw'; +import { shouldBuildLineOverlay } from '../draw/lineOverlay'; import { drawLoading } from '../draw/loading'; import { drawEmpty } from '../draw/empty'; import type { EngineConfigStep } from './types'; @@ -86,6 +87,13 @@ function dataSourceOf(useStash: boolean, hasPausedSnapshot: boolean): number { * lookup miss below (mirrors `EMPTY_CANDLES` in useLivelineEngine.ts). */ const EMPTY_MULTI_POINTS: LivelinePoint[] = []; +/** Stable empty array handed to `drawCandleFrame` as `lineVisible` on the + * frames where the line overlay provably isn't drawn — see the + * `wantLineVisible` gate in the candle pipeline below. Separate from + * `EMPTY_MULTI_POINTS` purely so neither const's purpose has to be inferred + * from the other's call sites; both are read-only. */ +const EMPTY_LINE_POINTS: LivelinePoint[] = []; + /** * Look up a multi-series entry's data points. `series` is either a live * `cfg.multiSeries` entry (no `.data` — its points live in `multiData`, @@ -869,46 +877,64 @@ export function engineStep( // is still at ~30% opacity, causing a visible shape jump. let lineVisible: LivelinePoint[]; let lineSmoothValue: number; + // Is the line overlay actually going to be drawn this frame? In steady + // candle mode — the common case, held for as long as the chart is on + // screen — it isn't, and the arrays built below are per-frame garbage the + // drawer immediately ignores: one object per visible candle on the else + // branch, one per visible tick (plus `closeRefs`) on the density-blend + // branch, every frame at 60fps. The predicate and the drawer's own + // presence test live together in draw/lineOverlay.ts, which documents + // (and lineOverlay.test.ts enforces) the invariant that this can only + // ever skip frames the drawer was going to ignore anyway. + // + // `lineSmoothValue` stays unconditional in both branches: it's a scalar, + // and computing it here rather than under the gate keeps this change to + // the allocation and leaves the value's derivation byte-for-byte as it was. + const wantLineVisible = shouldBuildLineOverlay(lineModeProg, chartReveal); if ( effectiveLineData && effectiveLineData.length > 0 && (lineDensityProg > 0.01 || lineModeProg > 0.05) ) { - // Density transition: blend candle-close values toward tick values - const closeRefs: { t: number; v: number }[] = []; - for (const c of drawCandles) { - closeRefs.push({ t: c.time + displayCandleWidth / 2, v: c.close }); - } - if (drawLive) closeRefs.push({ t: now, v: drawLive.close }); - - lineVisible = []; - let refIdx = 0; - for (const pt of effectiveLineData) { - if (pt.time < leftEdge || pt.time > rightEdge) continue; - while ( - refIdx < closeRefs.length - 2 && - closeRefs[refIdx + 1]!.t < pt.time - ) { - refIdx++; + if (!wantLineVisible) { + lineVisible = EMPTY_LINE_POINTS; + } else { + // Density transition: blend candle-close values toward tick values + const closeRefs: { t: number; v: number }[] = []; + for (const c of drawCandles) { + closeRefs.push({ t: c.time + displayCandleWidth / 2, v: c.close }); } - let interpClose: number; - if (closeRefs.length === 0) { - interpClose = pt.value; - } else if (closeRefs.length === 1 || pt.time <= closeRefs[0]!.t) { - interpClose = closeRefs[0]!.v; - } else if (refIdx >= closeRefs.length - 1) { - interpClose = closeRefs[closeRefs.length - 1]!.v; - } else { - const a = closeRefs[refIdx]!; - const b = closeRefs[refIdx + 1]!; - const span = b.t - a.t; - const frac = - span > 0 ? Math.max(0, Math.min(1, (pt.time - a.t) / span)) : 0; - interpClose = a.v + (b.v - a.v) * frac; + if (drawLive) closeRefs.push({ t: now, v: drawLive.close }); + + lineVisible = []; + let refIdx = 0; + for (const pt of effectiveLineData) { + if (pt.time < leftEdge || pt.time > rightEdge) continue; + while ( + refIdx < closeRefs.length - 2 && + closeRefs[refIdx + 1]!.t < pt.time + ) { + refIdx++; + } + let interpClose: number; + if (closeRefs.length === 0) { + interpClose = pt.value; + } else if (closeRefs.length === 1 || pt.time <= closeRefs[0]!.t) { + interpClose = closeRefs[0]!.v; + } else if (refIdx >= closeRefs.length - 1) { + interpClose = closeRefs[closeRefs.length - 1]!.v; + } else { + const a = closeRefs[refIdx]!; + const b = closeRefs[refIdx + 1]!; + const span = b.t - a.t; + const frac = + span > 0 ? Math.max(0, Math.min(1, (pt.time - a.t) / span)) : 0; + interpClose = a.v + (b.v - a.v) * frac; + } + const blended = + interpClose + (pt.value - interpClose) * lineDensityProg; + lineVisible.push({ time: pt.time, value: blended }); } - const blended = - interpClose + (pt.value - interpClose) * lineDensityProg; - lineVisible.push({ time: pt.time, value: blended }); } const smoothTick = s.lineTickSmoothInited @@ -920,10 +946,12 @@ export function engineStep( s.lineSmoothClose + (smoothTick - s.lineSmoothClose) * lineDensityProg; } else { // Candle-close resolution — no live tip; drawLine appends one at toX(now) - lineVisible = drawCandles.map((c) => ({ - time: c.time + displayCandleWidth / 2, - value: c.close, - })); + lineVisible = wantLineVisible + ? drawCandles.map((c) => ({ + time: c.time + displayCandleWidth / 2, + value: c.close, + })) + : EMPTY_LINE_POINTS; lineSmoothValue = s.lineSmoothInited ? s.lineSmoothClose : (drawLive?.close ?? drawCandles[drawCandles.length - 1]?.close ?? 0); From af120a922d69b023b34e11e459745cdbf6cc722b Mon Sep 17 00:00:00 2001 From: Andrew Fuller Date: Tue, 28 Jul 2026 18:56:09 -0400 Subject: [PATCH 3/4] docs: changelog entries and a maintainability handoff plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PLAN_MAINT.md is a survey of all of src/ (47 files, ~15.5k lines), written because the recent perf batches optimized aggressively within existing structures without revisiting the structures themselves. Seven ranked items with verified line numbers, an execution order, and standing rules for whoever picks it up. Follows the PLAN.md / PLAN_PERF.md house style. Notable: item 3 is a real (small) leak found by the survey — EngineState has eight per-series Maps, four are pruned by hand-written copy-pasted loops, and seriesAlpha has the same lifecycle but was never added, so it grows unboundedly as series ids churn. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 19 +++ PLAN_MAINT.md | 348 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 367 insertions(+) create mode 100644 PLAN_MAINT.md diff --git a/CHANGELOG.md b/CHANGELOG.md index eab9a03..8a8851a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,25 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Changed + +- **Candle mode skips building an unused line-overlay array** — in steady + candle mode the engine was assembling the line-mode point array (one + object per visible candle, or per visible tick during a density blend) + on every frame, which the draw layer then discarded because the line + overlay isn't drawn unless a line-mode or reveal transition is active. + It's now built only when it will actually be used. Internal only, no + visual change. Measured no CPU delta on a Debug simulator build — this + removes provably dead work rather than making live work faster. +- **Line-overlay presence logic consolidated** — the draw layer's "is the + candle-mode line overlay visible this frame" calculation and the engine's + companion "should I build its points" check previously lived as separate + inline expressions sharing a hardcoded threshold. Both now live in + `draw/lineOverlay.ts`, with a test asserting the invariant that keeps them + safe (the builder may only ever skip frames the drawer would ignore). + Behavior is unchanged — the extracted presence function is bit-identical + to the previous inline expression across the full input domain. + ## [0.2.0] - 2026-07-27 ### Added diff --git a/PLAN_MAINT.md b/PLAN_MAINT.md new file mode 100644 index 0000000..7ca7d92 --- /dev/null +++ b/PLAN_MAINT.md @@ -0,0 +1,348 @@ +# react-native-liveline — Maintainability hardening + +**STATUS: NOT STARTED (written 2026-07-28).** Nothing in this doc has been +implemented. Seven items, ranked below; work them in the order given in +"Suggested execution order" at the bottom, not top-to-bottom. + +**Read this first when resuming work.** This is the third standalone +workstream doc, alongside: + +- `PLAN.md` — the original web→RN port. Fully shipped as of v0.1.0. Don't + relitigate anything marked done there. +- `PLAN_PERF.md` — the react-native-graph comparison + perf hardening. + Complete as of 2026-07-18; a much larger follow-on perf batch landed + 2026-07-26/27 and shipped as v0.2.0. + +This doc is **not** about performance. Several items below would touch +per-frame hot paths, and the rule for every one of them is: *do not regress +the perf work that already landed.* Where an item has an allocation cost, +it's stated explicitly. + +## Preceding work (context for the line numbers below) + +A perf session on 2026-07-27/28 produced two changes. Both were +typecheck/lint/test green and visually verified on the iOS sim, and both +**measured no CPU delta** (three arms, Candles tab, Debug sim: before 65.4%, +after 66.9%, after-recheck 64.6% — the after arms bracket the before arm, so +any effect is below the measurement floor there). + +- **Landed:** a candle-mode dead-work gate in `engine/step.ts` — the + `wantLineVisible` guard around the two `lineVisible` builders, skipping an + array the drawer was already discarding in steady candle mode. Every line + number in this doc is verified against the tree *with* this applied. +- **Deliberately deferred:** pooled decimation scratch (`DecimateScratch` in + `math/decimate.ts`, threaded through `engine/state.ts` → `draw/line.ts` → + `draw/index.ts`, with six tests). Written, green, then reverted — see + item 4 for why, and pick it back up there. + +## How this doc came to be + +A maintainability survey of all of `src/` (47 files, ~15.5k lines) run on +2026-07-28, prompted by the observation that the 2026-07-26/27 perf batch +optimized aggressively *within* existing structures without revisiting the +structures themselves. Method: structural mapping of the two largest files, +a `difflib` similarity measurement between the three pipelines in +`engine/step.ts`, a scan for functions with ≥9 positional parameters, a +source-file-to-test-file coverage map, and targeted greps for copy-pasted +idioms. Item 3 below is a real (small) bug found by that survey, not a +hypothetical. + +## Findings & plan, ranked by impact + +### 1. `engineStep` is a ~1700-line function containing three inline pipelines + +**Problem:** `src/engine/step.ts:120` (`engineStep`) runs to the end of an +1818-line file. Inside one function body: + +- shared preamble: ~120–402 +- CANDLE MODE PIPELINE: 403–1054 +- MULTI-SERIES LINE MODE PIPELINE: 1055–1516 +- LINE MODE PIPELINE: 1517–end + +Measured with `difflib.SequenceMatcher` over the multi-series pipeline (466 +lines) against the single-line pipeline (274 lines): **132 identical lines, +48% of the smaller pipeline.** The largest identical run is **53 consecutive +lines** — the `updateRange` call through the `layout` literal through the +grid-layer sync — at `step.ts:1339` and `step.ts:1607`. Also duplicated: an +18-line reveal/empty-overlay tail block, and the 9-line +`updateWindowTransition` call. + +**Plan:** +- Extract `stepCandle`, `stepMulti`, `stepLine` into + `engine/pipelines/candle.ts` / `multi.ts` / `line.ts`. Each takes the + already-computed preamble results plus `(ctx, cfg, s, layout, …)` and + returns the same `StepResult` shape `engineStep` returns today. +- Extract the 53-line shared run as `prepareChartFrame()` in + `engine/frame.ts` — it is `updateRange` → destructure → `makeLayout` + (item 2) → grid-layer sync. The one real difference between the three + call sites is the `dt` passed to `updateGridLayer` (candle mode passes + `pausedDt`, the other two pass `dt`), so that's a parameter, not a + divergence to paper over. +- `engineStep` itself becomes: preamble → dispatch to one of three. +- **Do this last.** It is the highest-value item and the highest-risk one; + item 6 (tests) is the prerequisite that makes it safe. + +**Verification:** this is a pure refactor — no behavior change is intended. +Beyond typecheck/lint/test, run the iOS sim check across all three modes +(Line tab, Candles tab, Multi tab) and compare against screenshots taken +before starting. Also re-run the Part 2 CPU A/B from the `run-and-profile` +skill: a pure refactor must not move CPU, and if it does, something was +changed that shouldn't have been. + +### 2. The `layout` object is constructed byte-identically three times + +**Problem:** `step.ts:758`, `:1360`, `:1628` — the same 15-line +`const layout: ChartLayout = { … }` literal, including both the `toX` and +`toY` closures, three times over. Verified byte-identical. + +**Plan:** extract `makeLayout(w, h, pad, chartW, chartH, leftEdge, +rightEdge, rangeResult): ChartLayout` into `engine/helpers.ts` (or +`engine/frame.ts` if item 1 lands first — it's a natural part of +`prepareChartFrame`). Zero risk, mechanical, and it removes the possibility +of the three drifting apart silently. + +**Note:** keep the two closures as closures. They're allocated once per +frame per chart, which is already the case today; do not try to "optimize" +them into a shared object with mutable captured state. + +### 3. Copy-pasted per-series map pruning — has already caused a leak + +**Problem:** `step.ts:1098-1118` prunes per-series Maps when series are +removed, as four near-identical hand-written loops (`s.displayValues`, +`s.lineCaches`, `s.multiVisibleScratch`, `s.multiSeriesEntryScratch`). + +`EngineState` declares **eight** per-series `Map` fields +(`state.ts:49,50,56,97,105,128,172,173`). `s.seriesAlpha` (`state.ts:50`) +has exactly the same lifecycle as the four that are pruned — written per +series at `step.ts:1167`, read at `:1160` and `:1259` — but it is **never +pruned or cleared anywhere in the codebase** (confirmed: the only +references are its declaration, its construction at `state.ts:253`, and +those three uses). + +So a long-lived chart whose series ids churn accumulates dead `seriesAlpha` +entries forever. The impact is small — one number per dead id, and ids are +stable across renders in normal use — but it is a real unbounded-growth bug, +and it exists *because* adding a fifth map means remembering to hand-write a +fifth loop. + +**Plan:** +- Add `pruneByIds(currentIds: Set, maps: Map[])` + as a worklet helper (probably `engine/state.ts`, next to the state it + serves). +- Better: give `EngineState` a `perSeriesMaps` accessor returning the array + of maps that must be pruned together, so the registration lives next to + the field declarations and a new map is one line in one place. +- Add `seriesAlpha` to that set — **this is the actual bug fix.** +- Audit the other three unpruned per-series maps while you're there + (`smoothValuesScratch` is cleared per frame so it's fine; + `lastMultiStashRevs`/`lastMultiStashData` are pruned separately at + `step.ts:310-312`; confirm that's genuinely sufficient rather than + assuming it). + +**Verification:** unit-testable without the worklet runtime — construct an +`EngineState`, populate the maps with ids, prune against a smaller id set, +assert every map shrank. This is the cheapest real test in this whole doc. + +### 4. Positional-parameter soup — 11 functions with ≥9 parameters + +**Problem:** a scan of `src/` (excluding tests) for functions with ≥9 +positional parameters: + +| params | location | function | +|---|---|---| +| 19 | `engine/candleHelpers.ts:126` | `updateCandleWindowTransition` | +| 16 | `draw/line.ts:205` | `drawLine` | +| 15 | `draw/candlestick.ts:140` | `drawCandlesticks` | +| 14 | `engine/candleHelpers.ts:59` | `updateCandleRange` | +| 13 | `engine/step.ts:120` | `engineStep` | +| 12 | `engine/helpers.ts:104` | `updateRange` | +| 12 | `engine/helpers.ts:183` | `updateHoverState` | +| 11 | `draw/line.ts:166` | `paintLineCurve` | +| 11 | `engine/badge.ts:36` | `drawBadge` | +| 11 | `engine/helpers.ts:43` | `updateWindowTransition` | +| 9 | `draw/empty.ts:19` | `drawEmpty` | + +Many of these take long runs of consecutive `number` arguments, so a +transposed pair type-checks silently and fails only visually, at runtime, on +one device mode. + +**This item already blocked a change once.** A pooled-decimation-scratch +perf change (see "Preceding work" above) needed one more value inside +`drawLine`, which meant a 17th positional parameter and a matching edit at +all three call sites. It was written, tested, and green — then reverted +specifically because paying that cost for an unmeasurable win was the wrong +trade while the signature is this shape. **Land this item, then re-do the +scratch pooling as a field on the options object**, where it costs nothing +structurally. That's the intended sequencing, not an abandoned idea. + +**The fix already exists in this codebase and is applied inconsistently:** +`draw/index.ts` defines `DrawOptions` (`:135`), `MultiSeriesDrawOptions` +(`:422`), and `CandleDrawOptions` (`:675`) and passes options objects to +`drawFrame`/`drawMultiFrame`/`drawCandleFrame`. The functions those three +*call* are the ones still taking positional soup. + +**Plan:** +- Convert the table above to options objects, following the existing + `*Options` interface naming and the existing convention of documenting + each field on the interface. +- Start with `updateCandleWindowTransition` (19) and `drawLine` (17) — + worst offenders, biggest readability win. +- **Allocation note, read before starting:** every one of these is called + at most a couple of times per frame, not per point. An options object per + call is roughly ten small short-lived objects per frame, against a shim + that already pools far more than that per frame. This is not a hot-loop + concern. It *would* be if any of these moved inside a per-point loop — + none are, and none should be. +- Do **not** convert per-point helpers (`math/lerp.ts`, `math/color.ts`, + the spline emit functions) to options objects. Those genuinely are hot. + +### 5. The two cross-frame path caches have drifted apart + +**Problem:** `draw/lineCache.ts:68-80` and `draw/candleCache.ts:84-97` +declare overlapping flat invalidation-key fields with inconsistent names — +`kMin`/`kMax` in one, `kMinVal`/`kMaxVal` in the other, for the same +`layout.minVal`/`layout.maxVal`. Eight fields are common to both +(`kDataSource`, `kWindow`, `kH`, `kPadTop`, `kPadBottom`, `kChartW`, plus +the min/max pair). + +`draw/pathCache.ts` already established the precedent for factoring shared +cache machinery out of the two cache modules (`CachePath`, `ensured`) and +explains in its docblock why it stays free of Skia imports — so jest can +exercise cache logic with fake path recorders. That reasoning extends +cleanly to the key comparison. + +**Plan:** +- Add a `LayoutKey` struct + `writeLayoutKey(key, layout)` + + `layoutKeyMatches(key, layout)` to `draw/pathCache.ts`. +- Embed it in both slot types, leaving each cache's *own* data-identity + fields (`kDataRev`/`kLen`/`kFirstT`/… for the line, `kCandlesRev`/ + `kClosedCount`/`kRadius`/… for candles) where they are. +- Keep the comparison allocation-free and field-by-field — read the + docblock on `LineCacheSlot`'s key before touching this; the "flat numbers + only, compared field-by-field so a per-frame check allocates nothing" + property is deliberate and load-bearing. +- Both cache modules already have real test files + (`lineCache.test.ts` 667 lines, `candleCache.test.ts` 433 lines) — they + should keep passing unchanged. If they don't, the refactor changed + behavior. + +### 6. Test coverage is inverted — and it is the prerequisite for item 1 + +**Problem:** only 8 of 47 source files have tests, and the coverage is +concentrated in the small, already-safe modules: + +| file | lines | tests | +|---|---|---| +| `engine/step.ts` | 1818 | none | +| `draw/index.ts` | 1036 | none | +| `draw/canvas2d.ts` | 916 | none | +| `Liveline.tsx` | 642 | none | +| `useLivelineEngine.ts` | 589 | none | +| `draw/candlestick.ts` | 671 | yes | +| `engine/helpers.ts` | 253 | **none** | +| `engine/candleHelpers.ts` | 194 | **none** | +| `engine/quiescence.ts` | 87 | **none** | + +The three bolded ones matter most: they are **pure functions** with no Skia +dependency, they are the shared logic all three pipelines call, and they are +currently untested. `quiescence.ts` in particular is an 87-line pure +predicate that gates whether the engine skips picture re-recording at all — +a silent regression there is a perf cliff with no visual symptom. + +**Plan:** +- Write `engine/__tests__/helpers.test.ts` covering `updateRange`, + `updateWindowTransition`, `updateHoverState`. Follow the existing style + in `math/__tests__/math.test.ts` — plain function calls, no worklet + runtime, no Skia. +- Write `engine/__tests__/quiescence.test.ts` for `isQuiescentCandidate`: + assert each break condition independently forces `false`, and that the + all-static case returns `true`. +- Write `engine/__tests__/candleHelpers.test.ts` for `updateCandleRange`, + `updateCandleWindowTransition`, `candleAtX`. +- **Note what jest cannot reach:** the worklet runtime. See + `.claude/skills/run-and-profile` — the SharedValue-closure failure + documented there shipped green tests, clean tsc, clean eslint, and + passing screenshots, and still failed on the second frame in a real + launch. Tests are the safety net for *logic*, not for the Reanimated + boundary. An on-device check is still mandatory after item 1. + +### 7. `Liveline.tsx` writes the same pill bar three times + +**Problem:** `Liveline.tsx:408-580` — the window-selector pills, the +line/candle mode toggle, and the series-toggle chips are three +near-identical blocks of the same shape: a `` wrapping a +`` (two of the three) and a `.map()` of ``s. +The bar `` opens at `:413`, `:465`, and `:508` with the same four +computed style values each time. + +Above them, seven style scalars are recomputed inline on every render: +`activeColor`, `inactiveColor`, `barBg`, `barRadius`, `barPadding`, +`barGap`, `btnRadius` (`:357-368`). The file carries a top-level +`/* eslint-disable react-native/no-inline-styles */` to accommodate this. + +**Plan:** +- Extract a `` component (own file, `src/components/PillBar.tsx` + or similar — there is no components dir yet, so this establishes one) + taking the bar chrome + children, owning the `SlidingIndicator`. +- Extract `useBarStyle(windowStyle, isDark)` returning the seven scalars + memoized on its two inputs. +- The three call sites become the three distinct bits: which items, which + is active, what each renders. +- This should let the file-level eslint-disable narrow or disappear. If it + can't, say so in the commit message rather than leaving it silently. + +**Note:** this is the only item touching rendered UI rather than engine +internals. Screenshot the three control bars in both themes and all three +`windowStyle` values (`default` / `rounded` / `text`) before and after — +the styling branches on all of those. + +## Suggested execution order + +1. **#6 tests** (helpers, quiescence, candleHelpers) — cheap, no + behavior change, and the safety net everything else leans on. +2. **#3 map pruning** — small, self-contained, and ships an actual bug fix + (`seriesAlpha`). Unit-testable immediately after #6 establishes the + `engine/__tests__` directory. +3. **#2 `makeLayout`** — mechanical, five minutes, removes three-way drift + risk. +4. **#5 cache key unification** — contained to two modules that both + already have thorough tests. +5. **#4 options objects** — mostly mechanical but touches many call sites; + do it in separate commits per function, not one sweep. +6. **#7 `PillBar`** — independent of everything above; can be done at any + point by someone who'd rather work in the React layer than the engine. +7. **#1 pipeline extraction** — last, with #6 and #2 behind you. + +Items #4, #5, and #7 are mutually independent and independent of #1 — they +can be picked up in any order or in parallel by different people. #1 should +not start until #6 is done. + +## Rules for whoever picks this up + +- **Commit each item separately.** Same rule `PLAN_PERF.md` set, same + reason: a regression in one should be trivially bisectable. Do not bundle. +- **After every item**, from the repo root (yarn 4 is vendored, there is no + global yarn): + ``` + node .yarn/releases/yarn-4.11.0.cjs typecheck + node .yarn/releases/yarn-4.11.0.cjs lint + node .yarn/releases/yarn-4.11.0.cjs test + ``` + plus an iOS sim check via the `run-and-profile` skill for anything + touching `engine/` or `draw/`. +- **Carry the comments.** This codebase's comment density is an asset, not + noise. Several comments encode findings that cost real debugging time — + the SharedValue-closure trap, the measured-and-refuted `for...of` + pessimization, the `LEFT_EDGE_EPSILON` rationale, the pooled-rect safety + argument in `canvas2d.ts`. A refactor that drops them is a net loss even + if the code is shorter. When you move a block, move its comment; when you + merge two commented blocks, merge the comments. +- **This is a maintainability pass, not a perf pass.** If an item tempts + you into a perf change, note it and move on — `PLAN_PERF.md` is where + that conversation lives, and the standing open perf item is "Release + build on physical Android hardware remains unmeasured," not anything in + this doc. +- **Do not change public API.** Nothing here should alter `LivelineProps` + or anything exported from `src/index.tsx`. If an item seems to require + it, stop and raise it. From 7352acea39e79107281f50b026246ae2167e3902 Mon Sep 17 00:00:00 2001 From: Andrew Fuller Date: Tue, 28 Jul 2026 18:57:56 -0400 Subject: [PATCH 4/4] docs: cross-link the three workstream plan docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PLAN_PERF.md set the precedent of pointing the plan docs at each other so a resuming agent finds the right one. PLAN_MAINT.md is the third and was orphaned — PLAN.md's deferred section now names it, and PLAN_PERF.md points forward to it, including the note that its item 4 already blocked a perf change. --- PLAN.md | 4 ++++ PLAN_PERF.md | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/PLAN.md b/PLAN.md index 5923cad..3ef7b14 100644 --- a/PLAN.md +++ b/PLAN.md @@ -200,6 +200,10 @@ and published-package Metro transform: consumer-side, deferred as planned. `scrubActivationDelay` (example "Scroll" section) needs a real finger — synthetic input can't reach RNGH Pan gestures on the sim. - CI runs on GitHub once repo is pushed (workflows already scaffolded). +- Maintainability: see `PLAN_MAINT.md` (written 2026-07-28, not started) — a + seven-item structural pass over `src/`, the largest being the ~1700-line + `engineStep` and its three duplicated inline pipelines. Separate workstream + from this doc and from `PLAN_PERF.md`; explicitly *not* a perf pass. ## Task list mapping (session task tool) #1–#5 phases 0–4 complete; #6 = remaining candle/orderbook runtime polish diff --git a/PLAN_PERF.md b/PLAN_PERF.md index 5e30d11..50dd9ef 100644 --- a/PLAN_PERF.md +++ b/PLAN_PERF.md @@ -234,3 +234,12 @@ a manual on-device check via the example app (iOS sim and/or Android emulator — see `PLAN.md` for the device-testing workflow already established during the original port). Commit each item separately rather than bundling all four into one commit, so a regression in one is easy to bisect. + +## Follow-on workstream + +The structural debt this batch worked *around* rather than fixed is catalogued +in `PLAN_MAINT.md` (2026-07-28). Relevant to anyone resuming perf work: item 4 +there (positional-parameter soup) already blocked one perf change — pooled +decimation scratch needed a 17th positional parameter on `drawLine` and was +reverted rather than pay that cost, to be redone once the signature takes an +options object.