Enable TypeSubsumptionCache for IDE use #18499

majocha · 2025-04-24T12:18:05Z

Description

This builds upon Vlad's work from #18190 and #17668, extending the fix for #17501 into incremental compilation / typecheck in IDE editor.

This is resubmitted and cleaned up #18468.
TODO:
tests, comments, write-up, discussion

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/release-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 · 2025-04-24T12:18:48Z

❗ Release notes required

@majocha,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path	Release notes path	Description
`src/Compiler`	docs/release-notes/.FSharp.Compiler.Service/9.0.300.md	No release notes found or release notes format is not correct
`vsintegration/src`	docs/release-notes/.VisualStudio/17.14.md	No release notes found or release notes format is not correct

src/Compiler/Utilities/TypeHashing.fs

src/Compiler/Utilities/Caches.fs

majocha · 2025-04-24T22:25:42Z

src/Compiler/Checking/import.fs

@@ -106,15 +133,13 @@ type [<Struct; NoComparison; CustomEquality>] TTypeCacheKey =
 type ImportMap(g: TcGlobals, assemblyLoader: AssemblyLoader) =
    let typeRefToTyconRefCache = ConcurrentDictionary<ILTypeRef, TyconRef>()

-    let typeSubsumptionCache = ConcurrentDictionary<TTypeCacheKey, bool>(System.Environment.ProcessorCount, 1024)


In incremental mode ImportMap get created quite a lot, so this had to be attached somewhere else.

src/Compiler/Checking/import.fs

majocha · 2025-04-24T22:30:37Z

src/Compiler/Utilities/Caches.fs

+open System.Diagnostics.Metrics
+
+[<Struct; RequireQualifiedAccess; NoComparison>]
+type EvictionMethod =


I removed Blocking eviction method for now. It's another thing that would rot, because it's unused.

majocha · 2025-04-24T22:33:03Z

src/Compiler/Utilities/Caches.fs

+            member _.Remove(_) = ()
+        }
+
+type ICacheEvents =


Are events needed or should we just use Metrics counters directly?

It was added for observability.
If we can use metrics for it, it serves better.

(also less worries about loose GC handles IMO)

majocha · 2025-04-24T22:45:51Z

tests/FSharp.Test.Utilities/XunitHelpers.fs

+                use meterProvider =
+                    OpenTelemetry.Sdk.CreateMeterProviderBuilder()
+                        .AddMeter(nameof FSharp.Compiler.CacheInstrumentation)
+                        .AddMeter("System.Runtime")


This "System.Runtime" meter works only for net9.0 but is actually quite awesome. GC, memory, and lots of other metrics can be viewed in Prometheus in real time while the tests run. I use a simple docker setup like this

majocha · 2025-04-24T22:47:27Z

vsintegration/src/FSharp.Editor/LanguageService/LanguageService.fs

    // FSI-LINKAGE-POINT: unsited init
    do FSharp.Interactive.Hooks.fsiConsoleWindowPackageCtorUnsited (this :> Package)

+    // Uncomment to view cache metrics in the output window
+    // do Logging.FSharpServiceTelemetry.logCacheMetricsToOutput ()


Cache metrics viewable directly in VS output window.

majocha · 2025-04-24T22:51:35Z

src/Compiler/Utilities/Caches.fs

+    let private overrideVariable = "FSHARP_CACHE_OVERRIDE"
+
+    /// Use for testing purposes to reduce memory consumption in testhost and its subprocesses.
+    let OverrideMaxCapacityForTesting () =


Unfortunately, the settings that work best irl are not great for the CI. I'm still not sure what's going on, so this is an attempt.

src/Compiler/Checking/import.fs

T-Gro · 2025-04-25T08:23:56Z

src/Compiler/Utilities/Caches.fs

+        let options =
+            match Environment.GetEnvironmentVariable(overrideVariable) with
+            | null -> options
+            | _ -> { options with MaximumCapacity = 1024 }


So we have got:

CacheOptions.Default.MaximumCapacity => That one is never used ... ?

createTypeSubsumptionCache depending on the options.

and this piece overrides it agnostic of the options (I guess it assumes that CI run is always one-off and isolated, and works with a very small cache intentionally).

?

Yes the idea is to override only the MaximumCapacity when run in testhost process. We call OverrideMaxCapacityForTesting () at the start of xUnit test run. The env var should propagate to app domains on net472, and even subprocesses that we start a lot.

Ok that makes sense.
I agree to rather have small cache than no-cache at all for CI, so that we aspire to keep a higher level of dogfooding

deal with memory restrictions in CI, dispose caches

majocha · 2025-04-25T12:14:41Z

