diff --git a/docs/release-notes/.VisualStudio/18.vNext.md b/docs/release-notes/.VisualStudio/18.vNext.md index e6034dca8df..4ece675cd86 100644 --- a/docs/release-notes/.VisualStudio/18.vNext.md +++ b/docs/release-notes/.VisualStudio/18.vNext.md @@ -7,6 +7,7 @@ * Go To Definition no longer blocks the UI thread with a bare `Task.Wait`: the synchronous `IFSharpGoToDefinitionService` call now waits through the cancellable threaded-wait dialog, and the editor's `TaskCompletionSource` bridges run their continuations on the thread pool instead of inline on whichever thread finished the check, so repeated F12 on a large solution no longer starves semantic classification and other main-thread work. ([PR #20482](https://github.com/dotnet/fsharp/pull/20482)) * Peek Definition on an F# symbol whose definition lives in metadata no longer deadlocks Visual Studio. Peek holds the main thread in `JoinableTaskFactory.Run` without pumping messages while it asks the language service for the definition, and generating the metadata document needs that same thread; Peek now stops at definitions that already have a document, and Go To Definition, which owns the wait it makes, still opens the generated one. ([PR #20503](https://github.com/dotnet/fsharp/pull/20503)) +* 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)) diff --git a/tests/fsharp/Compiler/Service/MultiProjectTests.fs b/tests/fsharp/Compiler/Service/MultiProjectTests.fs index 2633494f3e4..5a8d22073a3 100644 --- a/tests/fsharp/Compiler/Service/MultiProjectTests.fs +++ b/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(), 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 + + [] + 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()) + + [] + 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) diff --git a/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs b/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs index db73206996b..678a3886410 100644 --- a/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs +++ b/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 +/// +/// The in-memory PE reference of a referenced project, kept while the project's dependent +/// semantic version is unchanged. Roslyn recreates +/// 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. +/// +[] +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) + + 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 + [] type private FSharpProjectOptionsMessage = | TryGetOptionsByDocument of @@ -131,74 +155,76 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) = let singleFileCache = ConcurrentDictionary() - // This is used to not constantly emit the same compilation. - let weakPEReferences = ConditionalWeakTable() + let peReferences = + ConcurrentDictionary() + let lastSuccessfulCompilations = ConcurrentDictionary() let scriptUpdatedEvent = Event() - 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(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 { @@ -349,9 +375,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 @@ -425,6 +455,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) @@ -510,16 +545,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()) | _ -> () @@ -557,6 +591,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) = cache.Clear() singleFileCache.Clear() lastSuccessfulCompilations.Clear() + peReferences.Clear() member _.ScriptUpdated = scriptUpdatedEvent.Publish