Uh oh!
There was an error while loading. Please reload this page.
perf(core): precompile workflow vm.Script at module init - #2524
Open
TooTallNate wants to merge 1 commit into
Open
perf(core): precompile workflow vm.Script at module init#2524TooTallNate wants to merge 1 commit into
TooTallNate wants to merge 1 commit into
Conversation
Compile the workflow bundle's `vm.Script` for each known workflow source filename when `workflowEntrypoint` is constructed (module-init time), rather than lazily on the first queue delivery's replay. Builders inline the deduplicated, sorted set of workflow filenames into generated routes via the new `workflowFilenames` entrypoint option, so the first replay is a cache hit instead of paying the bundle parse/compile on the critical path.
🦋 Changeset detectedLatest commit: 1ffc078 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests▲ Vercel Production (2 failed)astro (1 failed):
nextjs-turbopack (1 failed):
Details by Category❌ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
pranaygp
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the full diff, built @workflow/core + @workflow/builders clean, and ran the affected tests (script-cache, runtime, constants, get-workflow-filenames) locally — all green.
The design is nicely fail-safe: workflowFilenames is best-effort, and a wrong/missing/extra filename only ever degrades to a cache miss → lazy compile (exactly today's behavior), with no determinism or correctness risk. The filename derivation also lines up with the runtime: since the registry lookup __private_workflows.get(workflowName) has to succeed, workflowName === workflowId, so the build-time moduleSpecifier matches what runWorkflow compiles under. Tests, comments, and the namespace-options refactor all look solid.
One tradeoff I'd like validated before merge (inline on runtime.ts), plus two smaller notes. Nothing blocking.
| // (for stack-trace attribution); the builder passes that deduplicated set of | ||
| // filenames. When none are provided, fall back to a single representative | ||
| // filename so the bulk top-level parse is still done eagerly. | ||
| precompileWorkflowBundle(workflowCode, options?.workflowFilenames); |
Contributor
There was a problem hiding this comment.
Worth validating against a multi-file benchmark. This eagerly compiles the full bundle once per workflow source file at module init — getCachedWorkflowScript does new Script(fullBundle, { filename }) per filename, and V8 keys its script compilation cache on the filename as well as the source, so there's no parse sharing across them (the existing script-cache.ts doc already notes "the whole bundle is compiled once per distinct filename").
The lazy path only ever compiled the filenames of workflows this process actually served. For an app with many workflow files where a given serverless instance only handles a subset, this front-loads — and increases total — cold-start parse work for workflows that may never run on this instance. The first-request win is real only if module init runs off that request's critical path on the platform; otherwise end-to-end latency for the first invocation could regress as the number of source files grows.
Does the benchmark cover an app with many distinct workflow source files (not just many functions in one file)? If not, that's the case I'd most want to see before merging.
| * filename compiles the bulk of the bundle; the per-workflow filenames then | ||
| * recompile lazily (a cheap re-parse) on first replay. | ||
| */ | ||
| const PRECOMPILE_FALLBACK_FILENAME = '<workflow-bundle>'; |
Contributor
There was a problem hiding this comment.
The fallback doesn't actually move any parse cost off the first replay. It caches a Script under <workflow-bundle>, but runWorkflow looks it up under the real moduleSpecifier, so the first replay still does a full new Script(...) under that real filename (different filename ⇒ full recompile, no reuse of the fallback entry). So the no-filenames path pays an extra full top-level parse at init for a Script that is never read, then parses again at replay — net overhead plus a dead cache entry, not a saving.
Since the builders now always pass workflowFilenames, this only affects manual/legacy callers, so it's low-impact. But consider either making the no-filenames case a no-op, or tightening the comment here and at lines 327-330 — both currently read as if the fallback warms the replay path, which it doesn't.
| getQueueTopicPrefix('workflow', namespace); | ||
| const workflowFilenames = options?.workflowFilenames; | ||
| if (workflowFilenames && workflowFilenames.length > 0) { | ||
| const sorted = [...new Set(workflowFilenames)].sort(); |
Contributor
There was a problem hiding this comment.
Minor/nit: getWorkflowFilenamesFromManifest already returns a deduped, sorted array, so for the builder call sites this new Set(...).sort() is redundant work. It's harmless and reasonable as defensive normalization for direct callers of createWorkflowEntrypointOptionsCode — just flagging the double dedup/sort in case you'd rather pick one layer to own it.
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
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
vm.Scriptat module-init time (whenworkflowEntrypointis constructed) instead of lazily on the first queue delivery's replay, so the first replay is a cache hit rather than paying the bundle parse/compile on the critical path.workflowEntrypointgains an optionalworkflowFilenamesoption; the builders inline the deduplicated, sorted set of workflow source filenames into generated routes so the precompiledScriptmatches exactly whatrunWorkflowcompiles under (parseWorkflowName(name)?.moduleSpecifier || name).Details
packages/core/src/vm/script-cache.ts: addprecompileWorkflowScripts()(swallows compile errors so module init never breaks) and anisWorkflowScriptCached()test inspector.packages/core/src/runtime.ts:workflowEntrypoint(workflowCode, { namespace?, workflowFilenames? })warms the compiled-bundle cache at construction time.packages/builders:getWorkflowFilenamesFromManifest()derives the per-file source filenames from the manifest'sworkflowIds; wired into all generated-route sites (base-builderworkflow-only / combined / combined-watch,builder-deferredwatch / production) viacreateWorkflowEntrypointOptionsCode({ workflowFilenames }).Verified end-to-end against the example workbench: the generated flow route emits
workflowEntrypoint(workflowCode, { workflowFilenames: [...] })with the deduplicated, sorted filenames.Testing
precompileWorkflowScripts(cache warming, identical-to-lazyScript, empty list, swallows syntax errors),createWorkflowEntrypointOptionsCode(filenames dedup/sort/combine),getWorkflowFilenamesFromManifest, and aworkflowEntrypointmodule-init precompile assertion.@workflow/coreand@workflow/buildersbuild clean; affected test files pass.Changeset
@workflow/core: minor (newworkflowFilenamesoption + module-init precompile)@workflow/builders,@workflow/next: patch (route wiring)