Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -128,73 +128,151 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
ConcurrentDictionary<DocumentId, Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription>()

// This is used to not constantly emit the same compilation.
// However, when C# projects churn, Roslyn creates new Compilation instances with the same project ID and version,
// which makes ConditionalWeakTable defeat the purpose. We use a nested ConcurrentDictionary keyed by ProjectId and VersionStamp
// to map to the FSharpReferencedProject, ensuring stable references across churns.
let emitCache = ConcurrentDictionary<ProjectId, ConcurrentDictionary<VersionStamp, FSharpReferencedProject>>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unbounded memory growth (potential leak). The inner versionCache is only pruned when the whole project is removed from the solution (emitCache.TryRemove(projectId)); individual VersionStamp entries are never evicted. Each entry strongly holds an FSharpReferencedProject whose DelayedILModuleReader retains the emitted metadata MemoryStream once realized (the code deliberately never disposes it). Under exactly the C#-churn scenario this PR targets, every edit yields a new stamp and appends a new entry, so this dictionary grows without bound for the life of the project. This replaces the previous GC-collectable ConditionalWeakTable<Compilation,_> (entries freed once the Compilation was collected) with a strongly-rooted cache. Consider keeping only the latest stamp per project (clear/replace on a new stamp) or bounding the cache size (LRU).

let weakPEReferences = ConditionalWeakTable<Compilation, FSharpReferencedProject>()
let lastSuccessfulCompilations = ConcurrentDictionary<ProjectId, Compilation>()

let scriptUpdatedEvent = Event<FSharpProjectOptions>()

let createPEReference (referencedProject: Project) (comp: Compilation) =
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.
ms.Dispose() // it failed, dispose of stream
None

let resultOpt =
match weakComp.TryGetTarget() with
| true, comp -> tryStream comp
| _ -> None

match resultOpt with
| Some _ -> resultOpt
let createPEReference (referencedProject: Project) (comp: Compilation) ct =
cancellableTask {
let projectId = referencedProject.Id
let! stamp = referencedProject.GetDependentVersionAsync(ct)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetDependentVersionAsync changes on any text edit to this project or to any project it transitively references, so the cache will miss — and trigger a fresh, expensive metadata Emit — far more often than needed, and (combined with the unbounded versionCache above) accumulates entries faster. For a metadata-only PE reference the meaningful key is the semantic version: GetDependentSemanticVersionAsync changes only when the referenced project's public surface changes, which is what actually invalidates the emitted metadata. Also, the PR description states the key is project.Version, which does not match this call — please reconcile description and implementation.


match emitCache.TryGetValue(projectId) with
| true, versionCache ->
match versionCache.TryGetValue(stamp) with
| true, fsRefProj -> return fsRefProj
| _ ->
match weakPEReferences.TryGetValue comp with
| true, fsRefProj -> return fsRefProj
| _ ->
match lastSuccessfulCompilations.TryGetValue(projectId) with
| true, comp -> tryStream comp
| _ -> None

let getStamp = fun () -> stamp

let fsRefProj =
FSharpReferencedProject.PEReference(getStamp, DelayedILModuleReader(referencedProject.OutputFilePath, getStream))

weakPEReferences.Add(comp, fsRefProj)
fsRefProj
let mutable strongComp = comp
let weakComp = WeakReference<Compilation>(comp)
let mutable stampTime = 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.
stampTime <- DateTime.UtcNow
ms.Dispose()
None
| _ ->
strongComp <- Unchecked.defaultof<_> // Stop strongly holding the compilation since we have a result.
ms.Dispose() // it failed, dispose of stream
None

let resultOpt =
match weakComp.TryGetTarget() with
| true, comp -> tryStream comp
| _ -> None

match resultOpt with
| Some _ -> resultOpt
| _ ->
match lastSuccessfulCompilations.TryGetValue(projectId) with
| true, comp -> tryStream comp
| _ -> None

let getStampTime = fun () -> stampTime

let fsRefProj =
FSharpReferencedProject.PEReference(getStampTime, DelayedILModuleReader(referencedProject.OutputFilePath, getStream))

weakPEReferences.Add(comp, fsRefProj)
versionCache.[stamp] <- fsRefProj
return fsRefProj
| _ ->

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first-seen initialization path races: two threads hitting a not-yet-cached projectId each construct a separate versionCache, and emitCache.[projectId] <- versionCache unconditionally overwrites, discarding the other thread's just-added entry and forcing a redundant Emit. Use let versionCache = emitCache.GetOrAdd(projectId, fun _ -> ConcurrentDictionary<_,_>()) and then follow a single code path. That also lets you delete the ~55 lines of getStream/tryStream logic duplicated verbatim between this branch (lines 218-274) and the branch above (lines 153-209); keeping two identical copies risks them silently diverging when one is later fixed and the other is missed.

// Initialize for this project
let versionCache = ConcurrentDictionary<VersionStamp, FSharpReferencedProject>()
emitCache.[projectId] <- versionCache

match weakPEReferences.TryGetValue comp with
| true, fsRefProj -> return fsRefProj
| _ ->
let mutable strongComp = comp
let weakComp = WeakReference<Compilation>(comp)
let mutable stampTime = 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.
stampTime <- DateTime.UtcNow
ms.Dispose()
None
| _ ->
strongComp <- Unchecked.defaultof<_> // Stop strongly holding the compilation since we have a result.
ms.Dispose() // it failed, dispose of stream
None

let resultOpt =
match weakComp.TryGetTarget() with
| true, comp -> tryStream comp
| _ -> None

match resultOpt with
| Some _ -> resultOpt
| _ ->
match lastSuccessfulCompilations.TryGetValue(projectId) with
| true, comp -> tryStream comp
| _ -> None

let getStampTime = fun () -> stampTime

let fsRefProj =
FSharpReferencedProject.PEReference(getStampTime, DelayedILModuleReader(referencedProject.OutputFilePath, getStream))

weakPEReferences.Add(comp, fsRefProj)
versionCache.[stamp] <- fsRefProj
return fsRefProj
}

