Reuse a C# project's in-memory PE reference while its semantic version holds #20460
+189
−64
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,6 @@ open FSharp.Compiler.CodeAnalysis | |
| open Microsoft.VisualStudio.FSharp.Editor | ||
| open System.Threading | ||
| open Microsoft.VisualStudio.FSharp.Interactive.Session | ||
| 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>() | ||
|
|
||
| let peReferences = | ||
| ConcurrentDictionary<ProjectId, PEReferenceCacheEntry * FSharpReferencedProject>() | ||
|
|
||
| let lastSuccessfulCompilations = ConcurrentDictionary<ProjectId, Compilation>() | ||
|
|
||
| let scriptUpdatedEvent = Event<FSharpProjectOptions>() | ||
|
|
||
| let buildPEReference (referencedProject: Project) (entry: PEReferenceCacheEntry) = | ||
| let projectId = referencedProject.Id | ||
| 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 entry.TryGetCompilation() with | ||
| | ValueSome comp -> tryStream comp | ||
| | ValueNone -> None | ||
|
|
||
| match resultOpt with | ||
| | Some _ -> resultOpt | ||
| | _ -> | ||
| match lastSuccessfulCompilations.TryGetValue(projectId) with | ||
| | true, comp -> tryStream comp | ||
| | _ -> None | ||
|
|
||
| let getStamp = fun () -> stamp | ||
|
|
||
| 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 | ||
|
|
||
| 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! version = referencedProject.GetDependentSemanticVersionAsync(ct) | ||
|
|
||
| match tryGetPEReference referencedProject version with | ||
|
Member
There was a problem hiding this comment. 🤖🕵️⏱️🔥
Contributor
Author
There was a problem hiding this comment. Added a focused test in 3d74593:
Both pass locally (2/2), pre-existing This proves the emit/import avoidance directly (no editor Workspace needed —
Member
There was a problem hiding this comment. @xperiandri TODO - I think this hasnt been addressed
Contributor
Author
There was a problem hiding this comment. It is in, in
So of the three things asked for, PE emits are counted and proven, in both directions. Imports and checks are not counted — nothing in the test host exposes those counters, and reaching them would mean driving If you want the import and check counts as well, I would rather do that as a separate change that gives FCS a test hook for them, since the same hook is what any future claim about reference invalidation will need. |
||
| | 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) -> checker.ClearCache([ projectOptions ]) | ||
| | _ -> () | ||
|
|
||
|
xperiandri marked this conversation as resolved.
|
||
| lastSuccessfulCompilations.TryRemove(projectId) |> ignore | ||
| peReferences.TryRemove(projectId) |> ignore | ||
| legacyProjectSites.TryRemove(projectId) |> ignore | ||
| | FSharpProjectOptionsMessage.ClearSingleFileOptionsCache(documentId) -> | ||
| match singleFileCache.TryRemove(documentId) with | ||
| | true, (_, _, _, projectOptions, subscription) -> | ||
| 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 | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right about what they cover. They prove the mechanism this change relies on — that FCS reuses a
PEReferencewhile its stamp holds and re-emits when it changes — and they would indeed pass without the change, because they hand FCS the stamps directly instead of lettingFSharpProjectOptionsManagerchoose them. What is untested is the choosing: thepeReferencesentry, theGetDependentSemanticVersionAsynccomparison, and the removal on project removal.That test needs a C# project inside the editor test workspace, and on this branch the test host refuses one:
TestHostWorkspaceServices.GetLanguageServicesraisesNotSupportedExceptionfor every language but F#, so a C# project has no compilation and no semantic version to ask for. The infrastructure that lifts it — language services per language,AddCSharpProject,CompileToAssembly— is in #20463 (Host C# projects in the editor test workspace), which is not inmainyet.So the options are to bring those two test-infrastructure commits into this PR and add the manager-level test here — same-version recompute keeps the reference's stamp, an edit to the C# project changes it, removing the C# project drops the entry — or to add that test in a follow-up once #20463 lands, and keep this PR to the FCS-level proof plus the description's narrowed claim. @T-Gro, which do you prefer? I lean to the follow-up, to keep a perf change out of the business of duplicating test infrastructure across three open PRs, but I will do it here if you would rather see it land together.