perf(references): компактное хранение обращений + пакетная запись (−375 МиБ)#4272
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesSymbol occurrence storage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReferenceIndex
participant SymbolOccurrenceRepository
participant LocationRepository
ReferenceIndex->>ReferenceIndex: group non-stale occurrences by Symbol
ReferenceIndex->>SymbolOccurrenceRepository: saveAll(Symbol, occurrences)
ReferenceIndex->>SymbolOccurrenceRepository: deleteAll(stale occurrences)
ReferenceIndex->>LocationRepository: replaceOccurrences(uri, replacement)
Possibly related PRs
🚥 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: 1
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java (1)
119-180: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for the transition boundaries.
insert/removeimplement fairly intricate representation transitions (dedup bycompareTo, 2-element array creation, exactMAX_ARRAY_SIZEpromotion toConcurrentSkipListSet, 2-element collapse back to singleton, shrink-to-null). Logic traced through and appears correct at all these boundaries, but no dedicated test file forSymbolOccurrenceRepositoryis included in this change to lock in that behavior. As per coding guidelines, changes under**/*.javashould "maintain or improve test coverage using appropriate test frameworks (JUnit, AssertJ, Mockito)" — this correctness-critical, low-level logic is a good candidate for direct unit tests (e.g. singleton↔2-array, array atMAX_ARRAY_SIZE-1→MAX_ARRAY_SIZE,MAX_ARRAY_SIZE→skip-list promotion, duplicate insert/removal, remove-of-absent-element no-ops).Want me to draft a
SymbolOccurrenceRepositoryTestcovering these boundary cases?🤖 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/SymbolOccurrenceRepository.java` around lines 119 - 180, Add a dedicated JUnit/AssertJ test class for SymbolOccurrenceRepository covering insert/remove representation transitions: singleton to two-element array, array growth through MAX_ARRAY_SIZE to ConcurrentSkipListSet promotion, duplicate insertion, two-element collapse to singleton, shrink-to-null, and removal of absent occurrences as no-ops. Exercise the repository’s public API while asserting both contents and resulting behavior at each boundary.Source: Coding guidelines
🤖 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/SymbolOccurrenceRepository.java`:
- Around line 66-69: Update the class Javadoc around getAllBySymbol to
distinguish the immutable singleton/array copy-on-write paths from the
ConcurrentSkipListSet path, which exposes a live weakly consistent view during
concurrent save or deleteAll operations. Remove the claim that readers always
observe a consistent snapshot and document the accurate behavior for each
representation.
---
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java`:
- Around line 119-180: Add a dedicated JUnit/AssertJ test class for
SymbolOccurrenceRepository covering insert/remove representation transitions:
singleton to two-element array, array growth through MAX_ARRAY_SIZE to
ConcurrentSkipListSet promotion, duplicate insertion, two-element collapse to
singleton, shrink-to-null, and removal of absent occurrences as no-ops. Exercise
the repository’s public API while asserting both contents and resulting behavior
at each boundary.
🪄 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: 1dc383c4-6d1b-402a-952e-4b58fc859942
📒 Files selected for processing (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java
Contributor
nixel2007
force-pushed
the
perf/symbol-occurrence-compact
branch
from
July 14, 2026 20:21
a9a0d08 to
04e9820
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java (1)
91-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for concurrent saves across representation transitions.
The suite only exercises sequential writes, so regressions in the new per-key atomic-update guarantee could pass unnoticed. Add concurrent saves for one symbol and assert complete, unique, sorted results after crossing the array-to-
HubSetthreshold.As per coding guidelines, “maintain or improve test coverage using appropriate test frameworks.”
🤖 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/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java` around lines 91 - 102, Add a concurrent-save test alongside convertsToSkipListAboveThresholdAndKeepsAllOccurrences that writes many distinct occurrences for the same SYMBOL from multiple threads, crossing MAX_ARRAY_SIZE, then verifies getAllBySymbol returns the complete set exactly once in sorted order. Use the project’s existing concurrency/test utilities and ensure all save tasks finish before asserting the results.Source: Coding guidelines
🤖 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/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java`:
- Around line 91-102: Add a concurrent-save test alongside
convertsToSkipListAboveThresholdAndKeepsAllOccurrences that writes many distinct
occurrences for the same SYMBOL from multiple threads, crossing MAX_ARRAY_SIZE,
then verifies getAllBySymbol returns the complete set exactly once in sorted
order. Use the project’s existing concurrency/test utilities and ensure all save
tasks finish before asserting the results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0bd0d91-d53b-4cc4-92cf-a40552aa1673
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java
nixel2007
force-pushed
the
perf/symbol-occurrence-compact
branch
from
July 15, 2026 08:11
04e9820 to
5d60697
Compare
nixel2007
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
ArrayListinstead of using the fully qualified name.Multiple lines use the fully qualified
java.util.ArrayListclass name. To improve readability, consider addingimport java.util.ArrayList;at the top of the file and using the simple nameArrayListinstead.
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java#L158-L158: Replacenew java.util.ArrayList<SymbolOccurrence>()withnew ArrayList<>().src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java#L172-L172: Replacenew java.util.ArrayList<SymbolOccurrence>()withnew ArrayList<>().src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java#L187-L187: Replacenew java.util.ArrayList<SymbolOccurrence>()withnew ArrayList<>().🤖 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/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java` at line 158, Import ArrayList in SymbolOccurrenceRepositoryTest and replace the fully qualified java.util.ArrayList usages at lines 158, 172, and 187 with ArrayList using the diamond operator.
🤖 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/SymbolOccurrenceRepository.java`:
- Around line 106-115: Update deleteAll to group SymbolOccurrence values by
symbol before modifying occurrencesToSymbols, then remove each symbol’s entire
batch in one pass. Add or reuse a batched removal helper analogous to
mergeSorted that filters the existing collection against the grouped
occurrences, preserving single-value and empty-result handling while avoiding
repeated computeIfPresent/remove calls.
- Around line 42-60: Restore the >4096 occurrence fallback for hub symbols in
the single-insert path, especially around insertIntoArray, so repeated updates
stop performing O(n) array copy-on-write. Store oversized symbol occurrence
collections in the established ConcurrentSkipListSet representation while
retaining sorted array storage below the threshold. Update getAllBySymbol and
related mutation paths as needed to support both representations atomically.
---
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java`:
- Line 158: Import ArrayList in SymbolOccurrenceRepositoryTest and replace the
fully qualified java.util.ArrayList usages at lines 158, 172, and 187 with
ArrayList using the diamond operator.
🪄 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: c49749f3-e9c1-4b5b-97f6-b743b1f0bd05
📒 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/SymbolOccurrenceRepository.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java
nixel2007
force-pushed
the
perf/symbol-occurrence-compact
branch
from
July 15, 2026 08:40
5d60697 to
4f3a4eb
Compare
Contributor
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/references/ReferenceIndex.java`:
- Around line 187-195: Use the boolean result of replacement.add(occurrence) in
the loop over newOccurrences to skip duplicate occurrences before evaluating
stale.remove or adding to newBySymbol. Ensure each unique occurrence is
processed once while preserving the existing batched-save optimization.
🪄 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: 96b6d5f2-fc93-4de7-a164-9a6b7ab35a43
📒 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/SymbolOccurrenceRepository.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepositoryTest.java
- src/main/java/com/github/_1c_syntax/bsl/languageserver/references/model/SymbolOccurrenceRepository.java
Заменяет `Map<Symbol, ConcurrentSkipListSet<SymbolOccurrence>>` на `Map<Symbol, Object>`: сам `SymbolOccurrence` для символа с одним обращением (доминирующий случай), иначе отсортированный `SymbolOccurrence[]`. `ConcurrentSkipListSet` на каждый символ — самая расточительная по памяти коллекция JDK; на cpm (~13k модулей, 5.98M обращений, 1.45M символов) массивы вместо неё экономят ~375 МиБ реального heap (retained после GC, замер на cpm). Проблема массива — вставка копированием O(n), накопление по одному дало бы O(n²) на «хабах». Решается пакетной записью на документ: `ReferenceIndex.replaceReferences` группирует новые вхождения по символу и зовёт `saveAll` (галопирующий merge, одно копирование на документ вместо копирования на каждое обращение). `deleteAll` симметрично батчит удаление по символу. `BatchingSink` уже собирает пачку — бесплатно. Замер копирований на реальном cpm (инструментированный populateContext, настоящие последовательности пачек): одиночная запись 3.64e9 копий, пакетная 0.26e9 — в 14× меньше. При этом время построения индекса одинаковое (populateContext парсинг-bound); цель — память, по времени не хуже. `getAllBySymbol` возвращает `Collection` — представление без копирования. Все изменения атомарны по ключу через compute/computeIfPresent; массив неизменяем. SymbolOccurrenceRepositoryTest + ReferenceIndexTest + LocationRepositoryTest — 47/47 зелёные. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
nixel2007
force-pushed
the
perf/symbol-occurrence-compact
branch
from
July 15, 2026 10:09
4f3a4eb to
c11c62f
Compare
nixel2007
changed the title
|
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.
Проблема
SymbolOccurrenceRepositoryхранил обращения какMap<Symbol, ConcurrentSkipListSet<SymbolOccurrence>>— по одномуConcurrentSkipListSetна символ. Это самая расточительная по памяти коллекция JDK (заголовокConcurrentSkipListMap88Б + sentinel- и index-узлы на каждый элемент). На cpm (~13k модулей, 5.98M обращений, 1.45M символов) — ~627 МиБ накладных расходов контейнеров.Решение
Map<Symbol, Object>: самSymbolOccurrenceдля символа с одним обращением (доминирующий случай), иначе отсортированныйSymbolOccurrence[]. Никаких скип-листов и порогов.Проблема массива — вставка копированием O(n), а накопление обращений символа по одному давало бы O(n²) на «хабах». Решается пакетной записью на документ:
ReferenceIndex.replaceReferencesгруппирует новые вхождения по символу и зовётsaveAll, который сливает пачку с массивом за одно копирование (галопирующий mergeO(n+k)), а не копирует массив на каждое вхождение.BatchingSinkуже собирает пачку документа — так что это бесплатно.getAllBySymbolвозвращаетCollection— представление без копирования (чтение горячее: вывод типовExpressionTypeInferencer, диагностики).Память и время на cpm — прямой замер
populateContextпо cpm обеими сборками (единственное отличие — хранилище символьной стороны), retained heap после GC:populateContextConcurrentSkipListSet)Память: −375 МиБ реального heap. Время построения индекса — одинаковое: фаза упирается в парсинг 13k файлов (ANTLR), а запись reference-индекса — доли процента. Цель PR — память; по времени построения индекса не хуже.
(Разбор классов-контейнеров дампа даёт ~−546 МиБ, но это HPROF-shallow — ссылки по 8 Б; реальный heap с compressed oops — 4 Б, отсюда ~375.)
Почему массивы не медленнее скип-листа на записи
Массив вставляется копированием O(n), и накопление по одному дало бы O(n²) на «хабах». Держит время пакетная запись:
saveAllсливает все обращения символа из документа за одно копирование. Фактическая работа копирования на реальных пачках cpm (инструментированный прогон; средний размер пачки symbol-in-document = 3.56, 87% обращений — пачками ≥2):save(по одному)saveAll(этот PR)14× (а не ×3.56) потому, что выигрыш взвешен работой: хабы ссылаются из немногих документов (
F≪K), и батч за документ схлопываетO(K²)вO(K·F). Слияние — галопирующий merge (binarySearch позиции +arraycopyучастков), быстрый путь безstreamдля пачки из одного.deleteAllтоже батчит по символу — симметричноsaveAll.Тесты
Прямой
SymbolOccurrenceRepositoryTest(single / массив / крупная коллекция /saveAllслияние-дедуп / удаление со схлопыванием) +ReferenceIndexTest+LocationRepositoryTest— 47/47 зелёные. ПорядокgetReferencesToсохранён.replaceReferencesСтоит поверх уже влитого #4271:
replaceReferencesтеперь объединяет обе стороны — группировку по символу →saveAll(символьная сторона, этот PR) иreplaceOccurrences(сторона расположений, #4271). Конфликтов с develop нет.🤖 Generated with Claude Code
https://claude.ai/code/session_017AZeyPcsbdUBaLUGWbhrCi
Summary by CodeRabbit
New Features
Bug Fixes
Tests