perf(references): компактное хранение reference-индекса (−~870 МиБ на cpm)#4269
Closed
nixel2007 wants to merge 2 commits into
Closed
perf(references): компактное хранение reference-индекса (−~870 МиБ на cpm)#4269nixel2007 wants to merge 2 commits into
nixel2007 wants to merge 2 commits into
Conversation
…ository Заменяет `Map<Symbol, ConcurrentSkipListSet<SymbolOccurrence>>` на `Map<Symbol, Object>`, где значение — либо сам `SymbolOccurrence` (символ с единственным обращением, доминирующий случай, без обёртки-коллекции), либо отсортированный `SymbolOccurrence[]` для нескольких обращений. Все изменения атомарны по ключу через `compute`/`computeIfPresent`; массив неизменяем после публикации, поэтому читатели `getAllBySymbol` видят согласованный снимок и сохранённый порядок сортировки. `ConcurrentSkipListSet` — самая расточительная по памяти коллекция JDK: на символ приходились заголовок `ConcurrentSkipListMap` (88Б), sentinel-Node и вероятностные index-узлы. На большом проекте (cpm, ~13k модулей, 5.98M обращений, 1.45M символов) это давало ~627 МиБ накладных расходов. Замер на дампах live-heap (full-vs-full, счётчики SymbolOccurrence/Location/ Symbol совпадают): - контейнеры индекса: 627 МиБ -> 62 МиБ (-565 МиБ) - 40% символов (1 обращение) хранятся без контейнера вообще - 60% символов -> SymbolOccurrence[] (в среднем ~6 элементов) ReferenceIndexTest: 14/14 зелёные. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
…tory Заменяет `Map<URI, ConcurrentHashMap.newKeySet()>` на `Map<URI, SymbolOccurrence[]>`. Вхождения одного документа пишутся одним потоком (перестроение документа сериализовано), а `ReferenceIndex.replaceReferences` теперь заменяет весь набор расположений документа одним атомарным swap-ом ссылки на массив — конкурентные читатели видят согласованный снимок без «пустого окна», плотнее и без ~1 `ConcurrentHashMap$Node` на каждое вхождение. Символьная сторона индекса остаётся инкрементальной (добавить новые, удалить устаревшие). Гранулярный `updateLocation`/`deleteAll` (прямой API `addXxx`, используется тестами) сохранён как copy-on-write. Вторичный индекс `findByPosition` не изменился — читает тот же массив. На cpm (~13k модулей, 5.98M вхождений) `LocationRepository` держал ~5.98M `ConcurrentHashMap$Node` (~250 МБ + таблицы), теперь — массивы (~48 МБ). ReferenceIndexTest + LocationRepositoryTest: зелёные. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
Contributor
📝 WalkthroughWalkthroughChangesReference storage reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReferenceIndex
participant SymbolOccurrenceRepository
participant LocationRepository
ReferenceIndex->>ReferenceIndex: build ordered replacement set
ReferenceIndex->>SymbolOccurrenceRepository: save new occurrences
ReferenceIndex->>SymbolOccurrenceRepository: delete stale occurrences
ReferenceIndex->>LocationRepository: atomically replace document locations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/references/model/LocationRepository.java`:
- Around line 147-161: Update the Javadoc for replaceOccurrences to describe the
method contract rather than the document-rebuild caller scenario: state that it
atomically replaces the stored occurrences for the given URI, removes the URI
when the collection is empty, and invalidates the corresponding sorted
occurrences cache. Retain the parameter descriptions and occurrence uniqueness
invariant.
- Around line 163-180: Update LocationRepository.updateLocation and the
saveOccurrence path it serves to avoid linear duplicate checks and array copying
for every occurrence under the same URI. Use a set-backed or bulk-accumulation
structure for per-URI SymbolOccurrence values, while preserving equals-based
deduplication and the existing sortedOccurrences invalidation behavior.
🪄 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: 30e24fee-5b15-46af-a17a-a82732f07b43
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java
Comment on lines
147
to
+161
| /** | ||
| * Обновить данные о расположении обращения к символу. | ||
| * Заменить полный набор вхождений документа. Основной путь записи: перестроение | ||
| * документа собирает набор в буфер и заменяет его целиком одним атомарным swap-ом. | ||
| * | ||
| * @param uri URI документа. | ||
| * @param occurrences Новый набор вхождений документа (без дубликатов). | ||
| */ | ||
| public void replaceOccurrences(URI uri, Collection<SymbolOccurrence> occurrences) { | ||
| if (occurrences.isEmpty()) { | ||
| locations.remove(uri); | ||
| } else { | ||
| locations.put(uri, occurrences.toArray(EMPTY_OCCURRENCES)); | ||
| } | ||
| sortedOccurrences.remove(uri); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Javadoc describes calling scenario instead of the method's contract.
"Основной путь записи: перестроение документа собирает набор в буфер и заменяет его целиком..." describes how a caller uses this method (document rebuild), not replaceOccurrences' own contract (inputs/invariants/side effects). As per coding guidelines, 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 rewording
/**
- * Заменить полный набор вхождений документа. Основной путь записи: перестроение
- * документа собирает набор в буфер и заменяет его целиком одним атомарным swap-ом.
+ * Заменить полный набор вхождений документа одним атомарным swap-ом ссылки.
+ * Инвалидирует кэш {`@link` `#sortedOccurrences`} для URI.
*
* `@param` uri URI документа.
* `@param` occurrences Новый набор вхождений документа (без дубликатов).
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Обновить данные о расположении обращения к символу. | |
| * Заменить полный набор вхождений документа. Основной путь записи: перестроение | |
| * документа собирает набор в буфер и заменяет его целиком одним атомарным swap-ом. | |
| * | |
| * @param uri URI документа. | |
| * @param occurrences Новый набор вхождений документа (без дубликатов). | |
| */ | |
| public void replaceOccurrences(URI uri, Collection<SymbolOccurrence> occurrences) { | |
| if (occurrences.isEmpty()) { | |
| locations.remove(uri); | |
| } else { | |
| locations.put(uri, occurrences.toArray(EMPTY_OCCURRENCES)); | |
| } | |
| sortedOccurrences.remove(uri); | |
| } | |
| /** | |
| * Заменить полный набор вхождений документа одним атомарным swap-ом ссылки. | |
| * Инвалидирует кэш {`@link` `#sortedOccurrences`} для URI. | |
| * | |
| * `@param` uri URI документа. | |
| * `@param` occurrences Новый набор вхождений документа (без дубликатов). | |
| */ | |
| public void replaceOccurrences(URI uri, Collection<SymbolOccurrence> occurrences) { | |
| if (occurrences.isEmpty()) { | |
| locations.remove(uri); | |
| } else { | |
| locations.put(uri, occurrences.toArray(EMPTY_OCCURRENCES)); | |
| } | |
| sortedOccurrences.remove(uri); | |
| } |
🤖 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/references/model/LocationRepository.java`
around lines 147 - 161, Update the Javadoc for replaceOccurrences to describe
the method contract rather than the document-rebuild caller scenario: state that
it atomically replaces the stored occurrences for the given URI, removes the URI
when the collection is empty, and invalidates the corresponding sorted
occurrences cache. Retain the parameter descriptions and occurrence uniqueness
invariant.
Source: Coding guidelines
Comment on lines
+163
to
180
| /** | ||
| * Добавить одиночное вхождение к символу (гранулярный путь записи). Дубликат по | ||
| * {@code equals} игнорируется — семантика множества сохраняется. | ||
| * | ||
| * @param symbolOccurrence Обращение к символу. | ||
| */ | ||
| public void updateLocation(SymbolOccurrence symbolOccurrence) { | ||
| var location = symbolOccurrence.location(); | ||
| locations.computeIfAbsent(location.uri(), uri -> ConcurrentHashMap.newKeySet()) | ||
| .add(symbolOccurrence); | ||
| sortedOccurrences.remove(location.uri()); | ||
| var uri = symbolOccurrence.location().uri(); | ||
| locations.merge(uri, new SymbolOccurrence[]{symbolOccurrence}, (existing, added) -> { | ||
| if (contains(existing, added[0])) { | ||
| return existing; | ||
| } | ||
| var updated = Arrays.copyOf(existing, existing.length + 1); | ||
| updated[existing.length] = added[0]; | ||
| return updated; | ||
| }); | ||
| sortedOccurrences.remove(uri); | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how often addMethodCall/addModuleReference/addVariableUsage (and thus
# saveOccurrence -> updateLocation) are invoked per document.
rg -nP -C5 '\baddMethodCall\s*\(|\baddModuleReference\s*\(|\baddVariableUsage\s*\(' --type=java -g '!**/ReferenceIndex.java'Repository: 1c-syntax/bsl-language-server
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant files and call paths.
fd -a 'ReferenceIndex.java|LocationRepository.java' src/main/java
printf '\n--- ReferenceIndex outline ---\n'
ast-grep outline src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/ReferenceIndex.java --view expanded || true
printf '\n--- LocationRepository outline ---\n'
ast-grep outline src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java --view expanded || true
printf '\n--- ReferenceIndex relevant lines ---\n'
sed -n '1,260p' src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/ReferenceIndex.java | nl -ba | sed -n '1,260p'
printf '\n--- LocationRepository relevant lines ---\n'
sed -n '1,280p' src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java | nl -ba | sed -n '1,280p'Repository: 1c-syntax/bsl-language-server
Length of output: 1815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact call chain into LocationRepository.updateLocation.
rg -n "updateLocation\s*\(|saveOccurrence\s*\(|replaceOccurrences\s*\(|addMethodCall\s*\(|addModuleReference\s*\(|addVariableUsage\s*\(" src/main/java/com/github/_1c_syntax/bsl/languageserver/references -S
printf '\n--- ReferenceIndex.java ---\n'
sed -n '1,260p' src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/ReferenceIndex.java | nl -ba | sed -n '1,260p'
printf '\n--- LocationRepository.java (140-240) ---\n'
sed -n '140,240p' src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java | nl -ba | sed -n '140,240p'Repository: 1c-syntax/bsl-language-server
Length of output: 6439
updateLocation sits on a hot per-occurrence path and does linear work on every append. ReferenceIndex.addMethodCall / addModuleReference / addVariableUsage all funnel through saveOccurrence(), so repeated inserts for the same URI can still drift toward O(n²) during document indexing. If files can accumulate many references, a bulk accumulator or set-backed incremental structure would avoid the repeated scan/copy.
🤖 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/references/model/LocationRepository.java`
around lines 163 - 180, Update LocationRepository.updateLocation and the
saveOccurrence path it serves to avoid linear duplicate checks and array copying
for every occurrence under the same URI. Use a set-backed or bulk-accumulation
structure for per-URI SymbolOccurrence values, while preserving equals-based
deduplication and the existing sortedOccurrences invalidation behavior.
|
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Что и зачем
На больших проектах reference-индекс (
SymbolOccurrenceRepository+LocationRepository) — крупнейший потребитель памяти. На конфигурации cpm (~13k модулей, 5 979 539 вхождений, 1 449 862 символов) он держал большую часть хипа, причём почти всё это — не данные, а накладные расходы контейнеров.PR ничего не меняет в самих данных (
SymbolOccurrence,Location,Symbolи внешние мапы остаются как есть) — только заменяет расточительные per-элемент контейнеры на компактные массивы. Внешние контрактыReferenceIndexи обоих репозиториев сохранены.Методика замера
Live-heap дампы (
jmap -dump:live) снимались на cpm в фазе диагностик, когда индекс уже полностью построен (ServerContext.populateContextнаполняет его целиком до анализа). Сравнение честное, full-vs-full: во всех трёх дампах счётчики данных совпадают до штуки —SymbolOccurrence5 979 539,Location5 979 539,Symbol1 449 862. Значит дельты по контейнерам детерминированы. Размеры — shallow, МиБ.Коммит 1 —
SymbolOccurrenceRepository: скип-листы → single-or-arrayБыло:
Map<Symbol, ConcurrentSkipListSet<SymbolOccurrence>>— по одномуConcurrentSkipListSetна символ.ConcurrentSkipListSet— самая расточительная по памяти коллекция JDK: на символ приходились заголовокConcurrentSkipListMap(88 Б), sentinel-Node и вероятностные index-узлы. При 1.45M символов это давало сотни МБ на пустой инфраструктуре.Стало:
Map<Symbol, Object>, где значение — либо самSymbolOccurrence(символ с единственным обращением, доминирующий случай — вообще без обёртки-коллекции), либо отсортированныйSymbolOccurrence[]для нескольких обращений. Все изменения атомарны по ключу черезcompute/computeIfPresent; массив неизменяем после публикации, поэтому читателиgetAllBySymbolвидят согласованный снимок и прежний порядок сортировки.Распределение на cpm: 40% символов (584 456) имеют 1 обращение → хранятся без контейнера; 60% (865 406) →
SymbolOccurrence[]в среднем по ~6 элементов.ConcurrentSkipListMap$NodeConcurrentSkipListMap$IndexConcurrentSkipListMapConcurrentSkipListSetSymbolOccurrence[](новое)Экономия ≈ 565 МиБ.
ReferenceIndexTest— 14/14 зелёные (включая мутирующиеclearReferences,crossWorkspaceIsolation).Коммит 2 —
LocationRepository: concurrent-множества → массивыБыло:
Map<URI, ConcurrentHashMap.newKeySet()>— concurrent-множество на каждый URI. Каждое вхождение = отдельныйConcurrentHashMap$Node+ таблицы под каждый URI.Стало:
Map<URI, SymbolOccurrence[]>. Вхождения одного документа пишутся одним потоком (перестроение документа сериализовано), аReferenceIndex.replaceReferencesтеперь заменяет весь набор расположений документа одним атомарным swap-ом ссылки на массив — конкурентные читатели всегда видят согласованный снимок, без «пустого окна». Символьная сторона индекса осталась инкрементальной (добавить новые, удалить устаревшие). Вторичный индексfindByPositionне изменился — читает тот же массив. ГранулярныйupdateLocation/deleteAll(прямойaddXxx-API, используется тестами) сохранён как copy-on-write.ConcurrentHashMap$Node[LConcurrentHashMap$Node;(таблицы)SymbolOccurrence[]Уходит 6.07M
ConcurrentHashMap$Node(−255 МиБ) и −89 МиБ таблиц, взамен +39 МиБ массивов. Экономия ≈ 305 МиБ.ReferenceIndexTest+LocationRepositoryTest— зелёные.Суммарно на cpm
Детерминированная, атрибутируемая правкам экономия по контейнерам reference-индекса — ≈ 870 МиБ (565 + 305). Общий спад total-хипа больше (−1477 МиБ), но остаток — это разброс транзиентных объектов фазы диагностик (
Position,Range,Diagnostic,ATNConfig), т.к. дампы сняты в разные моменты; приписывать его правкам не корректно. Данные индекса (SymbolOccurrence/Location/Symbol) во всех дампах идентичны.Тестирование
Локально прогнаны
ReferenceIndexTestиLocationRepositoryTest— зелёные. Полный набор (в т.ч. диагностики, зависящие от reference-индекса) — на CI.🤖 Generated with Claude Code
https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
Summary by CodeRabbit