Reduce excess memory usage in TransparentCompiler #17543

TheAngryByrd · 2024-08-15T12:24:09Z

Description

This fixes #16979 with some additions that have been discussed in various chats/twitter threads. I've combined them for discussion purposes, we can split them up as desired.

The 3 parts are:

Makes the snapshots -> options use a ConditionalWeakTable rather than recreating possibly hundreds of options objects.
Adds many various knobs to the transparent compiler caching code, allowing tweaking for editors.

❓ Outstanding questions ❓

What type of benchmarking should I do for this?
- I could show before/after on allocations in FSAC.
- Add a benchmark dotnet scenario but I'm unsure how big a project graph needs to be before this exhibits the behavior.

Checklist

Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:
Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

Release notes files:
- If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
- If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/releae-notes/.Language/preview.md
- If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.
Information about the release notes entries format can be found in the documentation.
Example:
- More inlines for Result module. (PR #16106)
- Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363)
  *while! (Language suggestion #1038, PR #14238)
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

github-actions · 2024-08-15T12:24:45Z

❗ Release notes required

✅ Found changes and release notes in following paths:

Change path Release notes path Description

src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

TheAngryByrd · 2024-08-15T12:32:04Z

src/Compiler/Service/TransparentCompiler.fs

@@ -258,45 +258,119 @@ module private TypeCheckingGraphProcessing =

            return finalFileResults, state
        }
