Skip to content

perf(producer): drawElement films keep their parallel workers instead of dropping to one - #6633

Merged
miguel-heygen merged 3 commits into
mainfrom
perf/no-single-worker-inversion
Oct 28, 2026
Merged

miguel-heygen merged 3 commits into
mainfrom
perf/no-single-worker-inversion

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What

Long drawElement-eligible films no longer drop from parallel capture to a single drawElement worker on machines the 3-worker drawElement router doesn't cover (under 24 GB, or when the router is off). They keep the parallel screenshot workers the resolver sized, about 25% faster.

Why

The single-worker "priority inversion" (shouldPreferSingleWorkerDrawElement, films of 900+ frames) bet that one drawElement worker beats parallel screenshot capture. Measured today, it loses on the machines where it fires:

fixture (1080x1920 42 s / 1080p 5 min) inverted: 1 drawElement worker this PR: 4 screenshot workers
wall 38.35 s / 156.8 s 29.26 s / 117.2 s (-24% / -25%)
wall - compile (second set of runs) 32.14 s / 162.84 s 22.44 s / 119.50 s (-30% / -27%)
peak RSS, CLI + Chrome + ffmpeg 2,272 MB / 1,788 MB 4,103 MB / 2,872 MB

Apple M4 Pro (14 cores, 24 GB), published hyperframes@0.8.210, the router made ineligible with HF_DE_PARALLEL_MIN_MEM_MB=999999 (what a 16 GB Mac gets), compared with and without HF_DE_SINGLE_MIN_FRAMES=0. That knob is the same switch this PR flips by default. Interleaved, medians.

Peak memory rises to what parallel screenshot capture uses for any comp drawElement refuses on the same machine: 4,131 MB for the same 42 s film with one mix-blend layer, on main. The resolver's existing memory and heap caps still size the workers, and the low-memory profile still pins 8 GB machines to one worker.

Why under 100 lines

One lever: the inversion's frame floor already had an off switch; this flips its default. The rest is the tests that exercised the inversion opting back in.

Related work

Refs #5945.

How

resolveDeSingleMinFrames(env) owns the inversion's frame floor and defaults it to 0, the inversion's documented off switch. HF_DE_SINGLE_MIN_FRAMES=<n> turns it back on. The short-comp band derives from the same floor, so it is off too. The 3-worker drawElement router (24 GB+ machines) is unchanged.

Test plan

  • Unit test added: the floor is 0 unless HF_DE_SINGLE_MIN_FRAMES sets one, and the inversion predicate does not fire at the default.
  • Fails without the fix: renderOrchestrator.test.ts > shouldPreferSingleWorkerDrawElement (DE priority inversion) > is off unless HF_DE_SINGLE_MIN_FRAMES sets a floor (a default of 900 fails exactly this test, 1 failed / 289 passed).
  • renderOrchestrator.deGateProbe.test.ts and renderOrchestrator.segmentedDeRetry.test.ts exercise the inversion, so they now set HF_DE_SINGLE_MIN_FRAMES=900 (the opt-in): 3 and 7 passed.
  • tsc --noEmit (producer) exit 0; renderOrchestrator.test.ts 290 passed; oxfmt and oxlint clean.
  • Manual testing performed: before/after above.
  • Documentation updated (not applicable)
  • Comments follow CONTRIBUTING.md "Comments"

Output: frames come from screenshot capture instead of drawElement, as they already do for every film under 900 frames. PSNR between the two paths: 52.5 dB average (34.9 dB worst frame) on the 42 s film, 54.5 dB on the 5 minute film. Two screenshot runs of the same film differ by about 52 dB on their own.

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST CHANGES · 1 blocker · 0 important · 1 nit · head f324c663

  • blocker — required CI › Producer: unit tests fails at this head with 6 tests that pin the inversion the new default turns off:

    • renderOrchestrator.deGateProbe.test.ts: "a comp drawElement accepts still inverts…" (expected 4 to be 1, :174) and "a comp the engine refuses at init keeps the resolver's workers…" (expected [] to have a length of 1).
    • renderOrchestrator.segmentedDeRetry.test.ts: all 4 "retries the segmented stage once on a screenshot session after …" cases (expected null to be truthy).

    These suites still test a path you can turn back on with HF_DE_SINGLE_MIN_FRAMES. Set HF_DE_SINGLE_MIN_FRAMES=900 in them (and restore it afterwards), or rewrite them for the new default. The test plan only ran renderOrchestrator.test.ts.

  • nit — the shouldPreferSingleWorkerDrawElement JSDoc (renderOrchestrator.ts:~2226) still says DE-single "beats screenshot-parallel at every worker count" above 900 frames. That now contradicts the new default; point it at the measurement in this PR.

@github-actions

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 271 (base branch 271), smooth 197 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE — 2 resolved, 0 open, 0 new · f324c663 → c5e9446f (inversion suites stub HF_DE_SINGLE_MIN_FRAMES=900; JSDoc now cites the re-measurement; CI incl. Producer unit tests and Windows green)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 28, 2026
Merged via the queue into main with commit 7d83a3d Oct 28, 2026
165 checks passed
@miguel-heygen
miguel-heygen deleted the perf/no-single-worker-inversion branch October 28, 2026 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant