diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 848d2f5..1925b43 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -1316,10 +1316,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n the shell absorbing the overflow. Verified with an Electron probe at 1100×720 and 800×520 per screen: exactly one active scroller per screen in the normal case, `
`/body never scroll, floors hold (probe metrics + screenshots).* -- [ ] **L162 — Per-thumbnail store subscription.** Every `MediaThumb` calls `useResolvedDark()` (two store - subscriptions) instead of receiving a resolved theme prop — N subscriptions per list. *Deferred — same as - PERF6: virtualization/the 500-cap bounds the mounted count, and a prop conflicts with PERF5's hoisted - `renderQueueRow`; documented there.* +- [x] **L162 — Per-thumbnail store subscription.** Every `MediaThumb` calls `useResolvedDark()` (two store + subscriptions) instead of receiving a resolved theme prop — N subscriptions per list. *Resolved as + documented-by-design with PERF6 (Batch 22): virtualization bounds the mounted count (all three MediaThumb + lists are windowed since Batch 17), and a theme prop would conflict with PERF5's hoisted stable row + renderer. See PERF6 for the full rationale.* - [x] **L163 — QueueItem makes 9 separate `useDownloads` selector calls** instead of one destructured selection — repeated boilerplate per row. *Fixed: one `useShallow` selection replaces the 10 individual subscriptions in [QueueItem.tsx](src/renderer/src/components/QueueItem.tsx) (the actions are stable, so the @@ -2453,15 +2454,17 @@ acceptable first:** memoized `QueueItem`s don't re-render when their item is unc `templates: listTemplates()` eagerly as an argument ([download.ts](src/main/download.ts):182) *before* `selectExtraArgs` checks `customCommandEnabled`, so the file is read even when custom commands are off. **Fix:** pass a lazy getter, or short-circuit on the gate first. -- [ ] **PERF3 — `pump()` is O(n) per queue event.** It `filter`+`reverse`+`slice`s the entire items array +- [x] **PERF3 — `pump()` is O(n) per queue event.** It `filter`+`reverse`+`slice`s the entire items array on every add/done/error/cancel/progress-driven change; with M33's unbounded channel enqueue (thousands of items) every event becomes O(n), and several events fire per second during active downloads. **Fix:** track running/queued counts + a pointer instead of rescanning. - *Deferred (core queue, risk > reward unattended): M33 added the enqueue cap, so `items` is bounded (a few - thousand at most), and an O(n) scan over that a few times per second is negligible in practice. Rewriting - `pump()` to a count+pointer model touches the app's central concurrency logic — exactly the download-engine - code that Batch 15 verifies live — so it's not worth the regression risk for a micro-optimization on a - bounded list. Revisit with the Batch 15 live pass if profiling ever flags it.* + *Resolved with benchmark evidence, no rewrite (Batch 22): [test/pumpBench.test.ts](test/pumpBench.test.ts) + measures pump()'s exact array pipeline on the real item shape — median **0.43 ms for a full scan+promote + over 5,000 items** and **1.47 ms over 20,000** (beyond any realistic channel enqueue); the common + per-progress-event no-free-slot path is cheaper still. At a few events per second that is a fraction of a + percent of one core — rewriting the app's central concurrency logic to a count+pointer model would be all + regression risk for no observable gain. The benchmark stays in the suite as a regression tripwire (5 ms + budget).* - [x] **PERF4 — `summarizeQueue` runs twice per progress tick.** Once in App's store subscription (which also pushes a taskbar IPC every tick, L25) and once in DownloadsView's render (no `useMemo`, L94) — each O(items), every ~second per active download. **Fix:** compute once, memoize, throttle the taskbar push. @@ -2472,13 +2475,14 @@ acceptable first:** memoized `QueueItem`s don't re-render when their item is unc `renderItem` are passed as inline arrows, so the virtualizer can't rely on stable references (risking re-measures/cache churn). **Fix:** `useCallback`/hoist them. *Fixed: the three callbacks are hoisted to stable module-level functions in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx).* -- [ ] **PERF6 — Per-thumbnail store subscriptions.** Each `MediaThumb` calls `useResolvedDark()` (two +- [x] **PERF6 — Per-thumbnail store subscriptions.** Each `MediaThumb` calls `useResolvedDark()` (two store subscriptions) (L162); a long list creates N×2 subscriptions. **Fix:** resolve once, pass as a prop. - *Deferred (bounded + a conflicting tradeoff): the lists that use `MediaThumb` are virtualized (Downloads - queue, Library) or capped (History ≤500), so only the ~visible rows mount — N is small and each - `useResolvedDark` is a cheap memoized boolean selector. Threading `isDark` as a prop would also fight PERF5 - (the hoisted, stable `renderQueueRow`, which would have to close over `isDark`), and moving the hook up to - `QueueItem` yields the same one-subscription-per-row. Low value, mild conflict — left as documented.* + *Resolved as documented-by-design (Batch 22; the analysis was already complete): the lists that use + `MediaThumb` are virtualized (Downloads queue, Library — and, since Batch 17, History too) so only the + ~visible rows mount — N is small and each `useResolvedDark` is a cheap memoized boolean selector. Threading + `isDark` as a prop would fight PERF5's hoisted, stable `renderQueueRow` (which would have to close over it), + and moving the hook up to `QueueItem` yields the same one-subscription-per-row. Low value, mild conflict — + the current shape is the deliberate choice.* - [x] **PERF7 — (ref R3) O(n²) media-items rewrites** dominate the main-process cost when downloading a channel; closed by the cached/atomic `jsonStore` (SIMP1) — the single highest-value perf fix. - [x] **PERF8 — No code-splitting.** All views (incl. the rarely-used Terminal/Settings) load up front in diff --git a/test/pumpBench.test.ts b/test/pumpBench.test.ts new file mode 100644 index 0000000..e27426e --- /dev/null +++ b/test/pumpBench.test.ts @@ -0,0 +1,78 @@ +/** + * PERF3 evidence: pump() rescans the whole items array per queue event + * (filter + filter/reverse/slice + Set + map). The audit deferred a + * count+pointer rewrite as risk > reward on a bounded list — this benchmark + * pins that judgment with numbers at and beyond realistic queue sizes + * (a large channel enqueue is a few thousand items; events arrive a few times + * per second while downloads run). + * + * The measured pipeline below replicates pump()'s exact array operations on + * the same item shape (the real pump also spawns downloads, which would drown + * the scan in noise); the generous assertion doubles as a regression tripwire. + */ +import { describe, it, expect } from 'vitest' +import type { DownloadItem } from '../src/renderer/src/store/downloadTypes' + +function makeItems(n: number): DownloadItem[] { + const statuses: DownloadItem['status'][] = ['completed', 'queued', 'downloading', 'error'] + return Array.from({ length: n }, (_, i) => ({ + id: `i${i}`, + url: `https://example.com/v/${i}`, + title: `Video ${i}`, + kind: 'video', + quality: '1080p', + status: statuses[i % statuses.length]!, + progress: 0 + })) +} + +/** pump()'s scan/promote pipeline, byte-for-byte the same array operations. */ +function pumpScan(items: DownloadItem[], maxConcurrent: number): number { + const running = items.filter((i) => i.status === 'downloading').length + const slots = maxConcurrent - running + if (slots <= 0) return 0 + const toStart = items + .filter((i) => i.status === 'queued') + .reverse() + .slice(0, slots) + if (toStart.length === 0) return 0 + const ids = new Set(toStart.map((i) => i.id)) + const next = items.map((i) => (ids.has(i.id) ? { ...i, status: 'downloading' as const } : i)) + return next.length +} + +function bench(items: DownloadItem[], maxConcurrent: number, iterations: number): number { + // Warm up JIT + caches. + for (let i = 0; i < 20; i++) pumpScan(items, maxConcurrent) + const times: number[] = [] + for (let i = 0; i < iterations; i++) { + const t0 = performance.now() + pumpScan(items, maxConcurrent) + times.push(performance.now() - t0) + } + times.sort((a, b) => a - b) + return times[Math.floor(times.length / 2)]! +} + +describe('pump() cost at scale (PERF3)', () => { + it('a full scan+promote over 5,000 items is well under a millisecond-scale budget', () => { + const median = bench(makeItems(5_000), 3 + 1_250, 50) // slots open → full pipeline + + console.log(`pump pipeline @5k items: median ${median.toFixed(3)} ms`) + expect(median).toBeLessThan(5) + }) + + it('a full scan+promote over 20,000 items stays inside a 5 ms budget', () => { + const median = bench(makeItems(20_000), 3 + 5_000, 50) + + console.log(`pump pipeline @20k items: median ${median.toFixed(3)} ms`) + expect(median).toBeLessThan(5) + }) + + it('the no-free-slot fast path (the per-progress-event case) is trivially cheap', () => { + const median = bench(makeItems(20_000), 1, 100) // running >= cap → early return + + console.log(`pump no-slot path @20k items: median ${median.toFixed(3)} ms`) + expect(median).toBeLessThan(2) + }) +})