-
-type internal CompilerCaches(sizeFactor: int) =
+type CacheSizes = {


Probably should be internal

This allows an editor to finely tweak these options. This potentially trades memory for CPU usage but on larger solutions it a tradeoff someone can make it possible to have multiple solutions open.

So, how would the editor come up with these numbers? I wouldn't suggest to expose this to the user.

I think you should be able to say "use more memory" or "use less memory", potentially even "hold more/less stuff strongly/weakly" but does it really make sense to have such granular control in the editor? So that different editors would benefit from different configurations somehow?

And if there is some smart way to set these based the solutions the user is working here it would be nice to have that code here so everyone can use it.

I exposed these numbers so it was easier to try to adjust them individually but I do agree it's not something I'd want to expose to a user directly.

I think the only settings come down to "memory" vs "recacluating". Specifically, my work project can be around 10-15gb once fully type checked, if I have multiple versions of open for various reasons, I'd rather try to be aggressive with collecting and keeping it's profile small.

I guess we can expose it now for easier experimenting with some [<Experimental>] comment that it will be removed/simplified for sure. What I initially put there was pretty arbitrary but hopefully we can eventually come to some consensus here and expose only a few parameters that will be enough to tweak it.

TheAngryByrd · 2024-08-15T12:32:15Z

src/Compiler/Service/FSharpProjectSnapshot.fs

@@ -559,7 +559,7 @@ and [<Experimental("This FCS API is experimental and subject to change.")>] FSha
            referencedProjects: FSharpReferencedProjectSnapshot list,
            isIncompleteTypeCheckEnvironment: bool,
            useScriptResolutionRules: bool,
-            loadTime: DateTime,
+            loadTime: DateTime, 


Why the space?

TheAngryByrd · 2024-08-15T12:32:41Z

src/Compiler/FSharp.Compiler.Service.fsproj

+    <InternalsVisibleTo Include="FsAutoComplete.Core" />
+    <InternalsVisibleTo Include="FsAutoComplete" />


The FSAC InternalsVisibleTo to allow the cache tweaks

This feels very wrong, is this a temporary thing?

The transparent compiler itself is marked internal and unless the F# team wants to make it public, this is a better method than forcing FSAC to do reflection all the time.

Yeah, that's true, but there's actually a way to enable it using FSharpChecker.Create(useTransparentCompiler = true). Shouldn't more things be driven by that?

I just think this approach feels like a workaround that avoids proper communication, which seems like a missed opportunity. But if the team doesn't see it that way and everyone's fine with it, then I suppose it's okay.

One other thing that concerns me is that this could potentially be a common request for every FCS user.

I wonder if we should expose everything before we stabilize it? Or, we can expose necessary parts, but put Experimental on them. Or expose them via the checker?

I don't have a strong preference here. I went with the least invasive changes first to do exactly this, facilitate discussion :)

One other thing that concerns me is that this could potentially be a common request for every FCS user.

Yeah I agree here. Though, it would be nice to also be able to access other things like LanguageFeatures instead of our hack

I wonder if we should expose everything before we stabilize it? We can expose necessary parts, but put Experimental on them. Or expose them via the checker?

Since FCS makes breaking changes that FSAC needs to handle anyway, it's all kind of the same thing in the end to me. Whatever makes more sense for your team.

We'd be happy to have such InternalsVisibleTo entries too in Rider plugin. 🙂
We're also used to the changes in APIs.

In this case we should just expose everything needed via APIs.

I would also propose to add [Experimental] APIs for everything you need. Then at least we see everything that's needed when deciding on a stable API.

TheAngryByrd · 2024-08-15T12:33:04Z

src/Compiler/Service/FSharpProjectSnapshot.fs

+let internal snapshotTable = ConditionalWeakTable<ProjectSnapshot, FSharpProjectOptions>()
+
+let rec internal snapshotToOptions (projectSnapshot: ProjectSnapshot) =
+    snapshotTable.GetValue(projectSnapshot, fun projectSnapshot ->
+        {
+            ProjectFileName = projectSnapshot.ProjectFileName
+            ProjectId = projectSnapshot.ProjectId
+            SourceFiles = projectSnapshot.SourceFiles |> Seq.map (fun x -> x.FileName) |> Seq.toArray
+            OtherOptions = projectSnapshot.CommandLineOptions |> List.toArray
+            ReferencedProjects =
+                projectSnapshot.ReferencedProjects
+                |> Seq.map (function
+                    | FSharpReference(name, opts) -> FSharpReferencedProject.FSharpReference(name, opts.ProjectSnapshot |> snapshotToOptions)
+                    | PEReference(getStamp, reader) -> FSharpReferencedProject.PEReference(getStamp, reader)
+                    | ILModuleReference(name, getStamp, getReader) -> FSharpReferencedProject.ILModuleReference(name, getStamp, getReader))
+                |> Seq.toArray
+            IsIncompleteTypeCheckEnvironment = projectSnapshot.IsIncompleteTypeCheckEnvironment
+            UseScriptResolutionRules = projectSnapshot.UseScriptResolutionRules
+            LoadTime = projectSnapshot.LoadTime
+            UnresolvedReferences = projectSnapshot.UnresolvedReferences
+            OriginalLoadReferences = projectSnapshot.OriginalLoadReferences
+            Stamp = projectSnapshot.Stamp
+        }    


This is the main fix around creating too many options

This makes sense. Out of curiosity what are you using the resulting options for?

There seems to be paths that still want a FSharpProjectOptions. Following the current use of snapshotToOptions should show them.

Specifically they show up in transparent compiler which get turned into longer lived objects here:

fsharp/src/Compiler/Service/TransparentCompiler.fs

Line 1629 in f4860a4

projectSnapshot.ToOptions(),

fsharp/src/Compiler/Service/TransparentCompiler.fs

Line 1898 in f4860a4

projectSnapshot.ToOptions())

Ah, I see, didn't notice these usages. Wonder what would be a good way to phase them out of there... But for now at least your fix improves it.

Yeah that would the real fix. I did do before that but it was more invasive.

vzarytovskii · 2024-08-16T14:06:15Z

I could show before/after on allocations in FSAC.

Yeah, that's good enough. Also, if possible, CPU consumption.

0101 · 2024-09-16T12:36:32Z

src/Compiler/Service/TransparentCompiler.fs

@@ -258,45 +258,119 @@ module private TypeCheckingGraphProcessing =

            return finalFileResults, state
        }