let rec tryComputeOptionsBySingleScriptOrFile (document: Document) userOpName =
cancellableTask {
Expand Down Expand Up @@ -348,7 +426,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
)
elif referencedProject.SupportsCompilation then
let! comp = referencedProject.GetCompilationAsync(ct)
let peRef = createPEReference referencedProject comp
let! peRef = createPEReference referencedProject comp ct
referencedProjects.Add(peRef)

if canBail then
Expand Down Expand Up @@ -421,7 +499,8 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
lastSuccessfulCompilations.ToArray()
|> Array.iter (fun pair ->
if not (currentSolution.ContainsProject(pair.Key)) then
lastSuccessfulCompilations.TryRemove(pair.Key) |> ignore)
lastSuccessfulCompilations.TryRemove(pair.Key) |> ignore
emitCache.TryRemove(pair.Key) |> ignore)

checker.InvalidateConfiguration(projectOptions, userOpName = "tryComputeOptions")

Expand Down Expand Up @@ -510,6 +589,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
match cache.TryRemove(projectId) with
| true, (_, _, projectOptions) ->
lastSuccessfulCompilations.TryRemove(projectId) |> ignore
emitCache.TryRemove(projectId) |> ignore
checker.ClearCache([ projectOptions ])
| _ -> ()

Expand All @@ -518,6 +598,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
match singleFileCache.TryRemove(documentId) with
| true, (_, _, _, projectOptions, subscription) ->
lastSuccessfulCompilations.TryRemove(documentId.ProjectId) |> ignore
emitCache.TryRemove(documentId.ProjectId) |> ignore
checker.ClearCache([ projectOptions ])
subscription |> Option.iter (fun handler -> handler.Dispose())
| _ -> ()
Expand Down Expand Up @@ -555,6 +636,7 @@ type private FSharpProjectOptionsReactor(checker: FSharpChecker) =
cache.Clear()
singleFileCache.Clear()
lastSuccessfulCompilations.Clear()
emitCache.Clear()

member _.ScriptUpdated = scriptUpdatedEvent.Publish

Expand Down
Loading