Conversation
…cate @secretlint/profiler singletons The previous @secretlint/profiler optimization (cfc626a) silently regressed after the merge from main upgraded @secretlint/core to 11.5.0. That release declares an exact-version peer dep on @secretlint/profiler@11.5.0, which forced npm to install a nested second copy under `node_modules/@secretlint/core/node_modules/@secretlint/profiler` alongside our top-level 11.4.1 profiler. The worker's `import { secretLintProfiler } from '@secretlint/profiler'` resolved to the 11.4.1 singleton, so the `Object.defineProperty(secretLintProfiler, 'mark', ...)` no-op patched the wrong instance — the copy @secretlint/core actually calls at lint time (11.5.0) kept running its O(n^2) PerformanceObserver bookkeeping. As a result the security phase was back to ~2.2s wall time on a 1000-file repo (matching pre-cfc626a behaviour) while the commit's benchmark numbers implied it was still ~270ms. Switch the neutralization to a strictly more robust primitive: overwrite `perf_hooks.performance.mark` with a no-op inside the worker thread. Every copy of @secretlint/profiler — hoisted, nested, or future — calls `this.perf.mark(...)` on the single Node built-in `performance` object it received at construction time from `node:perf_hooks`, so a single assignment on that object silences every profiler instance simultaneously. Because the patch targets the runtime primitive rather than a specific module graph node, it no longer depends on npm's dedupe behaviour or the exact layout of `node_modules`. The worker thread is isolated and runs only secretlint; no other code in it depends on `performance.mark`, so there is no observable side effect. Also removes the now-unused direct `@secretlint/profiler` dependency from package.json. 1. Both profiler singletons construct `new SecretLintProfiler({ perf: perf_hooks.performance, ... })` 2. Both call `this.perf.mark(marker.type)` for every event 3. Overwriting `perf_hooks.performance.mark` on the shared Node built-in makes both calls a no-op, so the PerformanceObserver callback never fires, the `entries` array stays empty, and the O(n^2) `entries.find()` scan is eliminated Verified on this host (4-core Linux container) against origin/main, which is what the CI `perf-benchmark.yml` workflow compares. Interleaved pairs, no verbose mode, 40 runs. | Stat | main (ms) | PR (ms) | Δ | |--------------|-----------|---------|-----------------| | min | 1965 | 1914 | -51ms (-2.60%) | | median | 2181 | 2066 | -115ms (-5.27%) | | trimmed mean | 2216 | 2103 | -113ms (-5.11%) | | mean | 2282 | 2159 | -123ms (-5.38%) | | Stat | main (ms) | PR (ms) | Δ | |--------------|-----------|---------|-----------------| | min | 2445 | 2309 | -136ms (-5.56%) | | median | 2628 | 2495 | -133ms (-5.06%) | | trimmed mean | 2624 | 2509 | -114ms (-4.36%) | | mean | 2658 | 2518 | -141ms (-5.29%) | | Phase | branch pre-fix | PR | Δ | |--------------------|----------------|----------|------------| | File collection | ~471 ms | ~446 ms | ~-25 ms | | File processing | ~103 ms | ~95 ms | ~-8 ms | | **Security check** | **~2229 ms** | **~217 ms** | **~-2012 ms** | | Git log tokens | ~446 ms | ~451 ms | ~0 | | Selective metrics | ~454 ms | ~455 ms | ~0 | | Output token count | ~610 ms | ~610 ms | ~0 | In non-verbose mode the security-phase work overlaps heavily with other async phases, so end-to-end savings are smaller than the phase-timer drop. Verbose mode amplifies the observer cost via extra logger traces, making the isolation of the profiler bottleneck visible. - [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 an RSA private key block; secretlint still flags and excludes it)
⚡ Performance Benchmark
Details
History68303d0 perf(security): Patch perf_hooks.performance.mark to neutralize duplicate @secretlint/profiler singletons
|
|
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:
📝 WalkthroughWalkthroughRemoved 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1456 +/- ##
==========================================
- Coverage 87.20% 87.13% -0.07%
==========================================
Files 117 117
Lines 4439 4440 +1
Branches 1022 1023 +1
==========================================
- Hits 3871 3869 -2
- Misses 568 571 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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.
Deploying repomix with
|
| Latest commit: |
3af40d0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ae6e9de9.repomix.pages.dev |
| Branch Preview URL: | https://perf-security-neutralize-pro.repomix.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Address two review findings on #1456: - Critical (devin-ai-integration): This module is also imported from src/mcp/tools/fileSystemReadFileTool.ts in the MCP server main process. Applying the `Object.defineProperty` at module load would silently replace `performance.mark` process-wide, affecting any other code in the main process that relies on the Node built-in. Gate the patch on `!isMainThread` from `node:worker_threads` so it only runs inside Tinypool security-check worker threads, where this module is the sole runtime. In the MCP main-process path only a handful of files are linted per call, so leaving the profiler active there has no measurable cost. - Defensive (gemini-code-assist): Restore the try/catch around the `Object.defineProperty` call so a future Node.js version making `performance.mark` non-configurable would skip the optimization rather than crashing every security worker at load time.
Code Review — Claude (Incremental)Verdict: Approve. The second commit ( Since the previous Claude review covered the core approach thoroughly, this focuses on what changed in commit 2 and any remaining observations. Commit 2 Assessment (
|
Take main's version which includes the isMainThread guard and try/catch from PR #1456 (fix(security): Scope performance.mark patch to worker threads only).
2 participants
Summary
Fix a silent regression in the previous
@secretlint/profileroptimization (#1453 /cfc626a) and replace it with a strictly more robust approach.What regressed
After #1453 merged,
@secretlint/core@11.5.0landed on main. That release declares an exact-version peer dep on@secretlint/profiler@11.5.0. Because our top-levelpackage.jsonpinned@secretlint/profiler@^11.5.0as a direct dependency, npm ended up installing a second nested copy at:The worker's
import { secretLintProfiler } from '@secretlint/profiler'resolved to the top-level singleton, soObject.defineProperty(secretLintProfiler, 'mark', ...)no-op'd the wrong instance. The copy@secretlint/coreactually calls at lint time was the nested one, which kept running its O(n²)PerformanceObserverbookkeeping. Security phase wall time silently reverted to ~2.2s on a 1000-file repo.The fix
Patch
perf_hooks.performance.markdirectly instead of the profiler singleton:Why this is strictly better:
@secretlint/profilercopy — hoisted, nested, or future — constructs itself with{ perf: perf_hooks.performance }and callsthis.perf.mark(...)for every event. All copies share the single Node built-inperformanceobject, so one assignment silences every profiler instance simultaneously.node_moduleslayout / npm dedupe / version drift.performance.mark, so there is no observable side effect.Performance.prototype.mark— the prototype is left alone, so no otherPerformanceinstance (if any existed) in the Node process would be affected.Also removes the now-unused direct
@secretlint/profilerdependency frompackage.json.Benchmark
End-to-end wall time — PR vs
origin/main(4 cores, 40 interleaved runs)Same comparison under 2-CPU constraint (
taskset -c 0-1, 30 runs)Phase-level evidence (verbose mode, single run)
In non-verbose mode the security phase overlaps heavily with other async phases, so end-to-end savings are smaller than the phase-timer drop. Verbose mode amplifies the observer cost via extra logger traces, making the profiler bottleneck visible in isolation.
Checklist
npm run lint— 0 errors (2 pre-existing warnings in unrelated files)npm run test— 1102/1102 tests pass