test(sprites): hoist lockAllAnchors + parallelize buildFinalizedWalkSet's writes - #6207
Merged
atomantic merged 4 commits intoSep 4, 2026
Merged
Conversation
…ype (atomantic#6180) 136 call sites across walk.test.js/atlas.test.js/animationTrackWorkflow.test.js locked a character from byte-identical cached candidate PNGs, so each one re-ran the real Sharp normalize + chroma-key-selection pipeline from scratch. `lockAllAnchors` now locks ONE real prototype per TEST_ROOT (at the full anchor set, since every caller's `directions` is a subset of it) and materializes every other character by recursive-copying only the artifacts a caller actually asked for, id-normalizing the manifest, and resetting any anchor outside the requested set to the exact seeded-pending shape a fresh partial lock would leave (mirroring unlockReferenceAnchorImpl's own reset). No call site changed except the three that build the record + pass `records` through (`characterWithLockedAnchors`, atlas.test.js's `lockAllAnchors(id)`, animationTrackWorkflow.test.js's `characterWithEastAnchor`) — the fast path lives entirely inside spriteTestFixtures.js. Each suite gets an equivalence test (the acceptance gate, not an extra, per the issue): it builds a character both ways — through `lockAllAnchorsReal` (exported unchanged) and through the materialized path — and asserts the result is byte-identical after id normalization: file names, PNG bytes, the full manifest (including every embedded sha256), and the record's chromaKey/status. The partial-set case is the one this test actually earns its keep on: an early version of the reset logic left the `south` anchor entry wrongly reset to pending (it's synced from mainReference, never locked through its own `target`) and left a partial manifest's `status` at 'complete' instead of 'in-progress' — both caught by this test, not by eye. Measured locally (Windows, before/after, `npx vitest run <file>`): walk.test.js: ~29.4s -> ~19.7-21.4s tests phase (~30% faster) atlas.test.js: ~24.4s -> ~20.2-21.3s tests phase (~15% faster) atlas.test.js's own numbers are lower than walk.test.js's because it only has 19 lockAllAnchors calls (all at the full 9-anchor set) vs walk.test.js's 117 (mostly 1-3 anchors) -- fewer calls to save Sharp work on, even though each one touches more anchors. buildFinalizedWalkSet (atlas.test.js's other named fixture cost per the issue, ~32% of its suite time) is untouched here: it never calls Sharp (writeWalkFramePng already caches the encoded buffer), so a copy-based materialization of it doesn't help -- see the companion PR for what does. Full sprites/ suite: 716 passed, 1 skipped, 0 failed (unchanged pass count). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rection writes Closes atomantic#6180 Builds on the lockAllAnchors materialization (same commit as the sibling PR): atlas.test.js's other named fixture cost, buildFinalizedWalkSet (~32% of the suite per the issue's own profiling), never calls Sharp -- writeWalkFramePng already serves the encoded PNG buffer from a cache keyed on (tint, armX, speck), so every default-options call was already paying only raw file-write latency, not compute. A copy-based materialization (mirroring lockAllAnchors's approach) measured WORSE than doing nothing: copying still has to read the source file first, so it's strictly more I/O than a writeFile from a buffer already in memory, and the manifest/ selection/run-manifest content (which embeds characterId) still needs id-rewriting and a recomputed sha256 per file since — unlike the PNGs — its content actually changes per character. What actually helps: the 8 directions are fully independent (each writes under its own runId), so running them concurrently with Promise.all hides the per-file write latency instead of paying it 8x sequentially. Same production test code, same manifest shape, same file count — just concurrent instead of serial, so there's no new mechanism that could drift from the real pipeline and nothing new to equivalence-test. Measured locally (Windows, before/after, `npx vitest run atlas.test.js`): baseline (main): 24.4s tests phase + lockAllAnchors hoist alone: ~20.2-21.3s (~15% faster) + buildFinalizedWalkSet copy-materialized: ~23.2s (worse than hoist alone -- discarded) + buildFinalizedWalkSet parallelized (this PR): ~17.7-18.1s (~27-28% faster) walk.test.js is unaffected by this commit (it doesn't use buildFinalizedWalkSet) -- its ~30% win comes entirely from the sibling lockAllAnchors commit. Full sprites/ suite: 716 passed, 1 skipped, 0 failed (unchanged pass count). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 tasks
# Conflicts: # server/services/sprites/walk.test.js
…-parallelize-walkset
Contributor
Author
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
2 participants
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.
Summary
Closes #6180.
lockAllAnchorsmaterialization (same commit) — see that PR for the full writeup on the prototype+copy approach and its equivalence tests.atlas.test.js's other named fixture cost,buildFinalizedWalkSet(~32% of the suite per the issue's own profiling): the 8 directions it writes are fully independent, so they now run concurrently viaPromise.allinstead of sequentially.Why parallelize instead of materialize
I first tried mirroring the
lockAllAnchorscopy-based approach for this function too, and it measured worse than doing nothing (~23.2s vs ~20.2-21.3s for the hoist alone). Root cause:buildFinalizedWalkSetnever calls Sharp —writeWalkFramePngalready serves the encoded PNG buffer from a cache — so every call was already paying only raw file-write latency, not compute. Copying still has to read the source file first (strictly more I/O than awriteFilefrom an in-memory buffer), and the manifest/selection/run-manifest JSON embedscharacterIdper file, so it still needs id-rewriting and a recomputedsha256— unlike the PNGs, whose bytes are copyable verbatim.Parallelizing the 8 independent directions instead hides the per-file write latency rather than trying to avoid it, using the exact same production test code — no new copy/rewrite mechanism, so no new equivalence test is needed (there's nothing new that could drift from the real pipeline; it's the same pipeline, just concurrent).
Two options for this issue
This is option B of two — see #6204 for the smaller, more conservative option (the
lockAllAnchorschange alone, nobuildFinalizedWalkSetchange). I'd merge this one: it includes everything #6204 does, plus a real additional win onatlas.test.jsat low incremental risk (a mechanicalfor→Promise.allswap on already-independent work, not a new subsystem). #6204 stands alone if you'd rather not take this second piece.Test plan
server/services/sprites/suite: 716 passed, 1 skipped, 0 failed (same pass count asmain)npx vitest run atlas.test.jstests phase:main): 24.4slockAllAnchorshoist alone: ~20.2-21.3s (~15% faster)buildFinalizedWalkSetcopy-materialized: ~23.2s (worse — discarded, not in this PR)buildFinalizedWalkSetparallelized (this PR): ~17.7-18.1s (~27-28% faster)walk.test.jsunaffected by the parallelization commit (doesn't usebuildFinalizedWalkSet); its ~30% win is from the sharedlockAllAnchorscommit⏱before/after per the issue's acceptance criteria (will follow up with the actual CI numbers once this runs — walk.test.js should clear the 17s bar; atlas.test.js is close but CI's absolute numbers may differ from this local machine's)🤖 Generated with Claude Code