sway
sway copied to clipboard
FuelLabs
Run LSP benchmarks in CI
Description
Runs benchmarks in CI with criterion-compare-prs so the comparison shows as a comment on the PR.
The extra config in Cargo.toml is because of this issue: https://github.com/bheisler/criterion.rs/issues/275. It needs to be merged to master first (https://github.com/FuelLabs/sway/pull/5547) for the benchmark comparison to work.
Checklist
- [ ] I have linked to any relevant issues.
- [ ] I have commented my code, particularly in hard-to-understand areas.
- [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book).
- [ ] I have added tests that prove my fix is effective or that my feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
Breaking*orNew Featurelabels where relevant. - [ ] I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
- [ ] I have requested a review from the relevant team or maintainers.
Benchmark for dbb86a5
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.3±0.02ms | 5.3±0.03ms | 0.00% |
| code_lens | 321.6±6.93ns | 324.3±5.91ns | +0.84% |
| compile | 2.9±0.05s | 2.9±0.04s | 0.00% |
| completion | 4.9±0.10ms | 4.8±0.02ms | -2.04% |
| did_change_with_caching | 2.9±0.03s | 2.9±0.02s | 0.00% |
| document_symbol | 991.5±17.84µs | 966.8±15.17µs | -2.49% |
| format | 74.4±0.69ms | 74.0±1.11ms | -0.54% |
| goto_definition | 360.1±6.65µs | 353.4±5.61µs | -1.86% |
| highlight | 9.0±0.02ms | 9.1±0.09ms | +1.11% |
| hover | 528.9±11.38µs | 534.5±25.07µs | +1.06% |
| idents_at_position | 124.3±0.39µs | 122.6±0.81µs | -1.37% |
| inlay_hints | 671.5±10.24µs | 663.8±26.21µs | -1.15% |
| on_enter | 489.5±9.93ns | 493.0±16.26ns | +0.72% |
| parent_decl_at_position | 3.7±0.03ms | 3.7±0.02ms | 0.00% |
| prepare_rename | 356.0±5.52µs | 358.5±10.11µs | +0.70% |
| rename | 9.4±0.19ms | 9.4±0.02ms | 0.00% |
| semantic_tokens | 1057.6±21.00µs | 1041.1±20.44µs | -1.56% |
| token_at_position | 353.5±2.17µs | 350.5±2.06µs | -0.85% |
| tokens_at_position | 3.7±0.02ms | 3.7±0.17ms | 0.00% |
| tokens_for_file | 412.8±0.92µs | 411.7±2.63µs | -0.27% |
| traverse | 39.0±1.02ms | 39.2±0.76ms | +0.51% |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Benchmark for ca37ade
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.3±0.10ms | 5.3±0.12ms | 0.00% |
| code_lens | 331.8±19.78ns | 324.3±10.21ns | -2.26% |
| compile | 3.0±0.06s | 2.9±0.02s | -3.33% |
| completion | 4.9±0.12ms | 4.9±0.10ms | 0.00% |
| did_change_with_caching | 2.8±0.03s | 3.0±0.04s | +7.14% |
| document_symbol | 968.3±19.65µs | 1038.4±14.61µs | +7.24% |
| format | 75.1±1.05ms | 74.7±1.24ms | -0.53% |
| goto_definition | 359.9±6.13µs | 355.3±9.53µs | -1.28% |
| highlight | 9.2±0.14ms | 9.1±0.08ms | -1.09% |
| hover | 538.5±6.19µs | 525.2±7.86µs | -2.47% |
| idents_at_position | 123.6±1.01µs | 123.2±0.86µs | -0.32% |
| inlay_hints | 666.2±23.44µs | 670.2±26.59µs | +0.60% |
| on_enter | 494.5±18.47ns | 501.4±10.59ns | +1.40% |
| parent_decl_at_position | 3.8±0.06ms | 3.7±0.03ms | -2.63% |
| prepare_rename | 358.1±6.05µs | 350.9±9.19µs | -2.01% |
| rename | 9.7±0.18ms | 9.5±0.18ms | -2.06% |
| semantic_tokens | 1063.7±15.94µs | 1055.9±15.16µs | -0.73% |
| token_at_position | 354.4±4.01µs | 348.7±2.53µs | -1.61% |
| tokens_at_position | 3.7±0.13ms | 3.7±0.05ms | 0.00% |
| tokens_for_file | 413.3±2.00µs | 409.9±5.71µs | -0.82% |
| traverse | 39.5±0.80ms | 39.2±1.27ms | -0.76% |
Nice this will be handy. Just wondering if we should filter this job to run only if code in sway-core of sway-lsp was changed so it doesn't become noisy.
benchmark-sway-lsp:
name: Compare sway-lsp benchmarks against master
runs-on: ubuntu-latest
if: >-
contains(toJSON(github.event.pull_request.changed_files), 'sway-lsp/') ||
contains(toJSON(github.event.pull_request.changed_files), 'sway-core/')
steps:
- uses: Swatinem/rust-cache@v2
- uses: actions/checkout@v3
- uses: boa-dev/[email protected]
with:
cwd: "./sway-lsp"
branchName: ${{ github.base_ref }}
token: ${{ secrets.GITHUB_TOKEN }}
Nice this will be handy. Just wondering if we should filter this job to run only if code in
sway-coreofsway-lspwas changed so it doesn't become noisy.benchmark-sway-lsp: name: Compare sway-lsp benchmarks against master runs-on: ubuntu-latest if: >- contains(toJSON(github.event.pull_request.changed_files), 'sway-lsp/') || contains(toJSON(github.event.pull_request.changed_files), 'sway-core/') steps: - uses: Swatinem/rust-cache@v2 - uses: actions/checkout@v3 - uses: boa-dev/[email protected] with: cwd: "./sway-lsp" branchName: ${{ github.base_ref }} token: ${{ secrets.GITHUB_TOKEN }}
Good call out. I realized that we also only want to run it on pull requests, not releases or pushes to master. I think it's cleanest as it's own workflow. What do you think?
I also added all of the dependencies from sway-lsp's Cargo.toml that are in this repo.
Benchmark for 2c186a1
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.3±0.13ms | 5.3±0.02ms | 0.00% |
| code_lens | 283.1±8.52ns | 284.6±8.87ns | +0.53% |
| compile | 2.8±0.02s | 2.8±0.03s | 0.00% |
| completion | 4.9±0.10ms | 4.9±0.11ms | 0.00% |
| did_change_with_caching | 2.8±0.03s | 2.9±0.02s | +3.57% |
| document_symbol | 1043.0±47.49µs | 967.8±11.62µs | -7.21% |
| format | 70.8±0.98ms | 70.4±1.26ms | -0.56% |
| goto_definition | 355.0±6.48µs | 363.5±8.24µs | +2.39% |
| highlight | 9.1±0.18ms | 9.2±0.13ms | +1.10% |
| hover | 525.7±6.56µs | 523.7±7.24µs | -0.38% |
| idents_at_position | 131.1±0.28µs | 131.2±1.25µs | +0.08% |
| inlay_hints | 665.0±15.65µs | 662.0±8.76µs | -0.45% |
| on_enter | 484.0±15.14ns | 479.6±13.39ns | -0.91% |
| parent_decl_at_position | 3.7±0.03ms | 3.8±0.05ms | +2.70% |
| prepare_rename | 356.5±9.23µs | 373.1±5.00µs | +4.66% |
| rename | 9.6±0.24ms | 9.4±0.17ms | -2.08% |
| semantic_tokens | 1070.3±51.74µs | 1029.6±16.97µs | -3.80% |
| token_at_position | 350.2±3.20µs | 352.4±2.79µs | +0.63% |
| tokens_at_position | 3.7±0.09ms | 3.8±0.02ms | +2.70% |
| tokens_for_file | 410.0±9.93µs | 505.5±3.78µs | +23.29% |
| traverse | 39.2±1.13ms | 38.8±1.23ms | -1.02% |
Benchmark for c1daf98
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.3±0.05ms | 5.3±0.12ms | 0.00% |
| code_lens | 286.4±8.76ns | 286.0±10.78ns | -0.14% |
| compile | 2.9±0.03s | 2.9±0.05s | 0.00% |
| completion | 4.9±0.03ms | 4.8±0.01ms | -2.04% |
| did_change_with_caching | 2.9±0.02s | 2.9±0.02s | 0.00% |
| document_symbol | 949.2±9.87µs | 988.6±49.44µs | +4.15% |
| format | 70.4±1.01ms | 70.8±2.84ms | +0.57% |
| goto_definition | 361.0±8.09µs | 352.7±5.17µs | -2.30% |
| highlight | 9.2±0.30ms | 9.1±0.13ms | -1.09% |
| hover | 533.1±9.81µs | 531.0±5.08µs | -0.39% |
| idents_at_position | 123.9±1.57µs | 122.6±0.43µs | -1.05% |
| inlay_hints | 658.8±10.06µs | 662.7±21.84µs | +0.59% |
| on_enter | 484.6±14.49ns | 489.3±14.17ns | +0.97% |
| parent_decl_at_position | 3.7±0.09ms | 3.7±0.04ms | 0.00% |
| prepare_rename | 356.2±4.03µs | 355.6±5.30µs | -0.17% |
| rename | 9.4±0.16ms | 9.4±0.19ms | 0.00% |
| semantic_tokens | 1059.9±17.29µs | 1032.1±17.81µs | -2.62% |
| token_at_position | 354.8±2.38µs | 347.7±2.21µs | -2.00% |
| tokens_at_position | 3.7±0.04ms | 3.7±0.04ms | 0.00% |
| tokens_for_file | 408.8±2.53µs | 404.4±4.22µs | -1.08% |
| traverse | 38.3±1.07ms | 39.6±1.25ms | +3.39% |
How does this chooses the commit to run?
Last comment has Benchmark for https://github.com/FuelLabs/sway/commit/c1daf98ba4e34562a59150a3c654bcb228307463, but there is not commit c1da.
Hovering the link it says [Merge 9de8b0e483a133a3f5cfbace9be20f1432f472b0 into f912881c71174655c…](https://github.com/FuelLabs/sway/commit/c1daf98ba4e34562a59150a3c654bcb228307463), but 9de8b is not the last commit.
How does this chooses the commit to run?
Good catch, I would also expect the commits to be in the PR branch. It looks like the changes in the commit are correct, and the benchmark comparison is correct, so I think it's still worth using this. I've opened an issue: https://github.com/boa-dev/criterion-compare-action/issues/115
Benchmark for 4bc8877
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.4±0.11ms | 5.4±0.09ms | 0.00% |
| code_lens | 326.9±7.60ns | 334.3±11.32ns | +2.26% |
| compile | 3.1±0.05s | 3.0±0.05s | -3.23% |
| completion | 4.9±0.09ms | 4.9±0.04ms | 0.00% |
| did_change_with_caching | 2.9±0.04s | 2.9±0.03s | 0.00% |
| document_symbol | 950.5±12.69µs | 961.0±18.31µs | +1.10% |
| format | 71.3±1.00ms | 71.3±1.21ms | 0.00% |
| goto_definition | 347.5±6.17µs | 351.8±10.51µs | +1.24% |
| highlight | 9.2±0.16ms | 9.2±0.12ms | 0.00% |
| hover | 534.7±11.02µs | 532.3±9.82µs | -0.45% |
| idents_at_position | 121.1±1.41µs | 122.0±1.06µs | +0.74% |
| inlay_hints | 659.1±15.02µs | 661.1±20.49µs | +0.30% |
| on_enter | 478.9±14.22ns | 490.8±22.29ns | +2.48% |
| parent_decl_at_position | 3.7±0.05ms | 3.7±0.03ms | 0.00% |
| prepare_rename | 359.3±18.14µs | 351.9±4.90µs | -2.06% |
| rename | 9.6±0.18ms | 9.5±0.13ms | -1.04% |
| semantic_tokens | 1066.2±25.45µs | 1047.7±17.00µs | -1.74% |
| token_at_position | 352.1±4.21µs | 355.8±4.05µs | +1.05% |
| tokens_at_position | 3.7±0.14ms | 3.8±0.07ms | +2.70% |
| tokens_for_file | 408.1±5.41µs | 450.4±2.59µs | +10.37% |
| traverse | 39.3±0.96ms | 39.5±0.78ms | +0.51% |
I agree with you that it seems to be working by the github action code. Have just approved this PR whilst we wait to see what the action author thinks of the modification.
Benchmark for bb5d0ad
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.1±0.13ms | 5.1±0.07ms | 0.00% |
| code_lens | 301.1±21.31ns | 291.1±12.71ns | -3.32% |
| compile | 3.0±0.02s | 2.9±0.03s | -3.33% |
| completion | 4.7±0.08ms | 4.7±0.06ms | 0.00% |
| did_change_with_caching | 2.8±0.01s | 2.8±0.03s | 0.00% |
| document_symbol | 945.6±7.80µs | 953.6±10.11µs | +0.85% |
| format | 89.7±1.04ms | 89.7±1.28ms | 0.00% |
| goto_definition | 346.7±6.18µs | 396.0±4.43µs | +14.22% |
| highlight | 8.7±0.01ms | 8.7±0.12ms | 0.00% |
| hover | 537.2±6.61µs | 561.3±9.59µs | +4.49% |
| idents_at_position | 121.0±0.57µs | 122.1±0.75µs | +0.91% |
| inlay_hints | 645.2±16.46µs | 652.3±22.75µs | +1.10% |
| on_enter | 490.0±29.41ns | 481.1±13.97ns | -1.82% |
| parent_decl_at_position | 3.6±0.04ms | 3.6±0.08ms | 0.00% |
| prepare_rename | 345.4±4.09µs | 356.5±5.92µs | +3.21% |
| rename | 9.1±0.20ms | 9.1±0.21ms | 0.00% |
| semantic_tokens | 1042.8±8.53µs | 1031.1±13.62µs | -1.12% |
| token_at_position | 341.2±2.57µs | 353.4±2.53µs | +3.58% |
| tokens_at_position | 3.6±0.03ms | 3.6±0.04ms | 0.00% |
| tokens_for_file | 402.2±2.64µs | 409.8±2.81µs | +1.89% |
| traverse | 38.3±1.44ms | 37.9±0.59ms | -1.04% |
Benchmark for 52d59c4
Click to view benchmark
| Test | Base | PR | % |
|---|---|---|---|
| code_action | 5.3±0.12ms | 5.3±0.17ms | 0.00% |
| code_lens | 286.5±8.02ns | 286.0±7.15ns | -0.17% |
| compile | 3.1±0.03s | 3.1±0.03s | 0.00% |
| completion | 5.0±0.20ms | 5.0±0.19ms | 0.00% |
| did_change_with_caching | 3.0±0.03s | 3.0±0.01s | 0.00% |
| document_symbol | 1025.6±30.01µs | 1016.9±36.68µs | -0.85% |
| format | 90.9±1.28ms | 90.6±2.75ms | -0.33% |
| goto_definition | 366.2±9.10µs | 351.2±6.33µs | -4.10% |
| highlight | 8.9±0.12ms | 8.8±0.18ms | -1.12% |
| hover | 542.5±7.62µs | 562.8±10.12µs | +3.74% |
| idents_at_position | 122.1±0.48µs | 122.0±1.85µs | -0.08% |
| inlay_hints | 661.9±19.61µs | 657.7±16.50µs | -0.63% |
| on_enter | 493.8±14.59ns | 493.8±20.48ns | 0.00% |
| parent_decl_at_position | 3.6±0.05ms | 3.6±0.04ms | 0.00% |
| prepare_rename | 354.5±6.54µs | 358.2±5.07µs | +1.04% |
| rename | 9.6±0.25ms | 9.5±0.38ms | -1.04% |
| semantic_tokens | 1058.2±11.40µs | 1065.0±27.35µs | +0.64% |
| token_at_position | 356.7±2.83µs | 354.3±3.16µs | -0.67% |
| tokens_at_position | 3.6±0.06ms | 3.6±0.04ms | 0.00% |
| tokens_for_file | 416.6±4.42µs | 414.6±4.54µs | -0.48% |
| traverse | 39.3±1.02ms | 39.2±0.84ms | -0.25% |