Reuse a C# project's in-memory PE reference while its semantic version holds

docs/release-notes/.VisualStudio/18.vNext.md

@@ -5,6 +5,7 @@
 
 ### Fixed
 
+* Reuse the in-memory PE reference of a referenced C# project while its dependent semantic version is unchanged, so dependent F# projects no longer re-import every reference and re-check every file each time Roslyn recreates the `Compilation`. ([PR #20460](https://github.com/dotnet/fsharp/pull/20460))
 * Improve Find All References performance by throttling parallel typechecks. ([PR #20128](https://github.com/dotnet/fsharp/pull/20128))
 * Fixed Rename incorrectly renaming `get` and `set` keywords for properties with explicit accessors. ([Issue #18270](https://github.com/dotnet/fsharp/issues/18270), [PR #19252](https://github.com/dotnet/fsharp/pull/19252))
 * Fixed Find All References crash when F# project contains non-F# files like `.cshtml`. ([Issue #16394](https://github.com/dotnet/fsharp/issues/16394), [PR #19252](https://github.com/dotnet/fsharp/pull/19252))

tests/fsharp/Compiler/Service/MultiProjectTests.fs

@@ -4,6 +4,7 @@ namespace FSharp.Compiler.UnitTests
 
 open System
 open System.IO
+open System.Threading
 open FSharp.Compiler.Diagnostics
 open Xunit
 open FSharp.Test
@@ -209,3 +210,91 @@ let y = 1
 
 
 
+
+    // Focused counters for https://github.com/dotnet/fsharp/pull/20460#discussion_r3965947159:
+    // a referenced C# project's `Compilation` is recreated on every solution fork, but its
+    // in-memory PE reference is only re-emitted when the project's dependent semantic version
+    // (here, the reference's stamp) actually changes.
+    let private mkCountedCSharpPEReference (stamp: DateTime) =
+        let csSrc =
+            """
+namespace CSharpTest
+{
+    public class CSharpClass
+    {
+    }
+}
+            """
+
+        let csOptions = CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)
+        let csSyntax = CSharpSyntaxTree.ParseText(csSrc)
+        let csReferences = TargetFrameworkUtil.getReferences TargetFramework.NetStandard20
+        let cs = CSharpCompilation.Create("csharp_test.dll", references = csReferences.As<MetadataReference>(), syntaxTrees = [csSyntax], options = csOptions)
+
+        let mutable emitCount = 0
+
+        let getStream ct =
+            Interlocked.Increment(&emitCount) |> ignore
+            let ms = new MemoryStream()
+            cs.Emit(ms, cancellationToken = ct) |> ignore
+            ms.Position <- 0L
+            ms :> Stream |> Some
+
+        let csRefProj = FSharpReferencedProject.PEReference((fun () -> stamp), DelayedILModuleReader("""Z:\csharp_test.dll""", getStream))
+
+        csRefProj, (fun () -> emitCount)
+
+    let private projectReferencing (csRefProj: FSharpReferencedProject) =
+        let fsOptions = CompilerAssert.DefaultProjectOptions TargetFramework.Current
+
+        { fsOptions with
+            ProjectId = Some(Guid.NewGuid().ToString())
+            OtherOptions = Array.append fsOptions.OtherOptions [|"""-r:Z:\csharp_test.dll"""|]
+            ReferencedProjects = [|csRefProj|] }
+
+    let private checkUsesCSharpClass (options: FSharpProjectOptions) =
+        let fsText =
+            """
+module FSharpTest
+
+open CSharpTest
+
+let test() =
+    CSharpClass()
+            """
+            |> SourceText.ofString
+
+        match
+            CompilerAssert.Checker.ParseAndCheckFileInProject("test.fs", 0, fsText, options)
+            |> Async.RunSynchronouslyImmediate
+            |> snd
+        with
+        | FSharpCheckFileAnswer.Aborted -> failwith "check file aborted"
+        | FSharpCheckFileAnswer.Succeeded checkResults -> Assert.shouldBeEmpty checkResults.Diagnostics
+
+    [<Fact>]
+    let ``Reusing a CSharp reference's stamp avoids re-emitting a recreated Compilation``() =
+        let stamp = DateTime(2024, 1, 1)
+        let csRefProj1, emitCount1 = mkCountedCSharpPEReference stamp
+        let csRefProj2, emitCount2 = mkCountedCSharpPEReference stamp
+
+        let fsOptions = projectReferencing csRefProj1
+        checkUsesCSharpClass fsOptions
+        Assert.Equal(1, emitCount1())
+
+        // Same dependent semantic version (stamp unchanged): the checker must reuse its cached
+        // project build and never touch the recreated Compilation behind the new reference.
+        checkUsesCSharpClass { fsOptions with ReferencedProjects = [|csRefProj2|] }
+        Assert.Equal(0, emitCount2())
+
+    [<Fact>]
+    let ``Changing a CSharp reference's stamp does re-emit the new Compilation``() =
+        let csRefProj1, _ = mkCountedCSharpPEReference (DateTime(2024, 1, 1))
+        let csRefProj2, emitCount2 = mkCountedCSharpPEReference (DateTime(2024, 1, 2))
+
+        let fsOptions = projectReferencing csRefProj1
+        checkUsesCSharpClass fsOptions
+
+        // Different dependent semantic version (stamp changed): the checker must pick up the new reference.
+        checkUsesCSharpClass { fsOptions with ReferencedProjects = [|csRefProj2|] }
+        Assert.True(emitCount2() >= 1)

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

@@ -14,7 +14,6 @@ open FSharp.Compiler.CodeAnalysis
 open Microsoft.VisualStudio.FSharp.Editor
 open System.Threading
 open Microsoft.VisualStudio.FSharp.Interactive.Session
-open System.Runtime.CompilerServices
 open CancellableTasks
 open Microsoft.VisualStudio.FSharp.Editor.Extensions
 open System.Windows
@@ -101,6 +100,31 @@ module private FSharpProjectOptionsHelpers =
         else
             hasProjectVersionChanged
 
+/// <summary>
+/// The in-memory PE reference of a referenced project, kept while the project's dependent
+/// semantic version is unchanged. Roslyn recreates
+/// <see cref="T:Microsoft.CodeAnalysis.Compilation"/> instances freely - on every solution fork,
+/// and under memory pressure because it holds the final compilation weakly - and a reference
+/// created per instance carries a fresh stamp that invalidates every FCS cache keyed on it.
+/// </summary>
+[<Sealed>]
+type private PEReferenceCacheEntry(version: VersionStamp, compilation: Compilation) =
+    // Pinned until the first emit result, so the reader can always be materialised.
+    let mutable pinned = compilation
+    let latest = WeakReference<Compilation>(compilation)
+
+    member _.Version = version
+
+    member _.TryGetCompilation() =
+        match pinned with
+        | null ->
+            match latest.TryGetTarget() with
+            | true, compilation -> ValueSome compilation
+            | _ -> ValueNone
+        | pinned -> ValueSome pinned
+
+    member _.Emitted() = pinned <- null
+
 [<RequireQualifiedAccess>]
 type private FSharpProjectOptionsMessage =
     | TryGetOptionsByDocument of
@@ -131,74 +155,76 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
     let singleFileCache =
         ConcurrentDictionary<DocumentId, Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription>()
 
-    // This is used to not constantly emit the same compilation.
-    let weakPEReferences = ConditionalWeakTable<Compilation, FSharpReferencedProject>()
+    let peReferences =
+        ConcurrentDictionary<ProjectId, PEReferenceCacheEntry * FSharpReferencedProject>()
+
     let lastSuccessfulCompilations = ConcurrentDictionary<ProjectId, Compilation>()
 
     let scriptUpdatedEvent = Event<FSharpProjectOptions>()
 
-    let createPEReference (referencedProject: Project) (comp: Compilation) =
+    let buildPEReference (referencedProject: Project) (entry: PEReferenceCacheEntry) =
         let projectId = referencedProject.Id
-
-        match weakPEReferences.TryGetValue comp with
-        | true, fsRefProj -> fsRefProj
-        | _ ->
-            let mutable strongComp = comp
-            let weakComp = WeakReference<Compilation>(comp)
-            let mutable stamp = DateTime.UtcNow
-
-            // Getting a C# reference assembly can fail if there are compilation errors that cannot be resolved.
-            // To mitigate this, we store the last successful compilation of a C# project and re-use it until we get a new successful compilation.
-            let getStream =
-                fun ct ->
-                    let tryStream (comp: Compilation) =
-                        let ms = new MemoryStream() // do not dispose the stream as it will be owned on the reference.
-
-                        let emitOptions =
-                            Emit.EmitOptions(metadataOnly = true, includePrivateMembers = false, tolerateErrors = true)
-
-                        try
-                            let result = comp.Emit(ms, options = emitOptions, cancellationToken = ct)
-
-                            if result.Success then
-                                strongComp <- Unchecked.defaultof<_> // Stop strongly holding the compilation since we have a result.
-                                lastSuccessfulCompilations.[projectId] <- comp
-                                ms.Position <- 0L
-                                ms :> Stream |> Some
-                            else
-                                strongComp <- Unchecked.defaultof<_> // Stop strongly holding the compilation since we have a result.
-                                ms.Dispose() // it failed, dispose of stream
-                                None
-                        with
-                        | :? OperationCanceledException ->
-                            // Since we cancelled, do not null out the strong compilation ref and update the stamp.
-                            stamp <- DateTime.UtcNow
-                            ms.Dispose()
-                            None
-                        | _ ->
-                            strongComp <- Unchecked.defaultof<_> // Stop strongly holding the compilation since we have a result.
+        let mutable stamp = DateTime.UtcNow
+
+        // Getting a C# reference assembly can fail if there are compilation errors that cannot be resolved.
+        // To mitigate this, we store the last successful compilation of a C# project and re-use it until we get a new successful compilation.
+        let getStream =
+            fun ct ->
+                let tryStream (comp: Compilation) =
+                    let ms = new MemoryStream() // do not dispose the stream as it will be owned on the reference.
+
+                    let emitOptions =
+                        Emit.EmitOptions(metadataOnly = true, includePrivateMembers = false, tolerateErrors = true)
+
+                    try
+                        let result = comp.Emit(ms, options = emitOptions, cancellationToken = ct)
+
+                        if result.Success then
+                            entry.Emitted()
+                            lastSuccessfulCompilations.[projectId] <- comp
+                            ms.Position <- 0L
+                            ms :> Stream |> Some
+                        else
+                            entry.Emitted()
                             ms.Dispose() // it failed, dispose of stream
                             None
+                    with
+                    | :? OperationCanceledException ->
+                        // Since we cancelled, keep the compilation pinned and update the stamp.
+                        stamp <- DateTime.UtcNow
+                        ms.Dispose()
+                        None
+                    | _ ->
+                        entry.Emitted()
+                        ms.Dispose() // it failed, dispose of stream
+                        None
 
-                    let resultOpt =
-                        match weakComp.TryGetTarget() with
-                        | true, comp -> tryStream comp
-                        | _ -> None
+                let resultOpt =
+                    match entry.TryGetCompilation() with
+                    | ValueSome comp -> tryStream comp
+                    | ValueNone -> None
 
-                    match resultOpt with
-                    | Some _ -> resultOpt
-                    | _ ->
-                        match lastSuccessfulCompilations.TryGetValue(projectId) with
-                        | true, comp -> tryStream comp
-                        | _ -> None
+                match resultOpt with
+                | Some _ -> resultOpt
+                | _ ->
+                    match lastSuccessfulCompilations.TryGetValue(projectId) with
+                    | true, comp -> tryStream comp
+                    | _ -> None
+
+        let getStamp = fun () -> stamp
 
-            let getStamp = fun () -> stamp
+        FSharpReferencedProject.PEReference(getStamp, DelayedILModuleReader(referencedProject.OutputFilePath, getStream))
 
-            let fsRefProj =
-                FSharpReferencedProject.PEReference(getStamp, DelayedILModuleReader(referencedProject.OutputFilePath, getStream))
+    let tryGetPEReference (referencedProject: Project) (version: VersionStamp) =
+        match peReferences.TryGetValue referencedProject.Id with
+        | true, (entry, fsRefProj) when entry.Version = version -> ValueSome fsRefProj
+        | _ -> ValueNone
 
-            weakPEReferences.Add(comp, fsRefProj)
-            fsRefProj
+    let createPEReference (referencedProject: Project) (version: VersionStamp) (comp: Compilation) =
+        let entry = PEReferenceCacheEntry(version, comp)
+        let fsRefProj = buildPEReference referencedProject entry
+        peReferences.[referencedProject.Id] <- (entry, fsRefProj)
+        fsRefProj
 
     let rec tryComputeOptionsBySingleScriptOrFile (document: Document) userOpName =
         cancellableTask {
@@ -351,9 +377,13 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
                                     FSharpReferencedProject.FSharpReference(referencedProject.OutputFilePath, projectOptions)
                                 )
                         elif referencedProject.SupportsCompilation then
-                            let! comp = referencedProject.GetCompilationAsync(ct)
-                            let peRef = createPEReference referencedProject comp
-                            referencedProjects.Add(peRef)
+                            let! version = referencedProject.GetDependentSemanticVersionAsync(ct)
+
+                            match tryGetPEReference referencedProject version with
+                            | ValueSome peRef -> referencedProjects.Add peRef
+                            | ValueNone ->
+                                let! comp = referencedProject.GetCompilationAsync(ct)
+                                referencedProjects.Add(createPEReference referencedProject version comp)
 
                 if canBail then
                     return ValueNone
@@ -427,6 +457,11 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
                                 if not (currentSolution.ContainsProject(pair.Key)) then
                                     lastSuccessfulCompilations.TryRemove(pair.Key) |> ignore)
 
+                            peReferences.ToArray()
+                            |> Array.iter (fun pair ->
+                                if not (currentSolution.ContainsProject(pair.Key)) then
+                                    peReferences.TryRemove(pair.Key) |> ignore)
+
                             checker.InvalidateConfiguration(projectOptions, userOpName = "tryComputeOptions")
 
                             let parsingOptions, _ = checker.GetParsingOptionsFromProjectOptions(projectOptions)
@@ -512,16 +547,15 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
 
                 | FSharpProjectOptionsMessage.ClearOptions(projectId) ->
                     match cache.TryRemove(projectId) with
-                    | true, struct (_, _, projectOptions) ->
-                        lastSuccessfulCompilations.TryRemove(projectId) |> ignore
-                        checker.ClearCache([ projectOptions ])
+                    | true, struct (_, _, projectOptions) -> checker.ClearCache([ projectOptions ])
                     | _ -> ()
 
+                    lastSuccessfulCompilations.TryRemove(projectId) |> ignore
+                    peReferences.TryRemove(projectId) |> ignore
                     legacyProjectSites.TryRemove(projectId) |> ignore
                 | FSharpProjectOptionsMessage.ClearSingleFileOptionsCache(documentId) ->
                     match singleFileCache.TryRemove(documentId) with
                     | true, (_, _, _, projectOptions, subscription) ->
-                        lastSuccessfulCompilations.TryRemove(documentId.ProjectId) |> ignore
                         checker.ClearCache([ projectOptions ])
                         subscription |> Option.iter (fun handler -> handler.Dispose())
                     | _ -> ()
@@ -559,6 +593,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
         cache.Clear()
         singleFileCache.Clear()
         lastSuccessfulCompilations.Clear()
+        peReferences.Clear()
 
     member _.ScriptUpdated = scriptUpdatedEvent.Publish