perf(diagnostics): считать диагностики на bounded-пуле вместо ForkJoinPool#4264
Conversation
…of ForkJoinPool DefaultDiagnosticComputer вычислял диагностики через CompletableFuture.supplyAsync(...).join() + parallelStream() на diagnosticComputerExecutor (ForkJoinPool). Когда getDiagnostics вызывается из воркера ForkJoinPool (пакетный analyze через cliExecutor, AnalyzeProjectOnStart через analyzeOnStartExecutor), блокирующий join() — это managed blocking: пул поднимает компенсирующие потоки, каждый лишний воркер материализует ещё один DocumentContext, и естественный потолок «≈ число ядер» теряется (претензия §4.2 из #4248). Теперь каждая диагностика — отдельная задача на diagnosticComputerExecutor, результат собирается через Future.get(). Сам diagnosticComputerExecutor переведён с ForkJoinPool на bounded ThreadPoolExecutor: блокировка на get() обычного пула не проходит через ForkJoinPool.managedBlock, поэтому вызов из FJP-воркера компенсирующих потоков не плодит. Контекст workspace несут per-task снапшоты ContextPropagatingExecutorService. Замеры на SSL 3.2 (4 ядра, JFR + /proc, вывод идентичен): - analyze: thread-starts на cli-пуле 17→3, sustained active 45→31, SARIF 72160=72160. - LSP (AnalyzeProjectOnStart): thread-starts на analyze-on-start 17→3, sustained active ~45→~32, диагностик 72141=72141 по 2184 файлам. Компенсация снимается на обоих путях, в т.ч. на LSP-пути, которого внешняя ось batch-анализа не касается. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01BSiRGLm633B4EmvG3vkk4V
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDiagnostic computation now submits individual diagnostic tasks to a workspace-bounded executor, collects ChangesDiagnostic execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DefaultDiagnosticComputer
participant ExecutorService
participant BSLDiagnostic
DefaultDiagnosticComputer->>ExecutorService: submit one task per diagnostic
ExecutorService->>BSLDiagnostic: compute diagnostics
BSLDiagnostic-->>ExecutorService: return diagnostic list
ExecutorService-->>DefaultDiagnosticComputer: collect Future results
DefaultDiagnosticComputer->>DefaultDiagnosticComputer: filter ignored diagnostics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
| @WorkspaceScope(proxyMode = ScopedProxyMode.INTERFACES) | ||
| public ExecutorService diagnosticComputerExecutor() { | ||
| return createWorkspaceForkJoinPool("diagnostic-computer-"); | ||
| return createWorkspaceBoundedPool("diagnostic-computer-"); |
There was a problem hiding this comment.
В соседних методах нет термина bounded. Делай по подобию.
![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/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DefaultDiagnosticComputer.java (1)
42-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim calling-scenario details out of the class Javadoc.
Lines 48-52 describe who invokes this class and when (batch analyze,
AnalyzeProjectOnStartworkers), which the path instructions for this directory explicitly ask Javadoc to avoid. The rationale about avoiding managed-blocking compensation is useful, but the specific caller scenarios ("пакетный анализ, анализ проекта при старте") belong in an implementation-level comment (as already done inExecutorConfiguration), not in the public class contract Javadoc.As per path instructions,
src/main/java/**/*.java: "Javadoc should describe the contract of a class or method (what it does, inputs/outputs, invariants, side effects), not calling scenarios or who invokes it."📝 Suggested trim
/** * Реализация {`@link` DiagnosticComputer} по умолчанию. * <p> * Параллельно вычисляет диагностики всеми зарегистрированными анализаторами {`@link` BSLDiagnostic} * с обработкой ошибок и фильтрацией по правилам подавления и игнорируемым авторам. * <p> - * Каждый анализатор — отдельная задача на {`@code` diagnosticComputerExecutor}; результаты - * собираются через {`@link` Future#get()}. Блокировка на {`@code` get()} обычного пула, в отличие от - * {`@code` parallelStream()} на {`@code` ForkJoinPool} с блокирующим {`@code` join()}, не запускает - * managed-blocking компенсацию, поэтому вызов из воркера {`@code` ForkJoinPool} (пакетный анализ, - * анализ проекта при старте) не плодит компенсирующие потоки и живые {`@code` DocumentContext}. + * Каждый анализатор — отдельная задача на {`@code` diagnosticComputerExecutor}; результаты + * собираются через {`@link` Future#get()}. */(Move the managed-blocking/compensating-thread rationale into an inline
//comment near thecompute()implementation instead.)🤖 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/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DefaultDiagnosticComputer.java` around lines 42 - 52, Trim the class Javadoc for DefaultDiagnosticComputer to remove specific caller scenarios such as batch analysis and AnalyzeProjectOnStart, while retaining only the class contract and relevant managed-blocking rationale. Move that caller-specific rationale into an inline comment near the compute() implementation, without changing behavior.Source: Path instructions
🤖 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.
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DefaultDiagnosticComputer.java`:
- Around line 42-52: Trim the class Javadoc for DefaultDiagnosticComputer to
remove specific caller scenarios such as batch analysis and
AnalyzeProjectOnStart, while retaining only the class contract and relevant
managed-blocking rationale. Move that caller-specific rationale into an inline
comment near the compute() implementation, without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df388d2b-ead8-4a27-995d-a3fdcca8eee6
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/diagnostics/DefaultDiagnosticComputer.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/infrastructure/ExecutorConfiguration.java
…ss javadoc Address review feedback on #4264: - rename createWorkspaceBoundedPool -> createWorkspaceExecutorService, matching the sibling createSharedForkJoinExecutorService (no "bounded"/"pool" in the name; the method returns a wrapped ExecutorService). - move the managed-blocking / caller-scenario rationale out of the DefaultDiagnosticComputer class javadoc into an inline comment in compute(), per the repo convention that javadoc describes the contract, not callers. No behavior change. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01BSiRGLm633B4EmvG3vkk4V
Add a unit test for the new Future-based collection paths that SonarCloud flagged as uncovered on new code: results merged from all analyzers, a diagnostic throwing RuntimeException is skipped (others still returned), and a non-runtime failure surfaces as IllegalStateException from Future collection. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01BSiRGLm633B4EmvG3vkk4V
|
2 participants
Описание
DefaultDiagnosticComputerоборачивал расчёт вCompletableFuture.supplyAsync(...).join()наdiagnosticComputerExecutor(ForkJoinPool) и разворачивал анализаторы черезparallelStream(). КогдаgetDiagnostics()вызывается из воркераForkJoinPool— а это и пакетныйanalyze(cliExecutor), и анализ проекта при старте LSP (AnalyzeProjectOnStart→analyzeOnStartExecutor) — этотjoin()является managed blocking: пул поднимает компенсирующие потоки, чтобы держать номинальный параллелизм, и каждый лишний воркер материализует ещё одинDocumentContext. Естественный потолок «≈ число ядер» теряется.Фикс
Сабмитить каждый анализатор отдельной задачей на
diagnosticComputerExecutorи собирать результат черезFuture.get(). Блокировка наget()обычногоThreadPoolExecutorне проходит черезForkJoinPool.managedBlock, поэтому блокирующий вызов из воркераForkJoinPoolне плодит компенсирующие потоки.diagnosticComputerExecutorпереведён сForkJoinPoolна boundedThreadPoolExecutor(контекст workspace несётContextPropagatingExecutorServiceper-task); единственный потребитель —DefaultDiagnosticComputer.В отличие от #4263 (bounded-пул только на внешней оси
analyze), фикс на уровне diagnostic computer снимает компенсацию на всех путях, где диагностики считаются изForkJoinPool, — включая LSP-путьAnalyzeProjectOnStart, которого #4263 не касался.Замеры (JFR + /proc, SSL 3.2, 4 ядра,
commonPoolParallelism=4, тёплый кэш)Вывод идентичен до/после (
analyze— 72160 диагностик, LSP — 72141, 2184 файла).analyzeOnStart(было → стало)cli-*/analyze-on-start-*)compute()(≈ живыхDocumentContext)Про память. На просторной куче (
-Xmx6g) peak RSS почти не меняется — он определяется темпом аллокаций (одинаковым: работа та же) и нежеланием JVM отдавать страницы ОС, а не live-set. Эффект проявляется под давлением кучи: при зажатом-Xmxbaseline с 16 живыми документами упирается в потолок заметно чаще (Full GC при-Xmx1000m: 8 → 2; при-Xmx700m: 76 → 24), при паритетном выводе. Это ровно механика §4.2 #4248 (много ядер + крупная конфигурация → компенсация плодит десятки живыхDocumentContext→ 16–22 ГиБ / не заканчивается).Связанные задачи
Пункт §4.2 архитектурного разбора #4248. Альтернатива #4263 (закрывается в пользу этого PR как более широкая по охвату). Продолжение серии #4249–#4258.
Closes
Чеклист
Общие
DiagnosticProviderTest,AnalyzeCommandTest,ArchitectureTest— зелёные; вывод SARIF идентичен на реальном прогоне SSL, в т.ч. по LSP-пути)Дополнительно
diagnosticComputerExecutor— единственный потребитель — теперь boundedThreadPoolExecutorразмеромcommonPoolParallelism; при желании легко вынести в конфиг. Взаимоблокировки нет: внешние пулы (cliExecutor/analyzeOnStartExecutor) иdiagnosticComputerExecutor— разные пулы без обратной зависимости.Generated by Claude Code
Summary by CodeRabbit