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

Fix critical GC <> LSP bug and enable dynamic GC configuration. #5813

Merged
merged 27 commits into from
Apr 11, 2024

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented Apr 2, 2024

Description

The new garbage_collection_storage test doesn't pass yet. See #5814 to track progress on fixing this bug. This test now passes.

Ok this all works, feeling pretty confident that we've got to the bottom of this bug. The GC and LSP should be pretty robust now. Turned out to be 3 things that were causing the transient bugs.

  1. If the compiler was called from LSP, don't load the file from disk in the compiler to check if the cache is up to date. Instead, use the file version to check supplied by the LSP client.
  2. check if the compiler retuned a cached AST for the users workspace, if so, we traverse the AST's with the current LSP version of the engines as it hasn't had garbage collection applied.
  3. Also do the above check in the compilation thread. Only mem swap the engines_clone and engines if a cached AST for the users workspace was not used.

We are now also able to wrap the QE types in Arcs which is saving about ~23ms in cloning on each keystroke.

closes #5814
closes #5550

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 self-assigned this Apr 2, 2024
@JoshuaBatty JoshuaBatty added bug Something isn't working language server LSP server compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Apr 2, 2024
@JoshuaBatty JoshuaBatty marked this pull request as draft April 2, 2024 04:09
Copy link

github-actions bot commented Apr 2, 2024

Benchmark for 5c0bdbd

Click to view benchmark
Test Base PR %
code_action 5.5±0.16ms 5.4±0.10ms -1.82%
code_lens 293.4±13.50ns 292.1±11.41ns -0.44%
compile 5.8±0.08s 5.8±0.09s 0.00%
completion 4.9±0.13ms 4.9±0.03ms 0.00%
did_change_with_caching 5.3±0.04s 5.3±0.01s 0.00%
document_symbol 1023.7±28.20µs 1021.0±31.51µs -0.26%
format 87.2±2.14ms 87.5±1.37ms +0.34%
goto_definition 361.4±7.01µs 360.9±6.60µs -0.14%
highlight 9.1±0.17ms 9.1±0.03ms 0.00%
hover 625.5±17.07µs 601.3±20.95µs -3.87%
idents_at_position 123.3±0.30µs 123.3±2.02µs 0.00%
inlay_hints 665.4±15.80µs 664.9±24.81µs -0.08%
on_enter 509.3±18.45ns 486.7±43.12ns -4.44%
parent_decl_at_position 3.7±0.03ms 3.7±0.03ms 0.00%
prepare_rename 354.2±5.55µs 366.6±8.02µs +3.50%
rename 9.5±0.20ms 9.4±0.10ms -1.05%
semantic_tokens 1058.1±20.07µs 1062.8±25.50µs +0.44%
token_at_position 355.6±3.04µs 383.4±4.20µs +7.82%
tokens_at_position 3.7±0.04ms 3.8±0.03ms +2.70%
tokens_for_file 411.4±13.88µs 423.2±11.45µs +2.87%
traverse 43.3±2.49ms 43.8±1.83ms +1.15%

Copy link

github-actions bot commented Apr 3, 2024

Benchmark for 423c3bf

Click to view benchmark
Test Base PR %
code_action 5.5±0.03ms 5.5±0.04ms 0.00%
code_lens 294.5±6.92ns 294.1±9.34ns -0.14%
compile 6.1±0.15s 6.2±0.08s +1.64%
completion 5.0±0.04ms 5.1±0.06ms +2.00%
did_change_with_caching 5.6±0.05s 5.7±0.04s +1.79%
document_symbol 993.9±40.46µs 1030.8±21.11µs +3.71%
format 90.7±1.22ms 91.1±0.83ms +0.44%
goto_definition 360.6±8.95µs 368.7±10.34µs +2.25%
highlight 9.1±0.02ms 9.1±0.18ms 0.00%
hover 606.6±14.20µs 613.3±21.35µs +1.10%
idents_at_position 123.7±0.55µs 122.5±0.62µs -0.97%
inlay_hints 667.9±23.67µs 691.3±10.14µs +3.50%
on_enter 488.6±12.10ns 506.6±12.84ns +3.68%
parent_decl_at_position 3.7±0.03ms 3.8±0.02ms +2.70%
prepare_rename 361.0±6.22µs 364.8±8.26µs +1.05%
rename 9.7±0.17ms 9.9±0.22ms +2.06%
semantic_tokens 1078.5±35.17µs 1050.8±15.14µs -2.57%
token_at_position 462.7±3.67µs 360.4±9.29µs -22.11%
tokens_at_position 3.8±0.03ms 3.7±0.01ms -2.63%
tokens_for_file 418.7±4.53µs 424.0±7.66µs +1.27%
traverse 44.6±1.51ms 44.5±1.42ms -0.22%

Copy link

github-actions bot commented Apr 4, 2024

Benchmark for 1cf00d4

