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

LSP Optimization: Wrap programs_cache in an Arc #5472

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented Jan 15, 2024

Description

We clone the Engines before each keystroke in the language server. We shouldn't need to clone anything other than a pointer for the programs_cache. Measured against the benchmark example in sway-lsp tests, this saves us 23.015ms.

related to #5445

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 requested review from tritao and a team January 15, 2024 23:51
@JoshuaBatty JoshuaBatty self-assigned this Jan 15, 2024
@JoshuaBatty JoshuaBatty added language server LSP server compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Jan 15, 2024
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) January 15, 2024 23:51
ironcev
ironcev previously approved these changes Jan 16, 2024
@JoshuaBatty JoshuaBatty requested a review from a team January 16, 2024 09:34
@tritao
Copy link
Contributor

tritao commented Jan 16, 2024

Looks good, just wondering if we should wrap parse_module_cache while we're at it.

@JoshuaBatty
Copy link
Member Author

JoshuaBatty commented Jan 17, 2024

Looks good, just wondering if we should wrap parse_module_cache while we're at it.

Good idea, added in fe93ba1

@JoshuaBatty JoshuaBatty merged commit 30d8bfd into master Jan 17, 2024
34 of 35 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/clone_qe branch January 17, 2024 10:43
@JoshuaBatty JoshuaBatty mentioned this pull request Feb 5, 2024
7 tasks
JoshuaBatty added a commit that referenced this pull request Feb 5, 2024
## Description
This was added as an optimization in #5472. However, it seems to be the
cause of a transient bug that is causing the language server to crash
outlined in #5531.

We should revert this optimization until we understand how to avoid this
crash from happening. cc @tritao

## Checklist

- [x] 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](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [ ] 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
compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen language server LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants