perf(references): массивы вместо concurrent-множеств в LocationRepository (−305 МиБ, чтение 42×, индексация 37×)#4271
Merged
Conversation
…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
Перевод записи расположений на replaceOccurrences сделал мёртвыми (остались
только в тестах / без вызовов):
- LocationRepository.updateLocation, LocationRepository.deleteAll;
- ReferenceIndex.saveOccurrence + addMethodCall/addModuleReference/
addVariableUsage.
Билдеры *Occurrence оставлены (их зовёт ReferenceIndexFiller). Тест
crossWorkspaceIsolation переведён на прод-API replaceReferences.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
Contributor
📝 WalkthroughWalkthroughReference replacement now builds deterministic occurrence sets, deletes stale symbol records, and atomically replaces per-URI location snapshots. LocationRepository uses immutable arrays and removes incremental update APIs. Tests now inject occurrences through ChangesReference replacement and location snapshots
Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReferenceIndex
participant SymbolOccurrenceRepository
participant LocationRepository
ReferenceIndex->>ReferenceIndex: Build replacement occurrences
ReferenceIndex->>SymbolOccurrenceRepository: Delete stale occurrences
ReferenceIndex->>SymbolOccurrenceRepository: Save new occurrences
ReferenceIndex->>LocationRepository: Replace URI snapshot
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/LocationRepository.java (1)
147-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSort occurrences eagerly to eliminate the secondary index and halve memory usage.
Instead of maintaining a secondary
sortedOccurrencesmap and lazily sorting it on read, you can sort the array eagerly inreplaceOccurrencesbefore inserting it intolocations. This completely eliminates the need for thesortedOccurrencesmap, halving the memory usage for arrays (avoiding duplicate arrays per URI) and removing the lazy-evaluation complexity.Since
occurrences.toArray()allocates a new array that is not yet visible to other threads, sorting it in-place beforelocations.put()is completely thread-safe.♻️ Proposed refactor for the entire file
Update
replaceOccurrencesto sort the array before insertion:public void replaceOccurrences(URI uri, Collection<SymbolOccurrence> occurrences) { if (occurrences.isEmpty()) { locations.remove(uri); } else { - locations.put(uri, occurrences.toArray(EMPTY_OCCURRENCES)); + var array = occurrences.toArray(EMPTY_OCCURRENCES); + Arrays.sort(array, BY_RANGE_START); + locations.put(uri, array); } - sortedOccurrences.remove(uri); }Simplify
findByPositionand remove thesortedSnapshotmethod:- public Optional<SymbolOccurrence> findByPosition(URI uri, Position position) { - var sorted = sortedOccurrences.computeIfAbsent(uri, this::sortedSnapshot); - var index = lastStartingAtOrBefore(sorted, position); - if (index >= 0 && matches(sorted[index], position)) { - return Optional.of(sorted[index]); - } - return Optional.empty(); - } - - private SymbolOccurrence[] sortedSnapshot(URI uri) { - var snapshot = locations.getOrDefault(uri, EMPTY_OCCURRENCES).clone(); - Arrays.sort(snapshot, BY_RANGE_START); - return snapshot; - } + public Optional<SymbolOccurrence> findByPosition(URI uri, Position position) { + var sorted = locations.getOrDefault(uri, EMPTY_OCCURRENCES); + var index = lastStartingAtOrBefore(sorted, position); + if (index >= 0 && matches(sorted[index], position)) { + return Optional.of(sorted[index]); + } + return Optional.empty(); + }Remove the
sortedOccurrencesfield entirely:- /** - * Вторичный индекс для {`@link` `#findByPosition`}: отсортированный по началу - ... - */ - private final Map<URI, SymbolOccurrence[]> sortedOccurrences = new ConcurrentHashMap<>();Also, remove the
sortedOccurrences.remove(uri)call from thedelete(URI uri)method (Line 169).🤖 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 - 160, Update LocationRepository.replaceOccurrences to convert occurrences to an array, sort that array before publishing it through locations, and remove the sortedOccurrences invalidation. Remove the sortedOccurrences field and sortedSnapshot helper, simplify findByPosition to use the eagerly sorted locations array, and remove sortedOccurrences cleanup from delete(URI uri).src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java (1)
183-199: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSkip redundant database operations for duplicate occurrences.
The Javadoc notes that duplicates are permitted in
newOccurrences. However, when a duplicate is encountered,!stale.remove(occurrence)will returntrueon the second pass (because it was already removed on the first), triggering an unnecessarysymbolOccurrenceRepository.save(occurrence)operation.You can use the boolean return value of
replacement.add(occurrence)to easily bypass the lookup and save operations for duplicates.♻️ Proposed refactor
- var replacement = new LinkedHashSet<SymbolOccurrence>(); - for (var occurrence : newOccurrences) { - replacement.add(occurrence); - // Успешное удаление из stale означает, что вхождение уже есть в индексе — - // символьную сторону не трогаем; остаток stale после цикла подлежит удалению. - if (!stale.remove(occurrence)) { - symbolOccurrenceRepository.save(occurrence); - } - } + var replacement = new LinkedHashSet<SymbolOccurrence>(); + for (var occurrence : newOccurrences) { + if (replacement.add(occurrence)) { + // Успешное удаление из stale означает, что вхождение уже есть в индексе — + // символьную сторону не трогаем; остаток stale после цикла подлежит удалению. + if (!stale.remove(occurrence)) { + symbolOccurrenceRepository.save(occurrence); + } + } + }🤖 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/ReferenceIndex.java` around lines 183 - 199, Update the occurrence loop so the boolean result of replacement.add(occurrence) identifies duplicates and skips stale removal and symbolOccurrenceRepository.save for occurrences already present in replacement. Retain the existing stale cleanup and save behavior for the first occurrence of each value, then continue replacing the location occurrences with the complete replacement set.
🤖 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/references/model/LocationRepository.java`:
- Around line 147-160: Update LocationRepository.replaceOccurrences to convert
occurrences to an array, sort that array before publishing it through locations,
and remove the sortedOccurrences invalidation. Remove the sortedOccurrences
field and sortedSnapshot helper, simplify findByPosition to use the eagerly
sorted locations array, and remove sortedOccurrences cleanup from delete(URI
uri).
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndex.java`:
- Around line 183-199: Update the occurrence loop so the boolean result of
replacement.add(occurrence) identifies duplicates and skips stale removal and
symbolOccurrenceRepository.save for occurrences already present in replacement.
Retain the existing stale cleanup and save behavior for the first occurrence of
each value, then continue replacing the location occurrences with the complete
replacement set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cce0b5fb-d967-41fd-9255-0556d5f9c36e
📒 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/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexTest.java
|
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.
Что и зачем
LocationRepositoryхранил вхождения документа какMap<URI, ConcurrentHashMap.newKeySet()>— concurrent-множество на каждый URI. На больших проектах это ~1ConcurrentHashMap$Nodeна каждое вхождение: на cpm (~13k модулей, 5.98M вхождений) — миллионы узлов и сотни МБ чистого оверхеда контейнеров.Что сделано
Map<URI, SymbolOccurrence[]>вместо множеств. Вхождения одного документа пишутся одним потоком (перестроение документа сериализовано), аReferenceIndex.replaceReferencesзаменяет весь набор расположений документа одним атомарным swap-ом ссылки на массив — конкурентные читатели всегда видят согласованный снимок без «пустого окна». Символьная сторона индекса остаётся инкрементальной.findByPosition(бинарный поиск по sorted-снимку) не тронут.replaceOccurrencesсделалupdateLocation/deleteAllиReferenceIndex.saveOccurrence/addXxxмёртвыми (звались только из тестов) — удалены; тест переведён на прод-API.Замеры на cpm
Память (live-heap дампы, full-vs-full, счётчики данных совпадают):
ConcurrentHashMap$Node[LConcurrentHashMap$Node;SymbolOccurrence[](новое)Итого ≈ −305 МиБ.
Время (JMH, cpm-распределение occ/URI, avgt ns/op, меньше=лучше):
getSymbolOccurrencesByLocationUri(чтение, горячий)findByPosition(чтение, очень горячий)replaceOccurrencesvs циклnewKeySet.add)Чистая победа: и память, и время на горячих путях (чтение + индексация), без регрессий.
Тестирование
ReferenceIndexTest+LocationRepositoryTest— 33/33 зелёные, полныйcompileJava/compileTestJavaпрошёл.🤖 Generated with Claude Code
https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
Summary by CodeRabbit
Bug Fixes
Refactor