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

Add AST module caching support for LSP #5251

Merged
merged 29 commits into from
Nov 10, 2023
Merged

Add AST module caching support for LSP #5251

merged 29 commits into from
Nov 10, 2023

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented Nov 2, 2023

Description

This PR supersedes #4988 and builds on backend work done by @tritao in #4733.

A new ModuleId is created for each compiled module. The SourceId type now also stores the ModuleId that was used to create it. This allows us to perform garbage collection on types in the Declaration and Type engines that refer the provided module.

if let Some(module_id) = engines.se().get_module_id(&path) {
    engines.clear_module(&module_id);
}

This method isn't super cheap to call. My tests had this averaging at 54ms. We are currently performing garbage collection on every key stroke in LSP. We should look to have another metric such as only clearing once every 20 keystrokes or so in the future.

I ran benchmarking on recompiling the doc_comments example 20 times to simulate rapid keystrokes during coding. Here are the results.

Without Caching With Caching
89.489 s 4.6236 s

A speed up of 1835.49% is certainly great, yet 4.6 seconds still sounds like an eternity. Still have lots of room to improve but this should improve our current system dramatically.

closes #4994

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 added language server LSP server compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen labels Nov 2, 2023
@JoshuaBatty JoshuaBatty marked this pull request as draft November 2, 2023 02:55
@JoshuaBatty JoshuaBatty marked this pull request as ready for review November 2, 2023 05:24
@JoshuaBatty JoshuaBatty requested review from a team November 2, 2023 05:24
@tritao
Copy link
Contributor

tritao commented Nov 2, 2023

Really nice work, the speed up is massive!

This method isn't super cheap to call. My tests had this averaging at 54ms. We are currently performing garbage collection on every key stroke in LSP. We should look to have another metric such as only clearing once every 20 keystrokes or so in the future.

Calling it on every keystroke certainly seems excessive, even on every couple keystrokes. I think it could be worth thinking about having a memory size heuristics-based system instead, and calling it on idle time so it's less perceptible to the user.

@IGI-111
Copy link
Contributor

IGI-111 commented Nov 2, 2023

Very nice. It probably needs to be tuned further, but it's a great start and much cleaner than I would have expected for something like this.

@JoshuaBatty
Copy link
Member Author

Great, I'll start think about a nice heuristic to call gc from.

sdankel
sdankel previously approved these changes Nov 3, 2023
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.

Amazing performance improvement!

sway-types/Cargo.toml Outdated Show resolved Hide resolved
sway-lsp/tests/lib.rs Outdated Show resolved Hide resolved
sway-lsp/src/handlers/request.rs Outdated Show resolved Hide resolved
tritao
tritao previously approved these changes Nov 3, 2023
Copy link
Contributor

@tritao tritao left a comment

Choose a reason for hiding this comment

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

Overall looks great, though it seems some of the type engine insertions have a None source id. As far as I can tell, that means we won't track those, so we won't be able to GC them, right? Assuming that's the case, something we should look into improving in the future.

sway-core/src/type_system/info.rs Outdated Show resolved Hide resolved
@JoshuaBatty
Copy link
Member Author

Overall looks great, though it seems some of the type engine insertions have a None source id. As far as I can tell, that means we won't track those, so we won't be able to GC them, right? Assuming that's the case, something we should look into improving in the future.

Good question, at the top of the type engines insert function we check if None was provided

let source_id = source_id.map(Clone::clone).or_else(|| info_to_source_id(&ty));

If it's None then we try to map the type_info to a reserved source id.

/// Maps specific `TypeInfo` variants to a reserved `SourceId`, returning `None` for non-mapped types.
fn info_to_source_id(ty: &TypeInfo) -> Option<SourceId> {
    match ty {
        TypeInfo::Unknown
        | TypeInfo::UnsignedInteger(_)
        | TypeInfo::Numeric
        | TypeInfo::Boolean
        | TypeInfo::B256
        | TypeInfo::RawUntypedPtr
        | TypeInfo::RawUntypedSlice
        | TypeInfo::StringSlice
        | TypeInfo::Contract
        | TypeInfo::StringArray(_)
        | TypeInfo::Array(_, _) => Some(SourceId::reserved()),
        TypeInfo::Tuple(v) if v.is_empty() => Some(SourceId::reserved()),
        _ => None,
    }
}

This way we don't clear types that are shared between modules, and type_info variants that can hold spans are allowed to be None, thus can be cleared safely.

sdankel
sdankel previously approved these changes Nov 8, 2023
@JoshuaBatty
Copy link
Member Author

JoshuaBatty commented Nov 8, 2023

Calling it on every keystroke certainly seems excessive, even on every couple keystrokes. I think it could be worth thinking about having a memory size heuristics-based system instead, and calling it on idle time so it's less perceptible to the user.

I'm now only calling it on every 10th keystroke. Unfortunately we can't call it during idle as this would remove internal types needed for doing goto def, hover etc in the users workspace.

sdankel
sdankel previously approved these changes Nov 8, 2023
@JoshuaBatty JoshuaBatty requested a review from a team November 8, 2023 01:43
@JoshuaBatty
Copy link
Member Author

hmm calling it on every 10th keystroke is causing CI to run out of memory. I'll open an issue and we can work on this optimisation in a follow up PR.

@JoshuaBatty JoshuaBatty enabled auto-merge (squash) November 9, 2023 06:59
sdankel
sdankel previously approved these changes Nov 9, 2023
@JoshuaBatty JoshuaBatty requested a review from a team November 9, 2023 09:38
sway-lsp/tests/lib.rs Outdated Show resolved Hide resolved
@JoshuaBatty JoshuaBatty requested a review from a team November 10, 2023 01:10
@JoshuaBatty JoshuaBatty merged commit 28d3500 into master Nov 10, 2023
32 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/allocator branch November 10, 2023 08:13
IGI-111 added a commit that referenced this pull request Mar 10, 2024
## Description
Enables a did_change stress test with random wait times between each
trigger to emulate random key typing. This then internally kicks off
compilation and triggers garbage collection every 3 keystrokes.

I had intended to include this test when garbage collection was
implemented in #5251. However, at that time, we were only performing GC
every 10th keystroke and it was overloading the stack in CI. Now that we
have reduced this to every 3rd keystroke it seems to be fine for CI.

Unfortunately, this test wasn't running to catch a bug that slipped
through in #5306. As such, this PR currently allows a way for us to
reproduce this error but won't be able to pass CI until the underlying
issue is resolved.

P.S.: This also adds a temporary fix where we retain elements without a
source id. This is tantamount to leaking them but seems preferable to
disabling the GC altogether.

---------

Co-authored-by: xunilrj <[email protected]>
Co-authored-by: IGI-111 <[email protected]>
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.

Implement new allocator and support garbage collection
4 participants