-
-type internal CompilerCaches(sizeFactor: int) =
+type CacheSizes = {


So, how would the editor come up with these numbers? I wouldn't suggest to expose this to the user.

I think you should be able to say "use more memory" or "use less memory", potentially even "hold more/less stuff strongly/weakly" but does it really make sense to have such granular control in the editor? So that different editors would benefit from different configurations somehow?

And if there is some smart way to set these based the solutions the user is working here it would be nice to have that code here so everyone can use it.

0101 · 2024-09-16T12:38:39Z

src/Compiler/Service/TransparentCompiler.fs

+    static member Default = 
+        let sizeFactor = 100
+        CacheSizes.Create sizeFactor
+type internal CompilerCaches(sizeFactor: CacheSizes) =

    let sf = sizeFactor


This should be renamed if we keep it like this.

0101 · 2024-09-16T12:41:10Z

src/Compiler/FSharp.Compiler.Service.fsproj

+    <InternalsVisibleTo Include="FsAutoComplete.Core" />
+    <InternalsVisibleTo Include="FsAutoComplete" />


I would also propose to add [Experimental] APIs for everything you need. Then at least we see everything that's needed when deciding on a stable API.

0101 · 2024-09-16T12:44:34Z

src/Compiler/Service/TransparentCompiler.fs

@@ -1363,8 +1440,8 @@ type internal TransparentCompiler
                        node,
                        (fun tcInfo ->

-                            if tcInfo.stateContainsNodes |> Set.contains fileNode then
-                                failwith $"Oops!"
+                            // if tcInfo.stateContainsNodes |> Set.contains fileNode then


This seems like an unrelated fix. Not sure if it's been discussed elsewhere.

Been discussed in slack before (probably beyond any history now 😢 ). Yeah this could be reverted but I've seen this popup crashing the TC hard.

Yeah. It seems a bit fishy. I need to understand if this is something that should legitimately be happening, clearly previously I thought it shouldn't 😅

0101 · 2024-09-16T13:00:19Z

src/Compiler/Service/FSharpProjectSnapshot.fs

+let internal snapshotTable = ConditionalWeakTable<ProjectSnapshot, FSharpProjectOptions>()
+
+let rec internal snapshotToOptions (projectSnapshot: ProjectSnapshot) =
+    snapshotTable.GetValue(projectSnapshot, fun projectSnapshot ->
+        {
+            ProjectFileName = projectSnapshot.ProjectFileName
+            ProjectId = projectSnapshot.ProjectId
+            SourceFiles = projectSnapshot.SourceFiles |> Seq.map (fun x -> x.FileName) |> Seq.toArray
+            OtherOptions = projectSnapshot.CommandLineOptions |> List.toArray
+            ReferencedProjects =
+                projectSnapshot.ReferencedProjects
+                |> Seq.map (function
+                    | FSharpReference(name, opts) -> FSharpReferencedProject.FSharpReference(name, opts.ProjectSnapshot |> snapshotToOptions)
+                    | PEReference(getStamp, reader) -> FSharpReferencedProject.PEReference(getStamp, reader)
+                    | ILModuleReference(name, getStamp, getReader) -> FSharpReferencedProject.ILModuleReference(name, getStamp, getReader))
+                |> Seq.toArray
+            IsIncompleteTypeCheckEnvironment = projectSnapshot.IsIncompleteTypeCheckEnvironment
+            UseScriptResolutionRules = projectSnapshot.UseScriptResolutionRules
+            LoadTime = projectSnapshot.LoadTime
+            UnresolvedReferences = projectSnapshot.UnresolvedReferences
+            OriginalLoadReferences = projectSnapshot.OriginalLoadReferences
+            Stamp = projectSnapshot.Stamp
+        }    


This makes sense. Out of curiosity what are you using the resulting options for?

TheAngryByrd · 2024-12-08T19:03:51Z

In terms of why I want to get this fix in:

My work project is around 70 projects. This is the use after a full typecheck of the solution in FSAC:

Background Compiler:

Transparent Compiler:

(Actually this wasn't a full typecheck because I ran out of memory and it wouldn't finish)

Transparent Compiler (with this PR):

TheAngryByrd · 2024-12-08T19:51:29Z

I can't seem to run the ilverify.ps1 script

& : The module 'fsharp' could not be loaded. For more information, run 'Import-Module fsharp'.
At C:\Users\jimmy\Repositories\public\TheAngryByrd\fsharp\tests\ILVerify\ilverify.ps1:41 char:7
+     & $script -c $configuration $additional_arguments
+       ~~~~~~~
    + CategoryInfo          : ObjectNotFound: (fsharp\build.sh:String) [], CommandNotFoundException
    + FullyQualifiedErrorId : CouldNotAutoLoadModule

vzarytovskii · 2024-12-09T07:35:04Z

I can't seem to run the ilverify.ps1 script


& : The module 'fsharp' could not be loaded. For more information, run 'Import-Module fsharp'.

At C:\Users\jimmy\Repositories\public\TheAngryByrd\fsharp\tests\ILVerify\ilverify.ps1:41 char:7

+     & $script -c $configuration $additional_arguments

+       ~~~~~~~

    + CategoryInfo          : ObjectNotFound: (fsharp\build.sh:String) [], CommandNotFoundException

    + FullyQualifiedErrorId : CouldNotAutoLoadModule

It requires pwsh, not powershell (i.e. version 7, not 5).

T-Gro

The results look really good Jimmy!

I would want to get this in soon to start using it, could you please resolve the deleted "failwith $"Oops!"" code and changing the "sf" naming to reflect new reality?

0101 · 2024-12-10T09:58:29Z

In terms of why I want to get this fix in:

[...]

Wow all of that because of the extra FSharpOptions 😲 Or did you tweak the cache sizes also?

TheAngryByrd changed the title ~~Snapshot fixes~~ Transparent Compiler memory reduction in editors Aug 15, 2024

TheAngryByrd commented Aug 15, 2024

View reviewed changes

0101 reviewed Sep 16, 2024

View reviewed changes

TheAngryByrd added 5 commits November 18, 2024 10:18

fixes snapshot creating too many fsharpoptions

d3e5e17

Configurable cache sizes

41dd131

let FSAC get at the internals

419216f

mm

7676fe7

Merge remote-tracking branch 'upstream/main' into snapshot-fixes

ad9751e

TheAngryByrd force-pushed the snapshot-fixes branch from 1a8e11c to ad9751e Compare December 8, 2024 18:01

TheAngryByrd added 2 commits December 8, 2024 13:59

Remove internalsvisibleto

c7949a8

Allow CacheSizes to be set

15be216

Revert testing files

74162b1

TheAngryByrd force-pushed the snapshot-fixes branch from 75aacaf to 74162b1 Compare December 8, 2024 19:10

TheAngryByrd changed the title ~~Transparent Compiler memory reduction in editors~~ Reduce excess memory usage in TransparentCompiler Dec 8, 2024

Add release note

cabf9bd

TheAngryByrd marked this pull request as ready for review December 8, 2024 19:15

TheAngryByrd requested a review from a team as a code owner December 8, 2024 19:15

Formatting

344e376

TheAngryByrd mentioned this pull request Dec 8, 2024

Transparent fixes ionide/FsAutoComplete#1326

Draft

T-Gro reviewed Dec 9, 2024

View reviewed changes

remove oops and rename cacheSizes

e52faa2

T-Gro approved these changes Dec 10, 2024

View reviewed changes

Update surfacearea

d3175b3

0101 approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'main' into snapshot-fixes

a9cd021

T-Gro enabled auto-merge (squash) December 10, 2024 11:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce excess memory usage in TransparentCompiler #17543

Reduce excess memory usage in TransparentCompiler #17543

TheAngryByrd commented Aug 15, 2024 •

edited

Loading

github-actions bot commented Aug 15, 2024 •

edited

Loading

TheAngryByrd Aug 15, 2024

TheAngryByrd Aug 15, 2024 •

edited

Loading

0101 Sep 16, 2024

TheAngryByrd Sep 16, 2024

0101 Sep 16, 2024 •

edited

Loading

TheAngryByrd Aug 15, 2024

TheAngryByrd Aug 15, 2024

nojaf Aug 16, 2024

TheAngryByrd Aug 16, 2024

nojaf Aug 16, 2024 •

edited

Loading

vzarytovskii Aug 16, 2024 •

edited

Loading

TheAngryByrd Aug 16, 2024

auduchinok Aug 23, 2024

vzarytovskii Aug 23, 2024

0101 Sep 16, 2024

TheAngryByrd Aug 15, 2024

0101 Sep 16, 2024

TheAngryByrd Sep 16, 2024

0101 Sep 16, 2024

TheAngryByrd Sep 16, 2024

vzarytovskii commented Aug 16, 2024

0101 Sep 16, 2024

0101 Sep 16, 2024

0101 Sep 16, 2024

0101 Sep 16, 2024

TheAngryByrd Sep 16, 2024

0101 Sep 16, 2024

0101 Sep 16, 2024

TheAngryByrd commented Dec 8, 2024

TheAngryByrd commented Dec 8, 2024

vzarytovskii commented Dec 9, 2024

T-Gro left a comment

0101 commented Dec 10, 2024

		<InternalsVisibleTo Include="FsAutoComplete.Core" />
		<InternalsVisibleTo Include="FsAutoComplete" />

Reduce excess memory usage in TransparentCompiler #17543

Are you sure you want to change the base?

Reduce excess memory usage in TransparentCompiler #17543

Conversation

TheAngryByrd commented Aug 15, 2024 • edited Loading

Description

❓ Outstanding questions ❓

Checklist

github-actions bot commented Aug 15, 2024 • edited Loading

❗ Release notes required

Choose a reason for hiding this comment

TheAngryByrd Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0101 Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

vzarytovskii Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzarytovskii commented Aug 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAngryByrd commented Dec 8, 2024

TheAngryByrd commented Dec 8, 2024

vzarytovskii commented Dec 9, 2024

T-Gro left a comment

Choose a reason for hiding this comment

0101 commented Dec 10, 2024

TheAngryByrd commented Aug 15, 2024 •

edited

Loading

github-actions bot commented Aug 15, 2024 •

edited

Loading

TheAngryByrd Aug 15, 2024 •

edited

Loading

0101 Sep 16, 2024 •

edited

Loading

nojaf Aug 16, 2024 •

edited

Loading

vzarytovskii Aug 16, 2024 •

edited

Loading