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

Don't store untyped AST in state #691

Merged
merged 1 commit into from
Dec 28, 2020
Merged

Conversation

Krzysztof-Cieslak
Copy link
Member

This significantly reduces memory usage, especially on big projects. On FSharp.sln I observed reducing memory usage in main FSAC process from 2800 MB to 2100 MB in similar usage scenario (initial load, wait for background service to stabilize, do some small operations in a file from FSharp.Compiler.Service.fsproj)

Memory dump pre-change: (notice super high allocations for objects from AST)
image

Memory dump post-change:
image

@Krzysztof-Cieslak Krzysztof-Cieslak merged commit f3fbbbf into master Dec 28, 2020
@@ -105,7 +105,7 @@ type FsacClient(sendServerRequest: ClientNotificationSender) =
type BackgroundServiceServer(state: State, client: FsacClient) =
inherit LspServer()

let checker = FSharpChecker.Create(projectCacheSize = 1, keepAllBackgroundResolutions = false, suggestNamesForErrors = true)
let checker = FSharpChecker.Create(projectCacheSize = 1, keepAllBackgroundResolutions = false, suggestNamesForErrors = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

We make projectCacheSize 200 by default, it should probably just be 1000 or something silly. I highly recommend increasing its size. It significantly increases the CPU and memory usage of FCS for multi-project solutions because of a high degree of cache thrashing. Whatever guidance existed about this number before we did an actual investigation of this is wrong. See here for more info: dotnet/fsharp#10217

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is in the background service. Hmm. Not sure what to think of that, I assum eit only touches one project

@baronfel baronfel deleted the remove-ast-from-state branch October 21, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants