Обновление зависимостей + новый бенч#623
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a ChangesBenchmark Infrastructure Overhaul
Sequence DiagramsequenceDiagram
participant User
participant benchmark_compare_sh as benchmark-compare.sh
participant git as git archive
participant Gradle
participant JMH
participant analyze_py as benchmark-analyze-results.py
User->>benchmark_compare_sh: run with --quick/--label/--jvm-args
benchmark_compare_sh->>benchmark_compare_sh: load .env.benchmark, parse flags, set NEW_VERSION_NAME
benchmark_compare_sh->>git: extract branch files to temp dir
git-->>benchmark_compare_sh: archive with src/jmh overlay from workspace
benchmark_compare_sh->>Gradle: jmhJar in temp dir
Gradle-->>benchmark_compare_sh: *jmh*.jar → benchmark-results/
benchmark_compare_sh->>JMH: java $JVM_ARGS -prof MemoryProfiler -Dbench.edt.path=...
JMH-->>benchmark_compare_sh: old-results.json, $NEW_VERSION_NAME-results.json
benchmark_compare_sh->>analyze_py: --old old-results.json --new $NEW_VERSION_NAME-results.json --html
analyze_py-->>benchmark_compare_sh: comprehensive-analysis-with-values.png, report.html
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
docs/en/benchmark.md (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the
HEAD HEADexample intent.The quick-start example compares
HEADwith itself. If this is intentional (e.g., for a smoke test or baseline measurement), consider adding a brief explanation. If unintentional, replace the secondHEADwith a branch name likedevelop.🤖 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 `@docs/en/benchmark.md` at line 9, The benchmark-compare.sh example currently uses HEAD HEAD, which may be confusing because it compares the same revision. Update the example to either use a real comparison target in place of the second HEAD (for example a branch like develop) or add a short note near the command in benchmark.md explaining that HEAD HEAD is intentional for a smoke test/baseline. Keep the guidance close to the benchmark-compare.sh usage so readers understand the intent immediately.src/jmh/java/com/github/_1c_syntax/bsl/mdclasses/benchmark/MDClassesBenchmark.java (1)
49-54: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
Pathconversion to avoid per-iteration allocation.
Path.of(...)is currently invoked in every@Benchmarkmethod and@Setup. Since the path strings are fixed per-field, convert once in@Setupand store asPathfields:- private final String configPathEDT = System.getProperty( - "bench.edt.path", - "src/test/resources/ext/edt/ssl_3_1/configuration"); - private final String configPathDesigner = System.getProperty( - "bench.designer.path", - "src/test/resources/ext/designer/ssl_3_1/src/cf"); + private Path configPathEDT; + private Path configPathDesigner; + + `@Setup` + public void setup() { + configPathEDT = Path.of(System.getProperty( + "bench.edt.path", + "src/test/resources/ext/edt/ssl_3_1/configuration")); + configPathDesigner = Path.of(System.getProperty( + "bench.designer.path", + "src/test/resources/ext/designer/ssl_3_1/src/cf")); + // Предварительная загрузка для разогрева + MDClasses.createConfiguration(configPathEDT); + MDClasses.createConfiguration(configPathDesigner); + }Then remove
Path.of(...)wrappers from benchmark methods. This reduces allocation noise in the measured loop.🤖 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/jmh/java/com/github/_1c_syntax/bsl/mdclasses/benchmark/MDClassesBenchmark.java` around lines 49 - 54, The benchmark still converts the fixed config strings to Path inside each `@Benchmark` method and `@Setup`, causing repeated allocation in MDClassesBenchmark. Add cached Path fields initialized once in setup for configPathEDT and configPathDesigner, then update the benchmark methods to use those Path fields directly instead of wrapping them with Path.of(...). This keeps the conversion out of the measured loop and removes per-iteration allocation noise.
🤖 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 @.env.benchmark.example:
- Line 8: The benchmark env example is missing quotes around BENCH_JVM_ARGS, so
sourcing it will split the JVM flags into a bad assignment plus a command.
Update the BENCH_JVM_ARGS entry in the .env.benchmark.example template to wrap
the full value in quotes so benchmark-compare.sh can source it safely when
copied as-is.
In `@benchmark-analyze-results.py`:
- Around line 40-45: The comparison loop currently relies on zip(old_data,
new_data), which can pair unrelated benchmarks when result ordering differs or
one list has extra entries. Update the benchmark matching logic in
benchmark-analyze-results.py to key new_data by the full benchmark name from the
benchmark field, then look up each old_bench by that name before computing
change in the existing comparison flow. Keep the existing reporting path, but
use the matched benchmark pair instead of positional order in the affected
comparison code.
- Around line 17-18: The default input path for the --new argument in
benchmark-analyze-results.py is inconsistent with the filename produced by
benchmark-compare.sh, causing the analysis command to miss the generated results
after a default run. Update the parser.add_argument default for --new to match
the shell script’s output filename, and keep the help text aligned with the
actual generated JSON path so python3 benchmark-analyze-results.py works without
extra flags.
- Around line 21-22: The --html argument is currently always enabled because the
parser in benchmark-analyze-results.py uses action='store_true' with
default=True, so the HTML generation path never gets disabled. Update the
argument definition in the argparse setup so the flag is actually optional,
either by switching the default to False or by adding a separate --no-html
toggle, and make sure the branching logic that checks args.html later in the
script reflects the intended on/off behavior.
In `@benchmark-compare.sh`:
- Around line 64-70: `LABEL_NAME` is copied directly into `NEW_VERSION_NAME`,
which later feeds jar/result/commit path construction and can break file
handling or escape the results directory. Sanitize and normalize the value
before assigning it in the label-handling block, and make sure every place that
uses `NEW_VERSION_NAME` for path construction or `cat`-based reads/writes uses
the cleaned value consistently. Reference the `LABEL_NAME`/`NEW_VERSION_NAME`
flow in `benchmark-compare.sh` and verify the downstream path-building logic is
safe for spaces, slashes, and glob characters.
- Around line 149-150: The benchmark copy step in benchmark-compare.sh is
nesting the JMH sources instead of replacing them because git archive already
extracts src/ into the temp directory. Update the copy logic around the git
archive and cp flow so src/jmh from the current checkout replaces the branch’s
existing temp_dir/src/jmh contents rather than creating a nested jmh directory;
use the existing benchmark copy section to remove or overwrite the destination
before copying.
- Around line 239-251: The JVM and JMH argument assembly in benchmark-compare.sh
is using plain strings, so BENCH_EDT_PATH and BENCH_DESIGNER_PATH can be split
when they contain spaces. Refactor the argument handling around JVM_SYS_PROPS,
JVM_ARGS, JMH_COMMON_ARGS, and JMH_QUICK_ARGS into arrays and append each
-Dbench.*.path and benchmark flag as отдельные elements, then invoke java with
array expansion so the old-version.jar and NEW_VERSION_NAME.jar runs preserve
path integrity.
- Around line 27-33: The option parsing in benchmark-compare.sh for the --label
and --jvm-args cases does not validate that a following value exists before
reading $2 and shifting by 2. Update the argument-handling logic in the main
option switch to guard these branches, detect missing values for LABEL_NAME and
JVM_ARGS, and fail gracefully with a clear usage/error message instead of
attempting to consume an empty argument.
---
Nitpick comments:
In `@docs/en/benchmark.md`:
- Line 9: The benchmark-compare.sh example currently uses HEAD HEAD, which may
be confusing because it compares the same revision. Update the example to either
use a real comparison target in place of the second HEAD (for example a branch
like develop) or add a short note near the command in benchmark.md explaining
that HEAD HEAD is intentional for a smoke test/baseline. Keep the guidance close
to the benchmark-compare.sh usage so readers understand the intent immediately.
In
`@src/jmh/java/com/github/_1c_syntax/bsl/mdclasses/benchmark/MDClassesBenchmark.java`:
- Around line 49-54: The benchmark still converts the fixed config strings to
Path inside each `@Benchmark` method and `@Setup`, causing repeated allocation in
MDClassesBenchmark. Add cached Path fields initialized once in setup for
configPathEDT and configPathDesigner, then update the benchmark methods to use
those Path fields directly instead of wrapping them with Path.of(...). This
keeps the conversion out of the measured loop and removes per-iteration
allocation noise.
🪄 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: 50e04ce6-950c-48ee-9f68-50a3e641106f
⛔ Files ignored due to path filters (76)
src/test/resources/fixtures/external/ТестоваяВнешняяОбработка.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/external/ТестоваяВнешняяОбработка_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/external/ТестовыйВнешнийОтчет.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/external/ТестовыйВнешнийОтчет_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/AccountingRegisters.РегистрБухгалтерии1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/AccumulationRegisters.РегистрНакопления1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/BusinessProcesses.БизнесПроцесс1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/BusinessProcesses.БизнесПроцесс1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/CalculationRegisters.РегистрРасчета1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/CalculationRegisters.РегистрРасчета1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Catalogs.Справочник1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Catalogs.Справочник1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ChartsOfAccounts.ПланСчетов1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ChartsOfAccounts.ПланСчетов1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ChartsOfCalculationTypes.ПланВидовРасчета1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ChartsOfCharacteristicTypes.ПланВидовХарактеристик1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ChartsOfCharacteristicTypes.ПланВидовХарактеристик1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/CommonAttributes.ОбщийРеквизит1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Configuration.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Configuration_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Constants.Константа1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Constants.Константа1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/DataProcessors.Обработка1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/DataProcessors.Обработка1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/DefinedTypes.ОпределяемыйТип1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Documents.Документ1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Documents.Документ1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Enums.Перечисление1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Enums.Перечисление1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExchangePlans.ПланОбмена1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExchangePlans.ПланОбмена1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExternalDataSources.ТекущаяСУБД.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExternalDataSources.ТекущаяСУБД_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/InformationRegisters.РегистрСведений1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/InformationRegisters.РегистрСведений1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Reports.Отчет1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Reports.Отчет1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Tasks.Задача1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_18/Configuration.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_18/Configuration_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_24/Configuration_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_27/ExternalDataSources.ВнешнийИсточникДанных1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_27/ExternalDataSources.ВнешнийИсточникДанных1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_5_1/Configuration.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_ext/Configuration.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_ext/Configuration_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/BusinessProcesses.Задание.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/BusinessProcesses.Задание_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.Заметки.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.Заметки_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/CommonAttributes.ОбластьДанныхВспомогательныеДанные.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/CommonForms.Вопрос.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/CommonForms.Вопрос_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Constants.ЗаголовокСистемы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DataProcessors.ЗагрузкаКурсовВалют.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DataProcessors.ЗагрузкаКурсовВалют_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DocumentJournals.Взаимодействия.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DocumentJournals.Взаимодействия_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Documents.Анкета.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Documents.Анкета_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Enums.СтатусыОбработчиковОбновления.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Enums.СтатусыОбработчиковОбновления_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/InformationRegisters.СклоненияПредставленийОбъектов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/InformationRegisters.СклоненияПредставленийОбъектов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/InformationRegisters.ЭлектронныеПодписи.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/InformationRegisters.ЭлектронныеПодписи_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/SettingsStorages.ХранилищеВариантовОтчетов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/SettingsStorages.ХранилищеВариантовОтчетов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Tasks.ЗадачаИсполнителя.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Tasks.ЗадачаИсполнителя_edt.jsonis excluded by!src/test/resources/**
📒 Files selected for processing (10)
.env.benchmark.example.gitignorebenchmark-analyze-results.pybenchmark-compare.shbuild.gradle.ktsdocs/en/benchmark.mddocs/ru/benchmark.mdmkdocs.en.ymlmkdocs.ymlsrc/jmh/java/com/github/_1c_syntax/bsl/mdclasses/benchmark/MDClassesBenchmark.java
| BENCH_DESIGNER_PATH=src/test/resources/ext/designer/ssl_3_1/src/cf | ||
|
|
||
| # Дополнительные JVM-аргументы для JMH | ||
| BENCH_JVM_ARGS=-Xms4g -Xmx8g |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Quote JVM args so the env file can be sourced.
Because benchmark-compare.sh uses source .env.benchmark, Line 8 is parsed by Bash as BENCH_JVM_ARGS=-Xms4g followed by a command named -Xmx8g. This example will fail when copied as-is.
Proposed fix
-BENCH_JVM_ARGS=-Xms4g -Xmx8g
+BENCH_JVM_ARGS="-Xms4g -Xmx8g"📝 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
| BENCH_JVM_ARGS=-Xms4g -Xmx8g | |
| BENCH_JVM_ARGS="-Xms4g -Xmx8g" |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 8-8: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 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 @.env.benchmark.example at line 8, The benchmark env example is missing
quotes around BENCH_JVM_ARGS, so sourcing it will split the JVM flags into a bad
assignment plus a command. Update the BENCH_JVM_ARGS entry in the
.env.benchmark.example template to wrap the full value in quotes so
benchmark-compare.sh can source it safely when copied as-is.
Source: Linters/SAST tools
Comment on lines
+17
to
+18
| parser.add_argument('--new', default='benchmark-results/new-results.json', | ||
| help='Путь к JSON с результатами новой версии') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the default new-results path with benchmark-compare.sh.
The shell script writes benchmark-results/new-version-results.json by default, but this CLI reads benchmark-results/new-results.json, so python3 benchmark-analyze-results.py fails after a default benchmark run.
Proposed fix
- parser.add_argument('--new', default='benchmark-results/new-results.json',
+ parser.add_argument('--new', default='benchmark-results/new-version-results.json',📝 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
| parser.add_argument('--new', default='benchmark-results/new-results.json', | |
| help='Путь к JSON с результатами новой версии') | |
| parser.add_argument('--new', default='benchmark-results/new-version-results.json', | |
| help='Путь к JSON с результатами новой версии') |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 18-18: String contains ambiguous с (CYRILLIC SMALL LETTER ES). Did you mean c (LATIN SMALL LETTER C)?
(RUF001)
🤖 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 `@benchmark-analyze-results.py` around lines 17 - 18, The default input path
for the --new argument in benchmark-analyze-results.py is inconsistent with the
filename produced by benchmark-compare.sh, causing the analysis command to miss
the generated results after a default run. Update the parser.add_argument
default for --new to match the shell script’s output filename, and keep the help
text aligned with the actual generated JSON path so python3
benchmark-analyze-results.py works without extra flags.
Comment on lines
+21
to
+22
| parser.add_argument('--html', action='store_true', default=True, | ||
| help='Генерировать HTML-отчёт') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--html', action='store_true', default=True)
print(parser.parse_args([]).html)
PYRepository: 1c-syntax/mdclasses
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('benchmark-analyze-results.py')
text = p.read_text(encoding='utf-8')
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if 'add_argument' in line or 'args.html' in line:
print(f"{i}: {line}")
PY
echo '---'
grep -n "args.html\|--html\|--no-html" benchmark-analyze-results.py || trueRepository: 1c-syntax/mdclasses
Length of output: 586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('benchmark-analyze-results.py')
lines = p.read_text(encoding='utf-8').splitlines()
for start in (1, 1):
pass
for i, line in enumerate(lines, 1):
if 15 <= i <= 40:
print(f"{i}: {line}")
PYRepository: 1c-syntax/mdclasses
Length of output: 1239
Make --html actually optional. action='store_true' with default=True makes args.html always true, so the HTML branch at benchmark-analyze-results.py:548 never turns off. Use default=False or add a --no-html flag.
🤖 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 `@benchmark-analyze-results.py` around lines 21 - 22, The --html argument is
currently always enabled because the parser in benchmark-analyze-results.py uses
action='store_true' with default=True, so the HTML generation path never gets
disabled. Update the argument definition in the argparse setup so the flag is
actually optional, either by switching the default to False or by adding a
separate --no-html toggle, and make sure the branching logic that checks
args.html later in the script reflects the intended on/off behavior.
Comment on lines
40
to
+45
| for old_bench, new_bench in zip(old_data, new_data): | ||
| bench_name = old_bench['benchmark'].split('.')[-1] | ||
|
|
||
| # Производительность | ||
| old_time = old_bench['primaryMetric']['score'] | ||
| new_time = new_bench['primaryMetric']['score'] | ||
| change = ((old_time - new_time) / old_time) * 100 | ||
| change = ((old_time - new_time) / old_time) * 100 if old_time != 0 else 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match JMH results by benchmark name, not array order.
zip(old_data, new_data) silently compares unrelated benchmarks when result ordering differs or one side has an extra benchmark. Index the new results by full benchmark name before comparing.
Proposed fix
- for old_bench, new_bench in zip(old_data, new_data):
- bench_name = old_bench['benchmark'].split('.')[-1]
+ new_by_benchmark = {bench['benchmark']: bench for bench in new_data}
+
+ for old_bench in old_data:
+ full_bench_name = old_bench['benchmark']
+ new_bench = new_by_benchmark.get(full_bench_name)
+ if new_bench is None:
+ continue
+ bench_name = full_bench_name.split('.')[-1]Also applies to: 57-83
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 40-40: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 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 `@benchmark-analyze-results.py` around lines 40 - 45, The comparison loop
currently relies on zip(old_data, new_data), which can pair unrelated benchmarks
when result ordering differs or one list has extra entries. Update the benchmark
matching logic in benchmark-analyze-results.py to key new_data by the full
benchmark name from the benchmark field, then look up each old_bench by that
name before computing change in the existing comparison flow. Keep the existing
reporting path, but use the matched benchmark pair instead of positional order
in the affected comparison code.
Source: Linters/SAST tools
Comment on lines
+27
to
+33
| --label) | ||
| LABEL_NAME="$2" | ||
| shift 2 | ||
| ;; | ||
| --jvm-args) | ||
| JVM_ARGS="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate options that require a value.
--label or --jvm-args at the end of the command reads an empty $2 and then shift 2 fails. Add a guard before consuming the value.
Proposed fix
--label)
+ if [[ $# -lt 2 || "$2" == --* ]]; then
+ echo "❌ --label requires a value"
+ exit 1
+ fi
LABEL_NAME="$2"
shift 2
;;
--jvm-args)
+ if [[ $# -lt 2 || "$2" == --* ]]; then
+ echo "❌ --jvm-args requires a value"
+ exit 1
+ fi
JVM_ARGS="$2"
shift 2
;;📝 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
| --label) | |
| LABEL_NAME="$2" | |
| shift 2 | |
| ;; | |
| --jvm-args) | |
| JVM_ARGS="$2" | |
| shift 2 | |
| --label) | |
| if [[ $# -lt 2 || "$2" == --* ]]; then | |
| echo "❌ --label requires a value" | |
| exit 1 | |
| fi | |
| LABEL_NAME="$2" | |
| shift 2 | |
| ;; | |
| --jvm-args) | |
| if [[ $# -lt 2 || "$2" == --* ]]; then | |
| echo "❌ --jvm-args requires a value" | |
| exit 1 | |
| fi | |
| JVM_ARGS="$2" | |
| shift 2 | |
| ;; |
🤖 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 `@benchmark-compare.sh` around lines 27 - 33, The option parsing in
benchmark-compare.sh for the --label and --jvm-args cases does not validate that
a following value exists before reading $2 and shifting by 2. Update the
argument-handling logic in the main option switch to guard these branches,
detect missing values for LABEL_NAME and JVM_ARGS, and fail gracefully with a
clear usage/error message instead of attempting to consume an empty argument.
Comment on lines
+64
to
+70
| # Имя для маркировки результатов | ||
| if [ -n "$LABEL_NAME" ]; then | ||
| NEW_VERSION_NAME="$LABEL_NAME" | ||
| echo "🏷️ Маркировка результатов: $LABEL_NAME" | ||
| else | ||
| NEW_VERSION_NAME="new-version" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sanitize --label before using it in filenames.
NEW_VERSION_NAME is used to build jar/result/commit paths. Labels with spaces, /, or glob characters break unquoted cat calls and can write outside the results directory.
Proposed fix
if [ -n "$LABEL_NAME" ]; then
+ if [[ ! "$LABEL_NAME" =~ ^[A-Za-z0-9._-]+$ ]]; then
+ echo "❌ --label may contain only letters, numbers, dot, underscore, and dash"
+ exit 1
+ fi
NEW_VERSION_NAME="$LABEL_NAME"Also applies to: 198-202, 458-458
🤖 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 `@benchmark-compare.sh` around lines 64 - 70, `LABEL_NAME` is copied directly
into `NEW_VERSION_NAME`, which later feeds jar/result/commit path construction
and can break file handling or escape the results directory. Sanitize and
normalize the value before assigning it in the label-handling block, and make
sure every place that uses `NEW_VERSION_NAME` for path construction or
`cat`-based reads/writes uses the cleaned value consistently. Reference the
`LABEL_NAME`/`NEW_VERSION_NAME` flow in `benchmark-compare.sh` and verify the
downstream path-building logic is safe for spaces, slashes, and glob characters.
Source: Linters/SAST tools
Comment on lines
+149
to
+150
| git archive "$branch" build.gradle.kts settings.gradle.kts gradlew gradlew.bat lombok.config gradle.properties gradle/ src/ | tar -x -C "$temp_dir" | ||
| cp -r src/jmh "$temp_dir/src/jmh" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace src/jmh instead of nesting it.
Since the archive includes src/, $temp_dir/src/jmh may already exist. cp -r src/jmh "$temp_dir/src/jmh" then creates $temp_dir/src/jmh/jmh, so the build can still use the branch’s stale benchmark sources.
Proposed fix
git archive "$branch" build.gradle.kts settings.gradle.kts gradlew gradlew.bat lombok.config gradle.properties gradle/ src/ | tar -x -C "$temp_dir"
- cp -r src/jmh "$temp_dir/src/jmh"
+ rm -rf "$temp_dir/src/jmh"
+ mkdir -p "$temp_dir/src"
+ cp -r src/jmh "$temp_dir/src/jmh"📝 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
| git archive "$branch" build.gradle.kts settings.gradle.kts gradlew gradlew.bat lombok.config gradle.properties gradle/ src/ | tar -x -C "$temp_dir" | |
| cp -r src/jmh "$temp_dir/src/jmh" | |
| git archive "$branch" build.gradle.kts settings.gradle.kts gradlew gradlew.bat lombok.config gradle.properties gradle/ src/ | tar -x -C "$temp_dir" | |
| rm -rf "$temp_dir/src/jmh" | |
| mkdir -p "$temp_dir/src" | |
| cp -r src/jmh "$temp_dir/src/jmh" |
🤖 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 `@benchmark-compare.sh` around lines 149 - 150, The benchmark copy step in
benchmark-compare.sh is nesting the JMH sources instead of replacing them
because git archive already extracts src/ into the temp directory. Update the
copy logic around the git archive and cp flow so src/jmh from the current
checkout replaces the branch’s existing temp_dir/src/jmh contents rather than
creating a nested jmh directory; use the existing benchmark copy section to
remove or overwrite the destination before copying.
Comment on lines
+239
to
+251
| JVM_SYS_PROPS="" | ||
| if [ -n "$BENCH_EDT_PATH" ]; then | ||
| JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.edt.path=$BENCH_EDT_PATH" | ||
| fi | ||
| if [ -n "$BENCH_DESIGNER_PATH" ]; then | ||
| JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.designer.path=$BENCH_DESIGNER_PATH" | ||
| fi | ||
|
|
||
| echo "📊 Запуск бенчмарков для прошлой версии..." | ||
| java -jar "$RESULTS_DIR/old-version.jar" -prof com.github._1c_syntax.bsl.mdclasses.benchmark.MemoryProfiler -rf json -rff "$RESULTS_DIR/old-results.json" | ||
| java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/old-version.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/old-results.json" | ||
|
|
||
| echo "📊 Запуск бенчмарков для новой версии..." | ||
| java -jar "$RESULTS_DIR/new-version.jar" -prof com.github._1c_syntax.bsl.mdclasses.benchmark.MemoryProfiler -rf json -rff "$RESULTS_DIR/new-results.json" | ||
| java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/$NEW_VERSION_NAME.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/$NEW_VERSION_NAME-results.json" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use arrays for JVM/JMH args that include paths.
BENCH_EDT_PATH and BENCH_DESIGNER_PATH are expanded through string variables, so paths containing spaces are split before java receives the -Dbench.*.path values.
Proposed fix
-JVM_SYS_PROPS=""
+JVM_SYS_PROPS=()
if [ -n "$BENCH_EDT_PATH" ]; then
- JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.edt.path=$BENCH_EDT_PATH"
+ JVM_SYS_PROPS+=("-Dbench.edt.path=$BENCH_EDT_PATH")
fi
if [ -n "$BENCH_DESIGNER_PATH" ]; then
- JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.designer.path=$BENCH_DESIGNER_PATH"
+ JVM_SYS_PROPS+=("-Dbench.designer.path=$BENCH_DESIGNER_PATH")
fi
echo "📊 Запуск бенчмарков для прошлой версии..."
-java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/old-version.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/old-results.json"
+java $JVM_ARGS "${JVM_SYS_PROPS[@]}" -jar "$RESULTS_DIR/old-version.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/old-results.json"
echo "📊 Запуск бенчмарков для новой версии..."
-java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/$NEW_VERSION_NAME.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/$NEW_VERSION_NAME-results.json"
+java $JVM_ARGS "${JVM_SYS_PROPS[@]}" -jar "$RESULTS_DIR/$NEW_VERSION_NAME.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/$NEW_VERSION_NAME-results.json"📝 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
| JVM_SYS_PROPS="" | |
| if [ -n "$BENCH_EDT_PATH" ]; then | |
| JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.edt.path=$BENCH_EDT_PATH" | |
| fi | |
| if [ -n "$BENCH_DESIGNER_PATH" ]; then | |
| JVM_SYS_PROPS="$JVM_SYS_PROPS -Dbench.designer.path=$BENCH_DESIGNER_PATH" | |
| fi | |
| echo "📊 Запуск бенчмарков для прошлой версии..." | |
| java -jar "$RESULTS_DIR/old-version.jar" -prof com.github._1c_syntax.bsl.mdclasses.benchmark.MemoryProfiler -rf json -rff "$RESULTS_DIR/old-results.json" | |
| java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/old-version.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/old-results.json" | |
| echo "📊 Запуск бенчмарков для новой версии..." | |
| java -jar "$RESULTS_DIR/new-version.jar" -prof com.github._1c_syntax.bsl.mdclasses.benchmark.MemoryProfiler -rf json -rff "$RESULTS_DIR/new-results.json" | |
| java $JVM_ARGS $JVM_SYS_PROPS -jar "$RESULTS_DIR/$NEW_VERSION_NAME.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/$NEW_VERSION_NAME-results.json" | |
| JVM_SYS_PROPS=() | |
| if [ -n "$BENCH_EDT_PATH" ]; then | |
| JVM_SYS_PROPS+=("-Dbench.edt.path=$BENCH_EDT_PATH") | |
| fi | |
| if [ -n "$BENCH_DESIGNER_PATH" ]; then | |
| JVM_SYS_PROPS+=("-Dbench.designer.path=$BENCH_DESIGNER_PATH") | |
| fi | |
| echo "📊 Запуск бенчмарков для прошлой версии..." | |
| java $JVM_ARGS "${JVM_SYS_PROPS[@]}" -jar "$RESULTS_DIR/old-version.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/old-results.json" | |
| echo "📊 Запуск бенчмарков для новой версии..." | |
| java $JVM_ARGS "${JVM_SYS_PROPS[@]}" -jar "$RESULTS_DIR/$NEW_VERSION_NAME.jar" $JMH_COMMON_ARGS $JMH_QUICK_ARGS -rf json -rff "$RESULTS_DIR/$NEW_VERSION_NAME-results.json" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 248-248: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 248-248: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 248-248: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 248-248: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 251-251: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 251-251: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 251-251: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 251-251: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 `@benchmark-compare.sh` around lines 239 - 251, The JVM and JMH argument
assembly in benchmark-compare.sh is using plain strings, so BENCH_EDT_PATH and
BENCH_DESIGNER_PATH can be split when they contain spaces. Refactor the argument
handling around JVM_SYS_PROPS, JVM_ARGS, JMH_COMMON_ARGS, and JMH_QUICK_ARGS
into arrays and append each -Dbench.*.path and benchmark flag as отдельные
elements, then invoke java with array expansion so the old-version.jar and
NEW_VERSION_NAME.jar runs preserve path integrity.
Source: Linters/SAST tools
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.
Описание
Обновления зависимостей и бенчмарка
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
New Features
Documentation
Bug Fixes