perf: reduce per-event allocations in streaming pipeline#93
Conversation
- Move TextEncoder to module-level singleton in sdk-child.ts instead of creating a new instance per streamed event - Cache logger env-var reads (log level, silent, console) at module load instead of hitting process.env on every shouldLog/isConsoleEnabled call - Update logger tests to call _resetLoggerState() after setting env vars so cached values are refreshed Part of #92
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo unrelated micro-optimizations: ChangesTextEncoder reuse in sdk-child
Logger environment-variable caching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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}
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 (1)
tests/unit/utils/logger.test.ts (1)
28-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove
_resetLoggerState()call to after environment cleanup.The current order calls
_resetLoggerState()at line 30 before clearing environment variables (lines 32-34), which means the module-level cache is populated with potentially stale values and then the env vars are cleared, leaving the cached state inconsistent with the cleaned environment.If the test environment has any of these variables set (e.g.,
CURSOR_ACP_LOG_CONSOLE=1), the cachedCONSOLE_ENABLEDwould betrueeven after the env var is deleted, potentially causing tests like "writes logs to file by default (not console)" to fail.🔧 Proposed fix
beforeEach(() => { vi.clearAllMocks(); - _resetLoggerState(); // Reset module-level state before each test process.env = { ...originalEnv }; delete process.env.CURSOR_ACP_LOG_LEVEL; delete process.env.CURSOR_ACP_LOG_SILENT; delete process.env.CURSOR_ACP_LOG_CONSOLE; + _resetLoggerState(); // Reset module-level state AFTER clearing env vars });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/utils/logger.test.ts` around lines 28 - 35, The _resetLoggerState() function is being called before the environment variables are cleaned up in the beforeEach hook. Move the _resetLoggerState() call to execute after all the process.env deletions (after the delete process.env.CURSOR_ACP_LOG_CONSOLE line) so that the module-level cache is reset based on the cleaned environment state rather than being populated with potentially stale values before the environment is cleared.
🧹 Nitpick comments (1)
src/utils/logger.ts (1)
30-37: ⚡ Quick winConsider extracting env-reading logic into helper functions.
The boolean logic for
IS_SILENT(lines 31-33) andCONSOLE_ENABLED(lines 34-36) is duplicated in_resetLoggerState()(lines 69-74). Following the pattern ofreadConfiguredLevel(), consider extracting these into helper functions to eliminate duplication and ensure consistency.♻️ Suggested refactor
+function readIsSilent(): boolean { + return ( + process.env.CURSOR_ACP_LOG_SILENT === "1" || + process.env.CURSOR_ACP_LOG_SILENT === "true" + ); +} + +function readConsoleEnabled(): boolean { + return ( + process.env.CURSOR_ACP_LOG_CONSOLE === "1" || + process.env.CURSOR_ACP_LOG_CONSOLE === "true" + ); +} + let CONFIGURED_LEVEL: LogLevel = readConfiguredLevel(); -let IS_SILENT: boolean = - process.env.CURSOR_ACP_LOG_SILENT === "1" || - process.env.CURSOR_ACP_LOG_SILENT === "true"; -let CONSOLE_ENABLED: boolean = - process.env.CURSOR_ACP_LOG_CONSOLE === "1" || - process.env.CURSOR_ACP_LOG_CONSOLE === "true"; +let IS_SILENT: boolean = readIsSilent(); +let CONSOLE_ENABLED: boolean = readConsoleEnabled(); let CONFIGURED_PRIORITY = LEVEL_PRIORITY[CONFIGURED_LEVEL];Then update
_resetLoggerState():export function _resetLoggerState(): void { logDirEnsured = false; logFileError = false; CONFIGURED_LEVEL = readConfiguredLevel(); - IS_SILENT = - process.env.CURSOR_ACP_LOG_SILENT === "1" || - process.env.CURSOR_ACP_LOG_SILENT === "true"; - CONSOLE_ENABLED = - process.env.CURSOR_ACP_LOG_CONSOLE === "1" || - process.env.CURSOR_ACP_LOG_CONSOLE === "true"; + IS_SILENT = readIsSilent(); + CONSOLE_ENABLED = readConsoleEnabled(); CONFIGURED_PRIORITY = LEVEL_PRIORITY[CONFIGURED_LEVEL]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/logger.ts` around lines 30 - 37, The environment variable reading logic for IS_SILENT and CONSOLE_ENABLED is duplicated between the module-level initialization and the _resetLoggerState() function. Following the existing pattern of readConfiguredLevel(), create two new helper functions (e.g., readIsSilent() and readConsoleEnabled()) that encapsulate the boolean logic for checking CURSOR_ACP_LOG_SILENT and CURSOR_ACP_LOG_CONSOLE environment variables respectively. Then replace the duplicated logic in both the initial variable assignments and the _resetLoggerState() function with calls to these helper functions to ensure consistency and eliminate code duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/unit/utils/logger.test.ts`:
- Around line 28-35: The _resetLoggerState() function is being called before the
environment variables are cleaned up in the beforeEach hook. Move the
_resetLoggerState() call to execute after all the process.env deletions (after
the delete process.env.CURSOR_ACP_LOG_CONSOLE line) so that the module-level
cache is reset based on the cleaned environment state rather than being
populated with potentially stale values before the environment is cleared.
---
Nitpick comments:
In `@src/utils/logger.ts`:
- Around line 30-37: The environment variable reading logic for IS_SILENT and
CONSOLE_ENABLED is duplicated between the module-level initialization and the
_resetLoggerState() function. Following the existing pattern of
readConfiguredLevel(), create two new helper functions (e.g., readIsSilent() and
readConsoleEnabled()) that encapsulate the boolean logic for checking
CURSOR_ACP_LOG_SILENT and CURSOR_ACP_LOG_CONSOLE environment variables
respectively. Then replace the duplicated logic in both the initial variable
assignments and the _resetLoggerState() function with calls to these helper
functions to ensure consistency and eliminate code duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b34bb98c-29b2-4f8a-a6e8-f14cefbb925f
📒 Files selected for processing (3)
src/client/sdk-child.tssrc/utils/logger.tstests/unit/utils/logger.test.ts
Run _resetLoggerState after clearing log-related env vars so cached logger settings match the test harness instead of the parent shell. Add coverage for log-level env caching and refresh.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Nomadcxx
force-pushed
the
fix/streaming-pipeline-perf
branch
from
4b730db to
dcb04b9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
TextEncodersingleton instead ofnew TextEncoder()on every streamed event. Eliminates object allocation churn on the hot path.CURSOR_ACP_LOG_LEVEL,CURSOR_ACP_LOG_SILENT, andCURSOR_ACP_LOG_CONSOLEat module load. Replaces per-callprocess.envreads inshouldLog()andisConsoleEnabled()with cached constants._resetLoggerState()re-reads env for tests._resetLoggerState()after so cached values pick them up.Relates to #92 — this is the first of two PRs targeting quick wins from the performance audit. These are internal-only changes with no API or behaviour impact.
What this does NOT change
MixedDeltaTrackerstays — git history shows it exists to handle mixed delta/accumulated streams and swapping it would reintroduce duplicate output)Test plan
npm run buildcleanSummary by CodeRabbit
Refactor
Tests