Perf: async project invalidation + debounce script updateProjectOptions - #20126
Perf: async project invalidation + debounce script updateProjectOptions#20126xperiandri wants to merge 2 commits into
updateProjectOptions#20126Conversation
updateProjectOptions
…1 resumable-code composition error; async invalidation via cancellableTask and stable emitCache for C# PE references
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Focused on the two perf changes. The async/debounce conversions look correct; the main concerns are around the new emitCache lifetime and a couple of concurrency/disposal details noted inline.
| // 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>>() |
There was a problem hiding this comment.
Unbounded memory growth (regression vs. the old ConditionalWeakTable). The inner ConcurrentDictionary<VersionStamp, FSharpReferencedProject> is only ever cleared when the whole project is removed (emitCache.TryRemove(projectId)) or on ClearAllCaches. There is no per-version eviction, so every distinct dependent version of a referenced C# project produced during a session adds a new entry that is retained for the lifetime of the reactor — and each entry strongly holds an FSharpReferencedProject whose DelayedILModuleReader can pin emitted metadata bytes. The original weakPEReferences (ConditionalWeakTable<Compilation, _>) was self-cleaning: entries died when the Compilation was GC'd. This change trades a churn problem for an unbounded leak while a C# project is actively edited. Consider bounding this (e.g. keep only the latest stamp per project, or an LRU of size 1–2), since only the most recent version is normally useful.
| | _ -> | ||
| // Initialize for this project | ||
| let versionCache = ConcurrentDictionary<VersionStamp, FSharpReferencedProject>() | ||
| emitCache.[projectId] <- versionCache |
There was a problem hiding this comment.
Race on cache initialization. emitCache.[projectId] <- versionCache unconditionally overwrites. createPEReference runs inside cancellableTask flows that can execute concurrently, so two threads racing on the same (or interleaved) projectId can each create a fresh versionCache and clobber the other's — losing already-inserted references and creating divergent caches. Use emitCache.GetOrAdd(projectId, fun _ -> ConcurrentDictionary<_,_>()) and then operate on the returned instance, so the whole method has a single get-or-add path rather than two duplicated branches.
| | null -> () | ||
| | previousCts -> | ||
| previousCts.Cancel() | ||
| previousCts.Dispose() |
There was a problem hiding this comment.
Final CancellationTokenSource is never disposed. Each updateProjectOptions call disposes the previous CTS when swapping, but the last CTS stored in debounceCts (after the trailing caret move / on teardown) is never cancelled or disposed, leaking a CancellationTokenSource (and its registrations) per script view. Consider disposing debounceCts when the subscription/view is torn down. Also note the delayed async swallows the TaskCanceledException from Task.Delay via Async.Start's cancellation handling, which is fine — but relying on that is worth a brief comment.
Summary
Two perf fixes in
FSharpProjectOptionsManager.fs:.ResultblocksMailboxProcessorthread inFSharpProjectOptionsManager#20125 —.Resultblocking calls inhasDependentVersionChanged/isProjectInvalidatedwere blocking the single-threadedMailboxProcessorloop. Converted to asynccancellableTaskflow.updateProjectOptions(fires on every caret move) #20124 — scriptupdateProjectOptionsfired on every caret move. Added a 500ms debounce using a swappedCancellationTokenSourceandTask.Delay.Testing
Built successfully; deployed and validated in RoslynDev hive with CPU profiling.