perf(core): Lazy-load jschardet and iconv-lite in fileRead#1401
perf(core): Lazy-load jschardet and iconv-lite in fileRead#1401
Conversation
Defer importing jschardet and iconv-lite until first encounter of a non-UTF-8 file. The fast UTF-8 path (covers ~99% of source code files) never needs these libraries, saving ~25ms of combined import cost at startup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
⚡ Performance Benchmark
Details
History1bbaa94 perf(core): Lazy-load jschardet and iconv-lite in fileRead
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/file/fileRead.ts (1)
9-16: Consider caching the import promise to avoid redundant imports under concurrent calls.If multiple files fail UTF-8 decoding simultaneously, concurrent calls to
getEncodingDeps()could each triggerPromise.allbefore the cache variables are set. This is a minor inefficiency rather than a correctness bug, but can be avoided by caching the promise itself:♻️ Optional: Promise-based singleton pattern
let _jschardet: typeof import('jschardet') | undefined; let _iconv: typeof import('iconv-lite') | undefined; +let _encodingDepsPromise: Promise<{ jschardet: typeof import('jschardet'); iconv: typeof import('iconv-lite') }> | undefined; + const getEncodingDeps = async () => { - if (!_jschardet || !_iconv) { - [_jschardet, _iconv] = await Promise.all([import('jschardet'), import('iconv-lite')]); + if (!_encodingDepsPromise) { + _encodingDepsPromise = Promise.all([import('jschardet'), import('iconv-lite')]).then(([jschardet, iconv]) => { + _jschardet = jschardet; + _iconv = iconv; + return { jschardet, iconv }; + }); } - return { jschardet: _jschardet, iconv: _iconv }; + return _encodingDepsPromise; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/file/fileRead.ts` around lines 9 - 16, The current getEncodingDeps function can trigger multiple concurrent dynamic imports because it only caches loaded modules (_jschardet, _iconv); add a cached promise (e.g. _encodingDepsPromise) to act as a singleton for the in-flight import and change getEncodingDeps to return/await that promise when present, creating and assigning the Promise (which does the Promise.all([import('jschardet'), import('iconv-lite')]) and sets _jschardet/_iconv) only once; this ensures `_jschardet`, `_iconv`, and `getEncodingDeps` use the same import promise under concurrent calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/file/fileRead.ts`:
- Around line 9-16: The current getEncodingDeps function can trigger multiple
concurrent dynamic imports because it only caches loaded modules (_jschardet,
_iconv); add a cached promise (e.g. _encodingDepsPromise) to act as a singleton
for the in-flight import and change getEncodingDeps to return/await that promise
when present, creating and assigning the Promise (which does the
Promise.all([import('jschardet'), import('iconv-lite')]) and sets
_jschardet/_iconv) only once; this ensures `_jschardet`, `_iconv`, and
`getEncodingDeps` use the same import promise under concurrent calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2370b451-d7f0-4847-a212-451e023c95a2
📒 Files selected for processing (1)
src/core/file/fileRead.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1401 +/- ##
==========================================
+ Coverage 87.40% 87.41% +0.01%
==========================================
Files 116 116
Lines 4389 4393 +4
Branches 1018 1018
==========================================
+ Hits 3836 3840 +4
Misses 553 553 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying repomix with
|
| Latest commit: |
438fd99
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c13da98.repomix.pages.dev |
| Branch Preview URL: | https://perf-lazy-load-encoding-libs.repomix.pages.dev |
Code Review —
|
…ngDeps Guarantees exactly one import regardless of concurrent slow-path calls. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| _encodingDepsPromise ??= Promise.all([import('jschardet'), import('iconv-lite')]).then(([jschardet, iconv]) => ({ | ||
| jschardet, | ||
| iconv, | ||
| })); |
There was a problem hiding this comment.
🔴 Dynamic import of iconv-lite exposes only default, causing encodingExists/decode to be undefined at Node.js runtime
When iconv-lite (a CommonJS module) is loaded via dynamic import(), Node.js wraps module.exports under the .default property. Unlike jschardet (which Node.js can statically analyze for named exports), iconv-lite builds its exports dynamically (iconv = module.exports; iconv.encode = function…), so Node.js does not hoist named exports. The result is that encodingDeps.iconv.encodingExists and encodingDeps.iconv.decode are both undefined at runtime, even though TypeScript's type declarations make them appear valid.
I verified this empirically against the installed iconv-lite@0.7.x:
typeof deps.iconv.encodingExists → undefined
typeof deps.iconv.decode → undefined
typeof deps.iconv.default.encodingExists → function
typeof deps.iconv.default.decode → function
This means every non-UTF-8 file (the "slow path" at lines 73-77) will throw a TypeError: encodingDeps.iconv.encodingExists is not a function, which is caught by the outer catch at src/core/file/fileRead.ts:85 and silently returned as { content: null, skippedReason: 'encoding-error' }. Files that were previously decoded correctly (e.g. Shift-JIS, EUC-KR) are now incorrectly skipped. The existing tests pass because Vitest's CJS/ESM interop differs from Node.js runtime behavior, masking the bug.
| _encodingDepsPromise ??= Promise.all([import('jschardet'), import('iconv-lite')]).then(([jschardet, iconv]) => ({ | |
| jschardet, | |
| iconv, | |
| })); | |
| _encodingDepsPromise ??= Promise.all([import('jschardet'), import('iconv-lite')]).then(([jschardet, iconv]) => ({ | |
| jschardet, | |
| iconv: iconv.default ?? iconv, | |
| })); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Defer importing
jschardetandiconv-liteuntil first encounter of a non-UTF-8 file. The fast UTF-8 path (covers ~99% of source code files) never needs these libraries, saving ~25ms of combined import cost at startup.Extracted from #1377. Pure performance optimization with no functional changes.
Checklist
npm run testnpm run lint