Viewer performance: the gallery was quadratic in the size of the whole store - #208
Merged
Conversation
Measured on the real store (178 canvases across 15 projects), the worst project page took 4.2s cold and 2.2s warm. It now takes 0.09s. The cause was not the score cache, which is where I expected it. Scoring a canvas costs ~8ms and is cached. Computing the CACHE KEY cost ~95ms, because `evalCacheKey` asks `designedStatesFor` which state variants a canvas has, and that calls `listCanvases()` — which version-hashes every canvas in the store on every call. Once per card, twice on a cache miss. A 43-canvas project on a 178-canvas store did roughly 7,600 whole-canvas hashes per request, so the page was quadratic in the size of the whole store rather than the project being viewed. The cache was costing eleven times what it saved. The fix is to take one listing per render and thread it through: `designedStatesFor`, `evalCacheKey` and `evalFor` all accept states the caller already has, and `renderProjectPage` derives them from the listing it was already fetching. Both single-argument forms still work, so the detail page and the tests are unaffected. Everything else measured clean and was left alone: aggregation is 185ms cold and 65ms warm, and the score cache's 200-entry cap is not thrashing at 178 canvases. Neither was worth touching. test-viewer-perf.ts pins the shape rather than a wall-clock budget, so it means the same thing on a CI runner as on a laptop: quadrupling the store must not cost ~16x. Verified against the pre-fix code, which measures 14.7x (45ms → 662ms); the fix measures 4.0x (3ms → 12ms).
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Item 0 from the saved plan — the one slow thing you were already feeling daily. Measured first, because the three causes on that list were hypotheses and only one of them survived contact with a profiler.
What it measured, before
On the real store — 178 canvases across 15 projects, 3.9MB — the worst project page took 4.2 seconds cold and 2.2 seconds warm. Small projects were fine, which is why this only showed up as you accumulated repos.
The cause was not what I had written down
The saved note blamed the score cache thrashing against its 200-entry cap, live iframes, and per-repo aggregation cost. Two of those are wrong and the third is irrelevant at this size:
The actual cost was computing the cache key, at about 95ms per canvas.
evalCacheKeyneeds to know which state variants a canvas has, so it callsdesignedStatesFor, which callslistCanvases()— andlistCanvases()version-hashes every canvas in the store on every call. That happened once per card, and twice for any card whose score was not yet cached.So a 43-canvas project sitting on a 178-canvas store performed roughly 7,600 whole-canvas hashes per page request. The page was quadratic in the size of the entire store rather than the project being viewed, which is exactly why it degraded as you registered more repos rather than as any single project grew. The cache was costing eleven times what it saved.
The fix
Take one listing per render and thread it through.
designedStatesFor,evalCacheKeyandevalFornow accept the states a caller already has, andrenderProjectPagederives them from the listing it was already fetching for other reasons. Both single-argument forms still work unchanged, so the canvas detail page and the existing cache-key tests are untouched.Same store, same pages, after:
What I deliberately did not touch
Aggregation and the score cache both measured clean, so changing either would have been motion without evidence. The 200-entry cap is worth revisiting only if the store grows past it — the honest trigger is a measurement, not a hunch, and this PR does not pre-empt it.
Live iframes are still one request per card. That is real browser-side work, but it is lazy-loaded and it was not what made the page slow; the static-thumbnail change belongs with the artboard-aware thumbnail work, where it also earns its keep for mobile canvases.
The regression test
test-viewer-perf.tspins the shape rather than a wall-clock budget, so it means the same thing on a CI runner as on a laptop: quadrupling the store must not cost about sixteen times as much. The bound sits at eight, clear of honest variance and nowhere near quadratic.It was verified against the pre-fix code rather than assumed to work — a regression test that would not have caught the bug is worth nothing. Reverting
viewer.tsand running it gives 14.7x (45ms → 662ms); with the fix it gives 4.0x (3ms → 12ms).It also covers the threading itself: a caller passing states it already has must produce the same key as a caller that looks them up, or the gallery and the detail page would key the same canvas differently and never share a cache entry.