Conversation
Поле «открытого» объекта может ссылаться на набор, внутри которого лежит он сам: значение поля пришло от того же значения, которому это поле принадлежит. Слияние такой пары разворачивало вложенность ещё на уровень, а копия делается на каждом уровне — следующее слияние обходилось дороже предыдущего. Пути до корней сборщика показывают ровно это: цепочка из 43 переходов «карта из одного типа → карта из девяти полей», удерживаемая переменной на стеке одного незавершённого слияния, — не индекс и не кэш. Глубже восьми уровней наборы сливаются поверхностно: столкнувшиеся ключи остаются за левым набором, вглубь слияние не идёт. На ssl_3_1 живой набор после сборки 2507 → 2133 МБ, пиковый RSS 4526 → 4181 МБ, время и состав замечаний прежние. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cu7S3zYn7n6GMsdYf5v1q
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds depth-aware recursive merging for ChangesType merge depth control
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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}
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/types/model/TypeSet.java`:
- Around line 147-158: Update the public JavaDoc for TypeSet.union in
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/model/TypeSet.java:147-158
to document the MAX_MERGE_DEPTH bound and that colliding eager decorations
retain the left-hand value at that limit. Update the LocalField type-merge
JavaDoc in
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/model/LocalField.java:57-75
to state that merging follows the bounded TypeSet.union contract; preserve
accurate parameter, result, invariant, and side-effect descriptions.
- Around line 175-182: Update the depth-limit branch in TypeSet’s merge logic to
use TypeSet::keepFirst for both lazyElements and lazyFields instead of
LazyTypeSet::combine and LazyField::merge, preserving the merge limit when lazy
values are evaluated. Add a test that evaluates lazy decorations at
MAX_MERGE_DEPTH and verifies the left-hand values are retained.
🪄 Autofix
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 Plus
Run ID: 50955cac-288a-4e04-a03e-54642070eb62
📒 Files selected for processing (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/model/LocalField.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/model/TypeDecorations.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/model/TypeSet.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/types/model/TypeSetMergeDepthTest.java
| if (depth >= MAX_MERGE_DEPTH) { | ||
| return new TypeSet( | ||
| merged, | ||
| TypeDecorations.mergedFlat(this.elementTypes, other.elementTypes, TypeSet::keepFirst), | ||
| TypeDecorations.mergedNested(this.localFields, other.localFields, TypeSet::keepFirst), | ||
| TypeDecorations.mergedFlat(this.lazyElements, other.lazyElements, LazyTypeSet::combine), | ||
| TypeDecorations.mergedNested(this.lazyFields, other.lazyFields, LazyField::merge), | ||
| TypeDecorations.mergedFlat(this.describedTypes, other.describedTypes, TypeSet::keepFirst) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -a '^(LazyTypeSet|LazyField)\.java$' src/main/java -x sh -c '
printf "\n=== %s ===\n" "$1"
ast-grep outline "$1" --items all
rg -n -C 8 "combine|merge|TypeSet\\.union|\\.union\\(" "$1"
' sh {}Repository: 1c-syntax/bsl-language-server
Length of output: 2119
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file="$(fd -a '^TypeSet\.java$' src/main/java | head -n 1)"
printf '=== %s ===\n' "$file"
cat -n "$file" | sed -n '1,240p'
printf '\n=== LazyField.java ===\n'
lazy_field="$(fd -a '^LazyField\.java$' src/main/java | head -n 1)"
cat -n "$lazy_field"
printf '\n=== LazyTypeSet.java ===\n'
lazy_type_set="$(fd -a '^LazyTypeSet\.java$' src/main/java | head -n 1)"
cat -n "$lazy_type_set"
printf '\n=== usages and tests ===\n'
rg -n -C 4 'MAX_MERGE_DEPTH|LazyTypeSet\.combine|LazyField::merge|lazyElements|lazyFields|union\(' --glob '*.java' src/test src/main 2>/dev/null | head -n 500Repository: 1c-syntax/bsl-language-server
Length of output: 50385
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("src/main/java/com/github/_1c_syntax/bsl/languageserver/types/model")
type_set = (root / "TypeSet.java").read_text()
lazy_type_set = (root / "LazyTypeSet.java").read_text()
lazy_field = (root / "LazyField.java").read_text()
checks = {
"depth branch combines lazy elements": "mergedFlat(this.lazyElements, other.lazyElements, LazyTypeSet::combine)" in type_set,
"depth branch combines lazy fields": "mergedNested(this.lazyFields, other.lazyFields, LazyField::merge)" in type_set,
"LazyTypeSet.combine defers a union": bool(re.search(r'return new LazyTypeSet\(.*?->\s*a\.get\(\)\.union\(b\.get\(\)\)', lazy_type_set, re.S)),
"LazyField.merge combines lazy types": "LazyTypeSet.combine(first.types, second.types)" in lazy_field,
"eager depth branch keeps first": "mergedFlat(this.elementTypes, other.elementTypes, TypeSet::keepFirst)" in type_set
and "mergedNested(this.localFields, other.localFields, TypeSet::keepFirst)" in type_set
and "mergedFlat(this.describedTypes, other.describedTypes, TypeSet::keepFirst)" in type_set,
"normal lazy branch also combines without depth": "mergedFlat(this.lazyElements, other.lazyElements, LazyTypeSet::combine)" in type_set
and "mergedNested(this.lazyFields, other.lazyFields, LazyField::merge)" in type_set,
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
assert all(checks.values())
print("\nConclusion: the depth-limit branch does not retain the left lazy value. "
"It creates a LazyTypeSet whose resolver calls public TypeSet.union(), "
"which starts at depth 0.")
PYRepository: 1c-syntax/bsl-language-server
Length of output: 590
Preserve the merge limit for lazy decorations.
At depth >= MAX_MERGE_DEPTH, LazyTypeSet::combine defers a.get().union(b.get()), which restarts at depth zero when evaluated. LazyField::merge also combines colliding values instead of retaining the left value. Use TypeSet::keepFirst for both lazy maps in this branch and add a test for evaluated lazy decorations at the limit.
🤖 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/model/TypeSet.java`
around lines 175 - 182, Update the depth-limit branch in TypeSet’s merge logic
to use TypeSet::keepFirst for both lazyElements and lazyFields instead of
LazyTypeSet::combine and LazyField::merge, preserving the merge limit when lazy
values are evaluated. Add a test that evaluates lazy decorations at
MAX_MERGE_DEPTH and verifies the left-hand values are retained.
|
Поправил замеры в описании: снятые числа по памяти оказались шумом. Поставил счётчик в точку отсечения — на обычном анализе ssl_3_1 он не срабатывает ни разу: 0 из 858 375 слияний с декорациями. Значит на этой нагрузке ветка выдаёт ровно те же значения, что и develop, а разница живого набора (2507/2095/2676 МБ против 2133/2401/1826 МБ по трём прогонам на сторону) — разброс, а не эффект. Ценность правки — только в патологическом случае: сборка, где каждая функция считается отдельной единицей расчёта, раньше падала по памяти на 8 ГБ, теперь проходит за 126 с при 5,2 ГБ. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Замечание принято: контракт слияния описан в публичном javadoc |
|
|
Закрываю: правка боролась со следствием. Причина найдена и исправлена в #4440 — С той правкой ограничитель глубины не срабатывает ни разу, а прогон вовсе без него проходит штатно (31348 замечаний, 104 с, 4,8 ГБ, без OOM) — то есть страховка здесь больше ничего не держит. |
1 participant
Проблема
Поле «открытого» объекта может ссылаться на набор, внутри которого лежит он сам: значение поля пришло от того же значения, которому это поле принадлежит. Слияние такой пары
TypeSet.unionне завязывало узел, а разворачивало вложенность ещё на уровень — и копия делается на каждом уровне, поэтому следующее слияние обходилось дороже предыдущего.Нашлось при разборе памяти на ssl_3_1. Ключевой замер — не аллокации, а пути до корней сборщика (
jcmd JFR.dump path-to-gc-roots=true, 245 доживших до сборки объектов при занятых 8,3 ГБ). У всех корень один:То есть память держал не индекс и не кэш, а незавершённый рекурсивный расчёт. Цепочка удержания для одного 80-байтового объекта — 43 перехода:
Чередование 1 и 9 по всем замерам (2002 карты размера 1, 1995 размера 9) — это
localFields: внешняя карта «тип → поля» с одним типом и внутренняя из девяти полей. Структура с девятью полями, у которой одно поле — снова она же, на два десятка уровней вглубь. Аллокационный профиль показывает ту же спиральTypeSet.union → LocalField.merge → TypeDecorations.mergedNested → TypeSet.union → ….Защита от самоссылок в системе типов есть —
LazyTypeSetзаведён ровно дляУзел: Массив из см. Узел, — но она стоит на объявленныхсм.-ссылках, а поля, накопленные изВставить(...), идут жадным путём без неё.Что сделано
Слияние наборов ограничено по глубине вложенности декораций (
TypeSet.MAX_MERGE_DEPTH = 8):unionполучил внутреннюю форму с глубиной; глубже предела наборы сливаются поверхностно — столкнувшиеся ключи остаются за левым набором, вглубь рекурсия не идёт;LocalField.mergeпротаскивает глубину и коротко замыкается, когда обе стороны — одно и то же значение;TypeDecorations.mergedFlat/mergedNestedперестали копировать карту, когда сливать не с чем (левая сторона пуста).Восьми уровней вложенности структур с запасом хватает на то, что встречается в коде; приём известный — так же ограничивают разворачивание типов TypeScript и Flow.
Замеры на ssl_3_1
Пакетный анализ, репортер SARIF,
mode: onlyсUnknownMember+EventHandlerInvalidSignature.На обычном анализе ограничитель не срабатывает ни разу: 0 отсечений на 858 375 слияний с декорациями (счётчик в точке отсечения, ветка
probe/typeset-depth). То есть здесь правка выдаёт ровно те же значения, что и develop, — ничего не теряется и ничего не выигрывается:Разброс внутри каждой стороны больше разницы между сторонами — значимого эффекта на обычной нагрузке нет. Состав замечаний гуляет в пределах собственной недетерминированности анализа (у develop между двумя прогонами расходится 273 строки подписи, у ветки 363) — это отдельная болезнь, #4429.
Смысл правки — в патологическом случае. На сборке, где каждая функция считается отдельной единицей расчёта (наработки по #4429), раньше анализ падал с
OutOfMemoryErrorна-Xmx8g(9,4 ГБ RSS, 8 минут), а теперь проходит за 126 с при 5,2 ГБ. Худший круг расчёта по компоненте: было +12941 МБ, стало +62 МБ. То есть это страховка от разрастания, а не ускорение.Тесты
TypeSetMergeDepthTest: до правки глубина вложенности после 100 слияний — 101, после — число слияний на глубину не влияет вовсе (20 слияний и 100 дают одинаковый результат). Второй тест держит то, что на посильной глубине поля по-прежнему объединяются.Связанные задачи
Найдено при работе над #4429; самостоятельной задачи не заводил.
Summary by CodeRabbit
Bug Fixes
Tests