perf(workspacesymbol): убрать copy-on-write бакеты и лишнюю переиндексацию#4193
Conversation
…сацию WorkspaceSymbolIndex хранит PatriciaTrie с НЕизменяемыми List-бакетами: add (concat) и remove (withoutEntries) пересобирали весь бакет через List.copyOf на каждую запись — O(N²) для вездесущих 1С-имён (бакет ≈ размер воркспейса) и основной источник Object[]-мусора. Плюс при batch-анализе каждый документ индексируется дважды (populateContext, затем rebuild на этапе диагностик через AnalyzeCommand.getFileInfoFromFile), и второй clear бил по уже наполненным бакетам под глобальным write-lock'ом — сериализуя параллельные диагностики. A. index() пропускает переиндексацию, если набор записей не изменился (snapshot.equals(indexedByUri.get(uri))). Индексируются только имя/kind/range/теги/ containerName; Entry — record со значимым equals, порядок детерминирован (обход дерева символов). Поэтому повторный проход при анализе становится no-op и вообще не берёт write-lock. B'. Бакеты trie — мутабельные ArrayList: add = bucket.add (O(1) без копии), remove = bucket.removeIf(e.uri().equals(uri)) (скан на месте, без List.copyOf). Удалены concat, withoutEntries и IdentityHashMap-removedSet в clear. Мутация — только под write-lock, бакеты наружу не отдаются (search копирует в результат), память ≈ как было. Замер analyze на nixel2007/cpm (vs develop = A+B+findCommonModule): - withoutEntries/concat уходят из CPU-профиля (были #1, 13–21%); - Object[] allocation pressure 66% → 8%; - contention на WSI write-lock (суммарный parked по потокам) 1858.6 с → 0.9 с; - diag-фаза 5:42 → 0:40; JSON-отчёт побайтно идентичен baseline. WorkspaceSymbolIndexTest 23/0/0, SymbolProviderStreamingTest 7/0/0, SymbolProviderScriptVariantTest 2/0/0, BSLWorkspaceServiceTest 26/0/0, SymbolProviderTest — зелёные. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01EWA4hy5YvUBU1CXkdr4npc
📝 WalkthroughWalkthrough
ChangesWorkspaceSymbolIndex optimizations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Бакеты trie создаются как ArrayList с ёмкостью по умолчанию (10) и растут инкрементально по мере индексации документов. Подавляющее большинство ключей дерева редкие (1–3 записи), поэтому у каждого остаётся запас ёмкости, а в сумме по множеству ключей — заметный перерасход (так мутабельные бакеты из предыдущего коммита проигрывали по памяти прежним точно-размерным List.copyOf). По ServerContextPopulatedEvent (после populateContext) backing-массивы ужимаются до фактического размера — аналог trimToSize в SymbolTreeComputer. Последующие инкрементальные правки до-растят бакеты как обычно. Замер analyze на nixel2007/cpm: live heap 3.02 ГБ → 2.94 ГБ (−80 МБ; бакеты снова точного размера, как было до перехода на мутабельные списки). Без регрессий: WorkspaceSymbolIndex вне CPU-профиля, Object[] allocation ~9%, diag-фаза 0:36, JSON-отчёт побайтно идентичен. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01EWA4hy5YvUBU1CXkdr4npc
…при идентичном содержимом
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java`:
- Around line 702-715: The snapshot equality check against indexedByUri.get(uri)
on line 706 and the subsequent clear(uri) call are not protected by a single
lock, creating a potential race condition if event dispatch becomes
asynchronous. To fix this, wrap both the snapshot equality check and the
clear(uri) invocation within a single read-lock section to ensure atomicity of
the decision-and-mutation sequence. Alternatively, if synchronous event dispatch
is a critical architectural requirement, document this explicitly as a required
invariant to make the current safety mechanism clear and intentional for future
maintainers. Choose the locking approach to prevent thread B from calling
clear(uri) between the time thread A evaluates the equality check and executes
its own clear(uri) call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e278874-1f8c-461b-9bb6-0aaac7618b20
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java
| var snapshot = List.copyOf(collected); | ||
| // Переиндексация — no-op, если набор записей не изменился. При batch-анализе один и тот же | ||
| // документ перестраивается дважды (populateContext, затем rebuild на этапе диагностик), но | ||
| // индексируемые данные (имя, kind, range, теги, containerName) не меняются. Entry — record со | ||
| // значимым equals, порядок детерминирован (обход дерева символов), так что снимки равны и | ||
| // пересборка trie (взятие глобального write-lock'а) пропускается полностью. | ||
| if (snapshot.equals(indexedByUri.get(uri))) { | ||
| return; | ||
| } | ||
|
|
||
| var snapshot = List.copyOf(collected); | ||
| clear(uri); | ||
| if (snapshot.isEmpty()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how content-changed and clear events are published/dispatched and whether
# same-URI processing can be concurrent (async listeners / parallel streams).
rg -nP '@(Async|EventListener)' src/main/java -g '!**/test/**' -C2 \
| rg -nP 'Async|ContentChanged|DocumentCleared|ServerContextPopulated|class ' -C1
echo '--- event publication sites ---'
rg -nP 'publishEvent\(' src/main/java -C2
echo '--- parallel population paths ---'
rg -nP 'parallelStream|\.parallel\(\)|ForkJoinPool|CompletableFuture' src/main/java -C2 \
-g '*ServerContext*' -g '*Populate*' -g '*Analyze*'Repository: 1c-syntax/bsl-language-server
Length of output: 23328
🏁 Script executed:
# First, let's look at the WorkspaceSymbolIndex class structure
ast-grep outline src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.javaRepository: 1c-syntax/bsl-language-server
Length of output: 712
🏁 Script executed:
# Read the relevant sections around lines 702-715 and the handler methods
sed -n '200,280p' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java
sed -n '700,730p' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.javaRepository: 1c-syntax/bsl-language-server
Length of output: 5228
🏁 Script executed:
# Check the handleContentChanged and handleDataCleared implementations in the parent class
sed -n '55,80p' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/AbstractDocumentLifecycleClearableIndex.javaRepository: 1c-syntax/bsl-language-server
Length of output: 768
🏁 Script executed:
# Look for the index() method that contains the disputed code
rg -n 'def index|void index' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java -A5Repository: 1c-syntax/bsl-language-server
Length of output: 465
🏁 Script executed:
# Check Spring event listener execution model and whether handlers are async
rg -n '`@Async`|`@EventListener`' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java -B2 -A5Repository: 1c-syntax/bsl-language-server
Length of output: 1605
🏁 Script executed:
# Verify the lock mechanism
sed -n '1,100p' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java | grep -n 'lock\|Lock\|synchronized'Repository: 1c-syntax/bsl-language-server
Length of output: 280
🏁 Script executed:
# Check for Spring async configuration
rg -n '`@EnableAsync`|`@Async`|SimpleApplicationEventMulticaster|TaskExecutor' src/main/java -B2 -A2Repository: 1c-syntax/bsl-language-server
Length of output: 9254
🏁 Script executed:
# Check Spring bean configuration files
find src -name '*.java' -type f | xargs grep -l 'ApplicationEventMulticaster\|SimpleApplicationEventMulticaster\|EventPublisher' 2>/dev/null | head -5Repository: 1c-syntax/bsl-language-server
Length of output: 644
🏁 Script executed:
# Look at how events are actually published and if there's async dispatch
sed -n '245,275p' src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.javaRepository: 1c-syntax/bsl-language-server
Length of output: 1681
🏁 Script executed:
# Verify the clear() method and its locking
sed -n '280,320p' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.javaRepository: 1c-syntax/bsl-language-server
Length of output: 2008
🏁 Script executed:
# Check parent class clear() to understand what happens
rg -n 'private void clear\(URI uri\)' src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java -A20Repository: 1c-syntax/bsl-language-server
Length of output: 167
The race depends on event dispatch serialization; with current synchronous dispatch the sequence is safe but fragile.
With Spring's default synchronous publishEvent() dispatch and no @Async on handleContentChanged/handleDataCleared/handleDocumentClosed/handleDocumentRemoved, content-changed and lifecycle-clear events for the same URI are processed sequentially on a single event-dispatch thread, preventing concurrent access. However, this safety is implicit in the event dispatch model, not in the WorkspaceSymbolIndex locking strategy itself. The snapshot equality check (line 706) remains outside any lock, and the decision/mutation are split across separate lock sections. If event dispatch becomes asynchronous or @Async is added to any of these handlers, the race would become real: clear(uri) on thread B could execute between the equality check and clear(uri) call on thread A, silently dropping symbols from the index until the next content change. To make safety explicit and durable, either document that synchronous event dispatch is a required invariant, or wrap the snapshot check and clear(uri) call in a single read-lock section to ensure atomicity of the decision.
🤖 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/types/index/WorkspaceSymbolIndex.java`
around lines 702 - 715, The snapshot equality check against
indexedByUri.get(uri) on line 706 and the subsequent clear(uri) call are not
protected by a single lock, creating a potential race condition if event
dispatch becomes asynchronous. To fix this, wrap both the snapshot equality
check and the clear(uri) invocation within a single read-lock section to ensure
atomicity of the decision-and-mutation sequence. Alternatively, if synchronous
event dispatch is a critical architectural requirement, document this explicitly
as a required invariant to make the current safety mechanism clear and
intentional for future maintainers. Choose the locking approach to prevent
thread B from calling clear(uri) between the time thread A evaluates the
equality check and executes its own clear(uri) call.
|
1 participant
Что и зачем
WorkspaceSymbolIndexхранитPatriciaTrie<List<Entry>>с неизменяемыми бакетами. Обе мутации пересобирали бакет целиком черезList.copyOfна каждую запись:concat): O(bucket²) на наполнение популярного ключа;withoutEntries): скан всего бакета + копия на каждый удаляемый документ.Для вездесущих 1С-имён (
ПриСозданииНаСервереи т.п.) бакет ≈ размер воркспейса → квадратика по числу файлов и основной источникObject[]-мусора. Вдобавок при batch-анализе каждый документ индексируется дважды (populateContext, затемrebuildна этапе диагностик черезAnalyzeCommand.getFileInfoFromFile), и второйclearбил по уже наполненным бакетам под глобальным write-lock'ом, сериализуя параллельные диагностики.Изменение
index()собираетsnapshotи выходит, если он равен уже проиндексированному (snapshot.equals(indexedByUri.get(uri))). Индексируются только имя/kind/range/теги/containerName;Entry— record со значимымequals, порядок детерминирован. Повторный проход при анализе становится no-op и не берёт write-lock.ArrayList; add =bucket.add(O(1) без копии), remove =bucket.removeIf(e.uri().equals(uri))(скан на месте). Удаленыconcat,withoutEntries,IdentityHashMap-removedSet. Мутации только под write-lock, бакеты наружу не отдаются.Diff — один файл, 31+/34−.
Замер
analyzeнаnixel2007/cpm(vs develop = #4188+#4189+findCommonModule)withoutEntries/concatObject[]allocation pressureГлобальный write-lock был основной точкой сериализации параллельных populate/диагностик (события Spring синхронные → слушатель на worker-потоке → один write-lock на каждый
index()). Профилировкаjdk.ThreadPark: на этом замке простаивало ~1860 потоко-секунд (≈3.4 ядра весь прогон) — после фикса 0.9 с.Тесты
WorkspaceSymbolIndexTest23/0/0,SymbolProviderStreamingTest7/0/0,SymbolProviderScriptVariantTest2/0/0,BSLWorkspaceServiceTest26/0/0,SymbolProviderTest— зелёные.trimToSize (второй коммит)
Мутабельные бакеты —
ArrayListс ёмкостью по умолчанию (10), большинство ключей редкие → слак. ПоServerContextPopulatedEvent(послеpopulateContext) бакеты ужимаютсяtrimToSize(аналогSymbolTreeComputer). Замер: heap 3.02 → 2.94 ГБ (−80 МБ) — бакеты снова точного размера, как прежниеList.copyOf; остаток vs baseline (+0.12 ГБ) — run-to-run разброс ленивой материализации модели членов, не индекс. Скорость/аллокации не регрессируют.Открытые вопросы (draft)
index()не пересобирает бакеты / не берёт write-lock) — добавлю перед снятием с draft.🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
New Features