Click to view benchmark
Test Base PR %
code_action 5.4±0.05ms 5.4±0.01ms 0.00%
code_lens 288.7±7.58ns 295.7±9.73ns +2.42%
compile 5.8±0.05s 5.9±0.07s +1.72%
completion 4.9±0.06ms 5.0±0.02ms +2.04%
did_change_with_caching 5.4±0.04s 5.4±0.03s 0.00%
document_symbol 943.1±36.04µs 956.9±8.53µs +1.46%
format 89.5±1.16ms 89.1±1.43ms -0.45%
goto_definition 359.1±7.24µs 362.8±5.32µs +1.03%
highlight 9.1±0.02ms 9.1±0.04ms 0.00%
hover 595.6±18.93µs 601.1±22.55µs +0.92%
idents_at_position 126.2±2.20µs 122.1±0.72µs -3.25%
inlay_hints 665.5±6.55µs 680.0±43.14µs +2.18%
on_enter 482.6±15.18ns 493.4±13.17ns +2.24%
parent_decl_at_position 3.7±0.03ms 3.8±0.10ms +2.70%
prepare_rename 358.9±2.91µs 361.6±14.18µs +0.75%
rename 9.6±0.14ms 9.6±0.22ms 0.00%
semantic_tokens 1017.6±28.13µs 1066.1±13.70µs +4.77%
token_at_position 351.5±4.59µs 359.4±1.45µs +2.25%
tokens_at_position 3.7±0.03ms 3.7±0.05ms 0.00%
tokens_for_file 413.3±12.26µs 414.8±8.92µs +0.36%
traverse 42.6±1.52ms 42.1±1.07ms -1.17%

@JoshuaBatty JoshuaBatty requested review from a team April 5, 2024 03:26
@JoshuaBatty JoshuaBatty marked this pull request as ready for review April 5, 2024 03:27
Copy link

github-actions bot commented Apr 5, 2024

Benchmark for 71ce3ab

Click to view benchmark
Test Base PR %
code_action 5.5±0.09ms 5.3±0.03ms -3.64%
code_lens 291.7±14.17ns 313.0±13.79ns +7.30%
compile 6.3±0.08s 6.2±0.23s -1.59%
completion 5.1±0.16ms 4.8±0.21ms -5.88%
did_change_with_caching 5.7±0.09s 5.6±0.04s -1.75%
document_symbol 954.5±19.05µs 1077.2±40.01µs +12.85%
format 90.0±0.94ms 91.0±2.65ms +1.11%
goto_definition 372.6±41.68µs 370.2±5.31µs -0.64%
highlight 9.1±0.18ms 8.8±0.37ms -3.30%
hover 610.9±14.20µs 607.7±40.84µs -0.52%
idents_at_position 124.9±0.75µs 123.2±1.33µs -1.36%
inlay_hints 660.9±14.53µs 663.1±33.35µs +0.33%
on_enter 482.1±11.99ns 491.9±13.76ns +2.03%
parent_decl_at_position 3.7±0.03ms 3.6±0.05ms -2.70%
prepare_rename 364.6±4.23µs 369.4±12.12µs +1.32%
rename 9.7±0.15ms 9.2±0.09ms -5.15%
semantic_tokens 1053.3±15.67µs 1105.7±16.27µs +4.97%
token_at_position 356.8±2.72µs 364.3±3.78µs +2.10%
tokens_at_position 3.8±0.03ms 3.6±0.03ms -5.26%
tokens_for_file 408.2±3.56µs 432.5±3.25µs +5.95%
traverse 45.2±1.51ms 46.7±2.06ms +3.32%

@JoshuaBatty JoshuaBatty changed the title Add new garbage collection test for contracts and enable dynamic GC configuration. Fix critical GC <> LSP bug and enable dynamic GC configuration. Apr 5, 2024
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) April 5, 2024 03:43
sway-core/src/lib.rs Outdated Show resolved Hide resolved
@tritao
Copy link
Contributor

tritao commented Apr 5, 2024

Nice work, really glad to see this and that you have been able to the bottom of these issues 👍

@JoshuaBatty JoshuaBatty marked this pull request as draft April 8, 2024 02:48
Copy link

github-actions bot commented Apr 8, 2024

Benchmark for 79368d2

Click to view benchmark
Test Base PR %
code_action 5.4±0.11ms 5.4±0.10ms 0.00%
code_lens 334.3±9.21ns 300.2±10.68ns -10.20%
compile 5.8±0.06s 5.9±0.09s +1.72%
completion 5.1±0.15ms 5.0±0.02ms -1.96%
did_change_with_caching 5.3±0.03s 5.3±0.03s 0.00%
document_symbol 959.8±11.37µs 1047.2±10.70µs +9.11%
format 90.1±1.07ms 89.3±0.70ms -0.89%
goto_definition 368.7±5.80µs 368.3±8.36µs -0.11%
highlight 9.1±0.11ms 9.2±0.11ms +1.10%
hover 599.4±26.03µs 606.1±6.94µs +1.12%
idents_at_position 124.6±0.20µs 123.4±1.07µs -0.96%
inlay_hints 699.2±24.71µs 693.8±9.83µs -0.77%
on_enter 480.3±14.85ns 508.0±8.65ns +5.77%
parent_decl_at_position 3.7±0.04ms 3.7±0.02ms 0.00%
prepare_rename 363.6±5.29µs 367.8±3.41µs +1.16%
rename 9.5±0.19ms 9.6±0.15ms +1.05%
semantic_tokens 1061.5±10.90µs 1041.5±11.78µs -1.88%
token_at_position 366.8±2.61µs 366.8±1.29µs 0.00%
tokens_at_position 3.7±0.05ms 3.7±0.02ms 0.00%
tokens_for_file 416.9±3.05µs 409.1±4.12µs -1.87%
traverse 44.0±1.43ms 43.5±1.70ms -1.14%

