Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use ModuleId instead of SourceId as the metrics map key #5871

Merged
merged 7 commits into from
Apr 18, 2024

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented Apr 18, 2024

Description

Fixes a critical bug where the server would crash if compilation was triggered from anything other than the root main.sw file. This was due to the metrics_map using the SourceId rather then the ModuleId as the key.

This was introduced last week in #5813 as we now need to metrics to decide if the engines should be mem swapped or discarded.

closes #5870

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* or New Feature labels 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.

@JoshuaBatty JoshuaBatty marked this pull request as draft April 18, 2024 02:27
@JoshuaBatty JoshuaBatty changed the title Josh/metrics bug Use ModuleId instead of SourceId as the metrics map key Apr 18, 2024
Copy link

Benchmark for d29079c

Click to view benchmark
Test Base PR %
code_action 5.5±0.06ms 5.4±0.23ms -1.82%
code_lens 331.5±9.13ns 286.2±4.09ns -13.67%
compile 6.1±0.03s 6.2±0.04s +1.64%
completion 5.1±0.07ms 4.8±0.02ms -5.88%
did_change_with_caching 6.3±0.11s 6.1±0.07s -3.17%
document_symbol 1009.6±16.48µs 972.9±23.23µs -3.64%
format 73.8±1.08ms 74.2±1.27ms +0.54%
goto_definition 364.4±6.68µs 366.1±7.20µs +0.47%
highlight 9.0±0.02ms 8.8±0.25ms -2.22%
hover 609.1±25.62µs 603.4±18.16µs -0.94%
idents_at_position 122.8±0.31µs 123.2±1.10µs +0.33%
inlay_hints 663.3±11.52µs 653.0±7.54µs -1.55%
on_enter 506.6±10.91ns 528.0±17.15ns +4.22%
parent_decl_at_position 3.7±0.03ms 3.6±0.03ms -2.70%
prepare_rename 375.3±6.25µs 371.2±7.12µs -1.09%
rename 9.6±0.10ms 10.0±0.31ms +4.17%
semantic_tokens 1035.8±19.22µs 1056.6±55.69µs +2.01%
token_at_position 361.5±1.54µs 353.7±2.62µs -2.16%
tokens_at_position 3.7±0.02ms 3.6±0.16ms -2.70%
tokens_for_file 422.9±1.94µs 419.9±1.34µs -0.71%
traverse 50.4±1.19ms 51.3±1.89ms +1.79%

@JoshuaBatty JoshuaBatty self-assigned this Apr 18, 2024
@JoshuaBatty JoshuaBatty added bug Something isn't working language server LSP server P: critical Should be looked at before anything else labels Apr 18, 2024
@JoshuaBatty JoshuaBatty requested review from a team April 18, 2024 03:33
@JoshuaBatty JoshuaBatty marked this pull request as ready for review April 18, 2024 03:33
Copy link

Benchmark for 375c6fa

Click to view benchmark
Test Base PR %
code_action 5.6±0.12ms 5.4±0.04ms -3.57%
code_lens 328.7±9.99ns 288.3±9.12ns -12.29%
compile 6.3±0.05s 6.3±0.03s 0.00%
completion 5.2±0.10ms 5.0±0.15ms -3.85%
did_change_with_caching 6.3±0.08s 6.3±0.03s 0.00%
document_symbol 991.5±54.14µs 946.9±18.59µs -4.50%
format 73.3±1.36ms 74.6±1.04ms +1.77%
goto_definition 359.0±4.13µs 365.9±6.43µs +1.92%
highlight 9.2±0.06ms 8.8±0.04ms -4.35%
hover 597.5±17.93µs 612.4±27.36µs +2.49%
idents_at_position 123.2±1.21µs 122.3±1.15µs -0.73%
inlay_hints 660.5±20.61µs 647.9±25.85µs -1.91%
on_enter 509.4±14.80ns 498.0±16.43ns -2.24%
parent_decl_at_position 3.8±0.02ms 3.6±0.06ms -5.26%
prepare_rename 358.3±6.76µs 373.5±6.62µs +4.24%
rename 9.9±0.14ms 9.4±0.08ms -5.05%
semantic_tokens 1027.2±14.53µs 1041.5±16.96µs +1.39%
token_at_position 357.7±3.20µs 366.6±2.75µs +2.49%
tokens_at_position 3.8±0.04ms 3.6±0.05ms -5.26%
tokens_for_file 419.1±3.64µs 423.8±2.53µs +1.12%
traverse 51.7±2.42ms 50.3±1.78ms -2.71%

@JoshuaBatty JoshuaBatty requested a review from a team April 18, 2024 04:00
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) April 18, 2024 04:01
@JoshuaBatty JoshuaBatty merged commit 3df4ee7 into master Apr 18, 2024
36 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/metrics_bug branch April 18, 2024 05:24
JoshuaBatty added a commit that referenced this pull request Apr 18, 2024
## Description

waiting on #5871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working language server LSP server P: critical Should be looked at before anything else
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LSP server panic: index out of bounds. SourceEngine SourceID's out of sync.
3 participants