src/Compiler/Checking/import.fs

@@ -108,7 +108,9 @@ let createTypeSubsumptionCache (g: TcGlobals) =
                MaximumCapacity = 4 * 32768 }
    Cache.Create<TTypeCacheKey, bool>(options)

-let typeSubsumptionCaches = ConditionalWeakTable<TcGlobals, Cache<TTypeCacheKey, bool>>()
+let typeSubsumptionCaches = Cache.Create<TcGlobals, Cache<TTypeCacheKey, bool>>({ CacheOptions.Default with MaximumCapacity = 16 })


Each cache held here roughly corresponds to one assembly being built / one incremental builder. My understanding is that there is not much turnover here, unless something on the project level changes.
In a test run the story is different, hundreds can be created and disposed.

Now, that kind of asks for LFU.

Wouldn't LRU be also fitting here?
If a project does not need a cache (= likely is not getting touched for many minutes...) for a longer time, just get rid of it?

majocha · 2025-04-25T14:26:16Z

Performance of this is clearly off. I think I fixed the excessive memory use in CI (notwithstanding the Linux leg memory warnings, but they were always there ), but still, it runs much slower than main.

majocha added 3 commits April 24, 2025 13:54

add cache

5233567

enable typeSubsumptionCache in IDE

df70074

add some monitoring

460f592

github-project-automation bot added this to F# Compiler and Tooling Apr 24, 2025

github-project-automation bot moved this to New in F# Compiler and Tooling Apr 24, 2025

Merge branch 'main' into compiler-cache

e94a9e5

T-Gro reviewed Apr 24, 2025

View reviewed changes

src/Compiler/Utilities/TypeHashing.fs Show resolved Hide resolved

T-Gro reviewed Apr 24, 2025

View reviewed changes

src/Compiler/Utilities/Caches.fs Outdated Show resolved Hide resolved

majocha added 3 commits April 24, 2025 16:49

yeet LFU

b2a130a

flesh out the comparer

4c7e044

fix sources

a269b6a

majocha changed the title ~~Enable TypeSubSumptionCache in IDE use~~ Enable TypeSubsumptionCache for IDE use Apr 24, 2025

majocha added 2 commits April 25, 2025 00:06

try to deal with CI memory overload

874d9c1

Merge branch 'main' into compiler-cache

c266706

majocha commented Apr 24, 2025

View reviewed changes

src/Compiler/Checking/import.fs Outdated Show resolved Hide resolved

majocha commented Apr 24, 2025

View reviewed changes

majocha added 2 commits April 25, 2025 02:17

replace singleton with CWT again

4bca2df

remove sketchy logic

9ba405d

T-Gro reviewed Apr 25, 2025

View reviewed changes

src/Compiler/Checking/import.fs Outdated Show resolved Hide resolved

T-Gro reviewed Apr 25, 2025

View reviewed changes

majocha added 2 commits April 25, 2025 14:08

cache of caches

5c6a500

deal with memory restrictions in CI, dispose caches

Merge branch 'main' into compiler-cache

605c967

majocha commented Apr 25, 2025

View reviewed changes

ilver

3046f88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable TypeSubsumptionCache for IDE use #18499

Enable TypeSubsumptionCache for IDE use #18499

majocha commented Apr 24, 2025 •

edited

Loading

github-actions bot commented Apr 24, 2025

majocha Apr 24, 2025

majocha Apr 24, 2025

T-Gro Apr 25, 2025

majocha Apr 24, 2025

T-Gro Apr 25, 2025

majocha Apr 24, 2025 •

edited

Loading

majocha Apr 24, 2025

majocha Apr 24, 2025 •

edited

Loading

T-Gro Apr 25, 2025

majocha Apr 25, 2025

T-Gro Apr 25, 2025 •

edited

Loading

majocha Apr 25, 2025 •

edited

Loading

T-Gro Apr 25, 2025

majocha commented Apr 25, 2025

Enable TypeSubsumptionCache for IDE use #18499

Are you sure you want to change the base?

Enable TypeSubsumptionCache for IDE use #18499

Conversation

majocha commented Apr 24, 2025 • edited Loading

Description

Checklist

github-actions bot commented Apr 24, 2025

❗ Release notes required

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

majocha Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majocha Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

T-Gro Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

majocha Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majocha commented Apr 25, 2025

majocha commented Apr 24, 2025 •

edited

Loading

majocha Apr 24, 2025 •

edited

Loading

majocha Apr 24, 2025 •

edited

Loading

T-Gro Apr 25, 2025 •

edited

Loading

majocha Apr 25, 2025 •

edited

Loading