perf(producer): gate per-frame debug meta via optional isLevelEnabled#383
perf(producer): gate per-frame debug meta via optional isLevelEnabled#383vanceingalls wants to merge 1 commit intovance/hdr-benchmark-harnessfrom
Conversation
1d3a961 to
cadda3c
Compare
38cae8b to
723c98b
Compare
cadda3c to
aa41490
Compare
723c98b to
2f5ecdc
Compare
2f5ecdc to
57abd3a
Compare
aa41490 to
9b942fb
Compare
57abd3a to
c0d1d4e
Compare
3e3fa0c to
6d2f104
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
isLevelEnabled-gated per-frame debug metadata is a textbook perf fix for a hot-path logger call. Any per-frame logger.debug({ ... }) was paying the cost of constructing the metadata object (JSON serialization, heap allocation, GC pressure) on every frame even when the debug level was off and the message would never be written. Adding an optional isLevelEnabled check before the object construction makes it a no-op when the level is disabled — which is the common case in production.
Two things worth calling out:
-
This pattern scales beyond debug. Same treatment should probably be applied to any per-frame
logger.infoorlogger.tracecalls that construct non-trivial metadata — theif (logger.isLevelEnabled("debug")) { logger.debug({...}) }idiom is more verbose but saves real per-frame work. Worth a one-time audit acrossrenderOrchestrator.tsto catch other instances. -
Testing
isLevelEnabledbeing optional. The PR's "optional" qualifier is interesting — if a host logger doesn't implementisLevelEnabled, the code should fall back to constructing + emitting. Worth a unit test confirming both branches (with and without the method) work correctly, since the perf win is only realized when hosts actually implement the method.
Approved.
— Rames Jusso
6d2f104 to
6a9fcf1
Compare
092e9ba to
fdbbfd2
Compare
24db6e0 to
2c11740
Compare
fdbbfd2 to
3f90688
Compare
|
Thanks for the approval @jrusso1020. Both follow-ups landed in this PR. 1. One-time audit across renderOrchestrator.ts — done. Audited every per-frame `log.{info,debug,trace}` site in the streaming HDR encode loop. Findings, with the audit committed inline as a comment at `packages/producer/src/services/renderOrchestrator.ts:1970-1982` so the next person to add a hot-path log site doesn't have to redo the work:
The only happy-path per-frame log that constructed metadata was the one at line 1985 (`[Render] HDR layer composite frame`), which is now both `i % 30 === 0` and `isLevelEnabled` gated. Net result: in production (level=info, KEEP_TEMP unset) the streaming HDR encode loop performs zero allocations for logging metadata. 2. Both branches of `isLevelEnabled`-optional are tested — done. `packages/producer/src/logger.test.ts`:
The third test is the one that mattered most — without it, a refactor that changed `?? true` to `?? false` would silently drop diagnostics for any host whose logger doesn't implement the optional method, with no test failure to catch it. No outstanding action items. |
3f90688 to
bd86b42
Compare
29ab461 to
a4873ca
Compare
a549e46 to
8d9641b
Compare
a4873ca to
35fee67
Compare
8d9641b to
3fcaa60
Compare
35fee67 to
2159cf0
Compare
3fcaa60 to
e675bd9
Compare
2159cf0 to
d746856
Compare
e675bd9 to
f73328c
Compare
2217a93 to
8610f7e
Compare
f73328c to
55a41a8
Compare
8610f7e to
0d2175b
Compare
55a41a8 to
349f0f6
Compare
0d2175b to
cd40e4b
Compare
349f0f6 to
cd65ab0
Compare
cd40e4b to
5594d94
Compare
cd65ab0 to
5409b1a
Compare
5594d94 to
4ea0021
Compare
5409b1a to
c6a8280
Compare
4ea0021 to
c95ffe5
Compare
Add an optional `isLevelEnabled(level)` method to `ProducerLogger` so call
sites can short-circuit expensive metadata construction in hot loops
before handing it to a debug logger. Implement it in
`createConsoleLogger` and gate the per-frame HDR composite snapshot in
`renderOrchestrator` (every 30 frames) on
`log.isLevelEnabled?.("debug") ?? true`, so production runs at
`level="info"` skip the `Array.find` + `toFixed` + struct allocation
entirely while custom loggers without the new method keep their
existing behavior.
Also add unit coverage in `packages/producer/src/logger.test.ts` (17
tests) for level filtering, meta formatting, the new `isLevelEnabled`
path including a hot-loop call-site simulation that asserts zero
builder invocations at info level, and the `?? true` fallback for
loggers that omit the method.
Update `docs/packages/producer.mdx` with a new "Logging" section
documenting `ProducerLogger`, `createConsoleLogger`, `defaultLogger`,
and the `isLevelEnabled` gating pattern.
This closes 8C in `plans/hdr-followups.md`. 8D (gating the
`countNonZeroAlpha` / `countNonZeroRgb48` diagnostic counters) was
verified during the same work to already be guarded by
`shouldLog = debugDumpEnabled && debugFrameIndex >= 0`, where
`debugDumpEnabled` is itself driven by `KEEP_TEMP=1`, so the pixel
iteration is fully skipped on production runs and no code change was
needed.
Made-with: Cursor
c6a8280 to
2ce89a5
Compare

Summary
Add an optional
isLevelEnabled(level)method toProducerLoggerand use it to short-circuit per-frame HDR composite metadata construction inrenderOrchestratorwhen the log level is above debug.Closes Chunks 8C and 8D from
plans/hdr-followups.md.Why
Chunk 8Cofplans/hdr-followups.md. The per-frame HDR composite snapshot (every 30 frames) was building anArray.find+toFixed+ struct allocation unconditionally and handing it to a debug logger that immediately discarded it atlevel="info". On long renders, this is allocation pressure and CPU time wasted on log meta nobody reads.Chunk 8Dwas investigated in the same pass and found to already be guarded — see below.What changed
isLevelEnabled(level: ProducerLogLevel): booleanonProducerLogger.createConsoleLoggerimplements it.renderOrchestrator.tsper-frame HDR composite snapshot is now gated oni % 30 === 0 && (log.isLevelEnabled?.("debug") ?? true)— production runs atlevel="info"skip the meta-object construction entirely; custom loggers without the new method keep their existing behavior thanks to the?? truefallback.packages/producer/src/logger.test.ts(17 tests) covering level filtering, meta formatting, theisLevelEnabledpath, a hot-loop call-site simulation that asserts zero builder invocations at info level, and the?? truefallback for loggers that omit the method.docs/packages/producer.mdxgains a new "Logging" section documentingProducerLogger,createConsoleLogger,defaultLogger, and theisLevelEnabledgating pattern.8D resolution (no code change).
countNonZeroAlpha/countNonZeroRgb48calls live behindshouldLog = debugDumpEnabled && debugFrameIndex >= 0, wheredebugDumpEnabledis itself driven byKEEP_TEMP=1. The pixel iteration is fully skipped on production runs already, so 8D needed no fix — verified during the 8C work.Test plan
bun testin producer — 17/17 logger tests pass; existing service tests unchanged.level="info".?? truefallback preserves prior behavior for custom logger implementations that don't define the method.Stack
Chunks 8C + 8D of
plans/hdr-followups.md. Sits on top of the benchmark harness PR (Chunk 8A) so the optimization is measurable.