@JoshuaBatty JoshuaBatty requested review from xunilrj, tritao and a team April 10, 2024 05:55
@JoshuaBatty JoshuaBatty marked this pull request as ready for review April 10, 2024 05:56
Copy link

Benchmark for 0270bb2

Click to view benchmark
Test Base PR %
code_action 5.5±0.06ms 5.6±0.17ms +1.82%
code_lens 288.5±10.71ns 283.0±4.83ns -1.91%
compile 6.6±0.13s 6.6±0.11s 0.00%
completion 5.0±0.10ms 5.0±0.10ms 0.00%
did_change_with_caching 6.4±0.04s 6.4±0.06s 0.00%
document_symbol 957.5±105.36µs 1049.7±15.50µs +9.63%
format 76.8±1.37ms 76.6±0.72ms -0.26%
goto_definition 375.0±6.99µs 369.2±4.25µs -1.55%
highlight 9.1±0.13ms 9.1±0.04ms 0.00%
hover 610.5±22.11µs 603.9±13.82µs -1.08%
idents_at_position 122.1±0.27µs 123.6±1.01µs +1.23%
inlay_hints 666.0±11.94µs 706.3±23.28µs +6.05%
on_enter 499.6±9.72ns 492.5±16.49ns -1.42%
parent_decl_at_position 3.7±0.09ms 3.7±0.04ms 0.00%
prepare_rename 374.0±6.31µs 368.2±5.60µs -1.55%
rename 9.7±0.08ms 9.6±0.16ms -1.03%
semantic_tokens 1039.0±10.39µs 1054.9±17.29µs +1.53%
token_at_position 364.8±2.46µs 360.1±2.91µs -1.29%
tokens_at_position 3.7±0.06ms 3.7±0.03ms 0.00%
tokens_for_file 414.6±2.48µs 418.2±8.95µs +0.87%
traverse 49.4±1.40ms 50.1±0.87ms +1.42%

sdankel
sdankel previously approved these changes Apr 11, 2024
Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@JoshuaBatty JoshuaBatty requested a review from a team April 11, 2024 00:23
Copy link

Benchmark for 182bdfc

Click to view benchmark
Test Base PR %
code_action 5.5±0.01ms 5.5±0.12ms 0.00%
code_lens 333.1±8.10ns 296.6±12.84ns -10.96%
compile 6.0±0.04s 6.0±0.02s 0.00%
completion 4.9±0.06ms 5.0±0.02ms +2.04%
did_change_with_caching 6.1±0.10s 6.0±0.06s -1.64%
document_symbol 975.0±40.35µs 953.7±25.85µs -2.18%
format 75.8±2.21ms 76.1±1.77ms +0.40%
goto_definition 362.2±6.73µs 446.7±7.41µs +23.33%
highlight 9.1±0.15ms 9.2±0.09ms +1.10%
hover 599.2±19.48µs 606.2±5.83µs +1.17%
idents_at_position 121.3±0.22µs 122.6±1.33µs +1.07%
inlay_hints 666.0±26.46µs 674.9±18.81µs +1.34%
on_enter 480.6±14.08ns 503.6±15.27ns +4.79%
parent_decl_at_position 3.7±0.03ms 3.8±0.17ms +2.70%
prepare_rename 358.9±5.99µs 365.6±8.53µs +1.87%
rename 9.6±0.15ms 9.6±0.02ms 0.00%
semantic_tokens 1048.6±32.30µs 1085.5±21.66µs +3.52%
token_at_position 356.8±1.74µs 363.8±1.10µs +1.96%
tokens_at_position 3.7±0.03ms 3.7±0.03ms 0.00%
tokens_for_file 413.7±4.51µs 424.0±4.15µs +2.49%
traverse 49.3±1.98ms 49.0±2.25ms -0.61%

@JoshuaBatty JoshuaBatty requested a review from a team April 11, 2024 00:52
@JoshuaBatty JoshuaBatty merged commit 6c21a39 into master Apr 11, 2024
36 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/storage_gc_test branch April 11, 2024 09:08
IGI-111 added a commit that referenced this pull request Apr 11, 2024
## Description
Bump repo to 0.53.0

waiting on #5813

Co-authored-by: IGI-111 <igi-111@protonmail.com>
JoshuaBatty added a commit that referenced this pull request 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

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [x] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen language server LSP server
Projects
None yet
4 participants