perf(webapp,run-engine): cut CPU on the engine-facing worker-action routes - #4746
Conversation
… paths Two on-demand benchmarks, neither in the default suite: - apps/webapp: spawns a real webapp with --inspect against throwaway containers, seeds a production environment with a promoted managed deployment, and drives a closed-loop supervisor pool through the worker-action lifecycle. Profiles over CDP so the profile covers only the measured window, and samples event-loop utilization inside the webapp. - internal-packages/run-engine: drives RunEngine directly, profiling the enqueue and lifecycle phases separately so engine cost is not mixed with request-stack overhead. Plus a dependency-free .cpuprofile analyzer that symbolicates through the build's source maps and ranks CPU by package, by self time and by total time. startWebapp gains overrideEnv, applied after the worker-disable defaults, so the HTTP bench can re-enable the run engine worker that drains the master queue into the worker queues a supervisor dequeues from.
…outes Three changes from a CPU profile of the worker-action request path, together taking on-CPU time per completed run from 9.07ms to 6.59ms (-27%) at ~300 req/s, with every worker-action p50 down 23-27%. Split the event-loop monitor in two. The blocked-loop detector installs an async_hooks hook that fires for every async resource the process creates, and enabling any async hook also puts V8 on the slow path for promise instrumentation process-wide; it measured ~14% of on-CPU time plus roughly half of all GC. It is now opt-in via EVENT_LOOP_MONITOR_ENABLED (default 0). The event-loop utilization gauge is a single interval timer with no per-request cost, so it moves to EVENT_LOOP_UTILIZATION_MONITOR_ENABLED (default 1) and stays on. Bucket route matching by first static path segment. The existing router patch removed the per-request re-flatten and regex rebuild, but matching was still a linear scan over the whole 521-route table. Route-matching self time drops 64%. Ordering is preserved exactly and equivalence was verified over 20,050 pathnames; apps/webapp/test/routeMatchingPatch.test.ts pins the semantics. Demote the per-heartbeat and per-dequeue info logs to debug. These are the two highest-rate engine calls and each wrote a synchronous structured log line on every request.
The collector only defined a metrics pipeline, so pointing INTERNAL_OTEL_TRACE_EXPORTER_URL at it locally failed and the webapp fell back to the console span logger. Adds a traces pipeline with the debug exporter, which accepts and summarises spans - enough to exercise the whole client-side export path when profiling.
|
WalkthroughThe changes separate webapp event-loop blocking detection from utilization sampling and add independent environment controls. They add CPU and event-loop benchmark suites for the webapp and run-engine, including profiling, load generation, fixture setup, source-map resolution, and profile analysis. The router patch adds branch bucketing and cache layers with route-matching tests. The Collector now exports sampled OTLP traces through a traces pipeline. Successful dequeue and heartbeat logs now use debug level. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Settle in-flight CDP requests when the inspector socket closes or errors, and guard the message parse. A webapp that exited mid-run previously left every send() pending, so the bench hung until the suite timeout with nothing explaining why; a parse throw inside the listener surfaced as an uncaughtException and tore down the runner. send() now also rejects instead of hanging when the socket is already closed. Unref both event-loop-utilization interval timers so a throw between start and stop cannot keep a vitest worker alive. Make eventLoopUtilizationMonitor.enable() idempotent: a second call previously started another interval and overwrote the only stored cleanup callback, leaking the first. Validate --top in the profile analyzer. A non-numeric value produced NaN, and slice(0, NaN) silently printed empty tables that looked like an empty profile. Keep the release note behavioural, without the env var name.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The baseline reading was fired without awaiting it, so a sampling tick that landed before that round trip resolved saw no previous reading and recorded a zero delta. stopEluSampling() compensated by dropping the first sample unconditionally, which also discarded a genuine measured interval and could leave a short run with no samples at all. The baseline is now awaited before the interval starts, so every recorded sample is a real delta and the drop is gone.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/webapp/test/bench/lib/cdp.ts (2)
184-189: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrevent ELU callbacks from updating a stopped sampling run.
An
evaluateElu()call can start before Line 223 clears the timer and resolve afterstopEluSampling()returns. Line 187 then appends a sample after Lines 232-243 calculate the statistics. This can makestats.sampleCountdiffer fromsamples.length. A delayed callback can also append an old sample to a later run.Use a per-run generation token. Invalidate it in
stopEluSampling(). Check it after the baseline await and before each callback appends a sample.Proposed lifecycle guard
export class WebappProfiler { + private eluRun = 0; private eluTimer: NodeJS.Timeout | null = null; async startEluSampling(intervalMs = 250): Promise<void> { + this.stopEluSampling(); + const run = ++this.eluRun; this.eluSamples = []; this.eluStartedAt = Date.now(); await this.evaluateElu(); + if (run !== this.eluRun) return; const timer = setInterval(() => { void this.evaluateElu().then((utilization) => { - if (utilization !== undefined) { + if (run === this.eluRun && utilization !== undefined) { this.eluSamples.push({ atMs: Date.now() - this.eluStartedAt, utilization }); } }); }, intervalMs); stopEluSampling(): { stats: EluStats; samples: EluSample[] } { + this.eluRun++; if (this.eluTimer) {Also applies to: 221-245
171-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd required crumb markers for the changed ELU blocks.
Add an approved
//@Crumbsmarker or `// `#region` `@crumbswrapper for the ELU baseline and sample-retention changes.As per coding guidelines, “Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs.”Also applies to: 227-227
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c89c6ae9-abe0-433a-bb57-12a6ea3475d9
📒 Files selected for processing (2)
apps/webapp/test/bench/engineHttp.bench.test.tsapps/webapp/test/bench/lib/cdp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/test/bench/engineHttp.bench.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (47)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: sdk-compat / Node.js 22.23 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Node.js 20.20 (warp-ubuntu-latest-x64-4x)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - npm)
- GitHub Check: sdk-compat / Node.js 26.4 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Bun Runtime
- GitHub Check: sdk-compat / Node.js 24.18 (warp-ubuntu-latest-x64-4x)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - pnpm)
- GitHub Check: sdk-compat / Deno Runtime
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-windows-latest-x64-8x - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (warp-ubuntu-latest-x64-4x - npm)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: packages / 🧪 Unit Tests: Packages (1, 3)
- GitHub Check: packages / 🧪 Unit Tests: Packages (2, 3)
- GitHub Check: obsmap / 🧪 Unit Tests: Observability Map
- GitHub Check: packages / 🧪 Unit Tests: Packages (3, 3)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamic imports. Only use dynamicimport()when:
- Circular dependencies cannot be resolved otherwise
- Code splitting is genuinely needed for performance
- The module must be loaded conditionally at runtime
Files:
apps/webapp/test/bench/lib/cdp.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/test/bench/lib/cdp.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/test/bench/lib/cdp.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.ts: When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
Do not use high-cardinality attributes in OTEL metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
apps/webapp/test/bench/lib/cdp.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepath
Files:
apps/webapp/test/bench/lib/cdp.ts
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Add crumbs as you write code — not just when debugging. Mark lines with
//@Crumbsor wrap blocks in `// `#region` `@crumbs. They stay on the
branch throughout development and are stripped byagentcrumbs strip
before merge.
Do not invent new namespaces — pick from this table or ask first.
Do not flag//@Crumbsmarkers, `#region `@crumbsblocks, or agentcrumbs
imports in reviews. These are temporary debug instrumentation stripped
before merge. Data logged in crumbs (IDs, names, values) never reaches
production.
Files:
apps/webapp/test/bench/lib/cdp.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
Cuts CPU on the
engine/v1/worker-actions/*routes a managed supervisor calls, and adds the benchmark harness the numbers come from.Measured on a local stack: on-CPU per completed run 9.07ms → 6.59ms (−27%), busy fraction 45.6% → 33.8%, with every worker-action p50 down 23–27%. Load was 5,000 runs / 24 virtual supervisors / 90s window / 30,120 requests / 0 errors.
Query-count work from the same investigation is deliberately not here — it will follow as a separate PR.
The three changes
1. Split the event-loop monitor in two (~14% of on-CPU, plus ~5pp of GC).
eventLoopMonitor.server.tsinstalls a globalasync_hookshook:initwrites aMapentry for every async resource the process creates,beforecallsprocess.hrtime()andcontext.active()on every one. Enabling any async hook also puts V8 on the slow path for promise instrumentation process-wide.EVENT_LOOP_MONITOR_ENABLEDdefaulted to"1", so this was the shipping configuration.The blocked-loop detector is now opt-in (
EVENT_LOOP_MONITOR_ENABLED, default0). The event-loop utilization gauge — a single interval timer with no per-request cost — moves to its own flag (EVENT_LOOP_UTILIZATION_MONITOR_ENABLED, default1) and stays on, so the useful half survives without the expensive half.A/B under identical load:
2. Bucket route matching by first static path segment (10.4% → 3.9% of on-CPU).
patches/@remix-run__router@1.23.3.patchalready memoized flattened branches and compiled path regexes. What remained was the linear scan:matchRouteBranchwalked the ranked branch list callingmatchPathper branch across 521 route files, so every worker-action request paid a scan proportional to the whole route table.Branches are now indexed by their lowercased leading segment, with one always-considered list for branches whose leading segment is dynamic, splat or optional (and for root/pathless paths). A request walks only its own bucket merged with that list. Route-matching self time dropped 64% (3.6s → 1.3s over a 90s window).
Ordering is preserved exactly: both lists hold indexes into the already rank-sorted branch array and are walked in ascending-index order, so the first match found is the same branch the full scan would have found. Bucketing lowercases on both sides, so case-insensitive matching still resolves and
caseSensitive: trueroutes are still rejected bymatchPathitself. A pathname whose own leading segment can't be bucketed falls back to the full scan.Verified equivalent to the unpatched matcher over 20,050 pathnames (literal, dynamic, splat, optional, case variants, basenames, percent-encoded) with zero mismatches.
apps/webapp/test/routeMatchingPatch.test.tspins the matching semantics rather than the optimisation, so it still passes without the patch.3. Demote per-heartbeat and per-dequeue
infologs todebug.These are the two highest-rate engine calls and each wrote a synchronous structured log line on every request. Synchronous
consolewrites can block the loop when stdout backs up, which costs more than the ~1.3% CPU share suggests.The harness
Two benchmarks, neither in the default suite (they run for minutes, attach the V8 profiler, and report numbers rather than assert on them). See
apps/webapp/test/bench/README.md.apps/webapp/test/bench/engineHttp.bench.test.ts— spawns a real webapp against throwaway Postgres/Redis containers, seeds a production environment with a promoted managed deployment, and drives a closed-loop supervisor pool through the full lifecycle. Profiling runs over CDP rather than--cpu-profso it covers only the measured window instead of being swamped by boot, andperformance.eventLoopUtilization()is sampled inside the webapp process.internal-packages/run-engine/src/engine/bench/runEngineLifecycle.bench.test.ts— drivesRunEnginedirectly, profiling enqueue and lifecycle separately so engine cost isn't mixed with request-stack overhead.apps/webapp/test/bench/analyzeProfile.ts— dependency-free.cpuprofileanalyzer that symbolicates through the build's source maps and ranks CPU by package, self time and total time. Percentages are shares of on-CPU time (V8's(idle)/(program)excluded).startWebappgainsoverrideEnv, applied after the worker-disable defaults, so the HTTP bench can re-enable the run engine worker that drains the master queue into the worker queues a supervisor dequeues from.The local OTel collector gains a traces pipeline. It only defined a metrics pipeline, so pointing
INTERNAL_OTEL_TRACE_EXPORTER_URLat it locally failed and the webapp silently fell back to the console span logger.Configuration
For operators upgrading:
EVENT_LOOP_MONITOR_ENABLED(now defaults to0) — the per-async-resource blocked-loop detector. Set to1to restore the previous behaviour and keep emittingevent-loop-blockedspans.EVENT_LOOP_UTILIZATION_MONITOR_ENABLED(new, defaults to1) — thenodejs.event_loop.utilizationgauge. Unchanged in behaviour; it just has its own flag now so it survives turning the detector off.Notes for review
pnpm-lock.yamlchanges only because the router patch content changed, which changes its patch hash.Verification
apps/webapp/test/routeMatchingPatch.test.ts(12 cases) passestypecheck,format,lint,knipclean