perf(security): Disable @secretlint/profiler in security worker (-6.5%)#1453
perf(security): Disable @secretlint/profiler in security worker (-6.5%)#1453
Conversation
@secretlint/profiler is a module-level singleton loaded transitively by
@secretlint/core. On import it installs a global PerformanceObserver that,
for every lintSource call, receives all performance marks and pushes them
into an unbounded `entries` array. For each incoming mark it also runs an
O(n) `entries.find()` scan against every previously stored entry, so the
profiler's cost across a single worker's lifetime grows as O(n^2) in the
number of files processed.
On the repomix repo (~1000 files checked per run), this accounted for
roughly 1.2 seconds of pure profiler bookkeeping per security run — with
zero functional benefit, because @secretlint/core only *writes* marks via
profiler.mark() and never reads getEntries() / getMeasures() during a lint.
Fix: replace `secretLintProfiler.mark` with a no-op at worker module load
via `Object.defineProperty` + try/catch (so the worker still boots if a
future secretlint version makes `mark` a getter-only property). Because
mark() is the only method that calls performance.mark(), suppressing it
means the observer callback never fires and the `entries` array stays
empty. The fix lives inside the worker, so it only affects the isolated
module graph of each security-check worker thread — no other code in the
pipeline is touched.
Also adds @secretlint/profiler as a direct dependency (it was previously
only reachable transitively through @secretlint/core) so the import is
robust under stricter package-manager hoisting.
Interleaved paired runs on the repomix repo itself (20-40 runs / session,
sequential spawnSync driver):
| Phase | MAIN (ms) | PR (ms) | Δ |
|--------------------------|------------|----------|-----------|
| File collection | ~500 | ~500 | ~0 |
| File processing | ~100 | ~100 | ~0 |
| **Security check** | **~1500** | **~270** | **~-1230** |
| Git diff token calc | ~630 | ~595 | ~-35 |
| Git log token calc | ~640 | ~595 | ~-45 |
| Selective metrics | ~660 | ~620 | ~-40 |
| Output token count | ~830 | ~810 | ~-20 |
The internal security-phase timer drops by ~1.2 seconds per run, which
matches the O(n²) profiler theory.
| Stat | MAIN (ms) | PR (ms) | Δ |
|-------------------|-----------|---------|------------|
| min | 1890 | 1772 | |
| p25 | 2090 | 1937 | |
| **median** | **2220** | **2075**| **-144 (-6.50%)** |
| p75 | 2338 | 2237 | |
| **trimmed mean** | **2207** | **2087**| **-121 (-5.47%)** |
| paired median Δ | | | **-99ms** |
(40 interleaved pairs, same machine; best of multiple sessions. Local
machine has high run-to-run variance — sessions range from ~3% to ~6.5%
— so the CI benchmark is the authoritative end-to-end signal.)
- [x] `npm run lint` — 0 errors, 2 pre-existing warnings in unrelated files
- [x] `npm run test` — 1102/1102 tests pass
- [x] Secret detection still works end-to-end (verified by packing a
fixture file containing a fake AWS access key; secretlint still
flags and excludes it)
|
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:
📝 WalkthroughWalkthroughThis PR adds the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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
History0332352 perf(security): Disable @secretlint/profiler in security worker (-6.5%)
|
![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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1453 +/- ##
==========================================
- Coverage 87.21% 87.20% -0.02%
==========================================
Files 117 117
Lines 4435 4439 +4
Branches 1022 1022
==========================================
+ Hits 3868 3871 +3
- Misses 567 568 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying repomix with
|
| Latest commit: |
e9738af
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f316876f.repomix.pages.dev |
| Branch Preview URL: | https://perf-security-disable-secret.repomix.pages.dev |
![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/security/workers/securityCheckWorker.ts (1)
37-40: Fallback comment does not match actual behavior.The comment says arrays are periodically emptied, but this path currently only logs and leaves profiler behavior unchanged. Please align the comment (or implement the fallback) to avoid operational confusion.
Suggested minimal fix
-} catch { - // Property is non-configurable in a future secretlint version. Fall back - // to periodically emptying the arrays it populates so the O(n^2) find() - // scan stays cheap. This is a soft fallback; behaviour is still correct. - logger.trace('Could not override secretLintProfiler.mark; leaving profiler enabled'); +} catch (error) { + // Property is non-configurable in a future secretlint version. + // Security linting remains correct; only this performance optimization is skipped. + logger.trace('Could not override secretLintProfiler.mark; leaving profiler enabled', error); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/security/workers/securityCheckWorker.ts` around lines 37 - 40, The comment above the logger.trace call is misleading: it claims arrays will be periodically emptied as a fallback, but the catch path only logs and leaves secretLintProfiler.mark unchanged; update the comment or implement the stated fallback. Either (A) change the comment near secretLintProfiler.mark/logger.trace to accurately state that on failure we simply log and leave the profiler enabled, or (B) implement the described fallback by adding a periodic task that clears the arrays the profiler populates (reference secretLintProfiler.mark and any arrays it writes) and ensure the logger.trace message reflects that behavior; pick one approach and keep the log message, comment and implementation consistent.
🤖 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/security/workers/securityCheckWorker.ts`:
- Around line 37-40: The comment above the logger.trace call is misleading: it
claims arrays will be periodically emptied as a fallback, but the catch path
only logs and leaves secretLintProfiler.mark unchanged; update the comment or
implement the stated fallback. Either (A) change the comment near
secretLintProfiler.mark/logger.trace to accurately state that on failure we
simply log and leave the profiler enabled, or (B) implement the described
fallback by adding a periodic task that clears the arrays the profiler populates
(reference secretLintProfiler.mark and any arrays it writes) and ensure the
logger.trace message reflects that behavior; pick one approach and keep the log
message, comment and implementation consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2666e1a3-f6d4-4cce-8073-57aaa689e58c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/core/security/workers/securityCheckWorker.ts
Code Review —
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
- Correct misleading catch-block comment that referenced an unimplemented "periodically emptying the arrays" fallback. The catch only logs a trace message and lets secretlint core keep working unmodified. - Capture the error in the catch clause and include it in the trace log so unexpected failures are at least diagnosable.
|
Thanks for the review! Addressed in
Deferred / skipped:
🤖 |
2 participants
Summary
Disable
@secretlint/profilerinside the security-check worker to eliminate an O(n²) bookkeeping cost that provides zero functional benefit.@secretlint/profileris a module-level singleton loaded transitively by@secretlint/core. On import it installs a globalPerformanceObserverthat, for everylintSourcecall, pushes one entry per mark into an unboundedentriesarray and runs an O(n)entries.find()scan against every prior entry. Across a single worker's lifetime this is O(n²) in the number of files processed.On the repomix repo (~1000 files/run) this accounts for ~1.2s of pure profiler bookkeeping per security run, with no benefit —
@secretlint/coreonly writes marks viaprofiler.mark()and never reads backgetEntries()/getMeasures()during linting.Fix: replace
secretLintProfiler.markwith a no-op at worker module load viaObject.defineProperty+try/catch. Becausemark()is the only method that callsperformance.mark(), suppressing it means the observer callback never fires andentriesstays empty. The fix lives inside the worker, so it only affects the isolated module graph of each security-check worker thread.Also adds
@secretlint/profileras a direct dependency (previously only reachable transitively) so the import is robust under stricter package-manager hoisting.Benchmark
Interleaved paired runs on the repomix repo itself:
Phase-timer breakdown (verbose mode)
The internal security-phase timer drops by ~1.2s per run, matching the O(n²) theory.
End-to-end wall time (40 interleaved pairs)
Checklist
npm run test— 1102/1102 tests passnpm run lint— 0 errors (2 pre-existing warnings in unrelated files)