Cache reference timestamps behind an `IVsAsyncFileChangeEx2` watcher for snapshot reuse by xperiandri · Pull Request #20457 · dotnet/fsharp

xperiandri · 2026-09-05T19:26:29Z

On large projects, every typecheck in Visual Studio checked the timestamp of each -r: reference on disk, hundreds of file-system calls per request. Reference timestamps are now cached and refreshed by VS file-change notifications, so a reference is checked again only when it changes.

References are watched only while the transparent compiler and snapshot reuse are both on. Not covered: building a snapshot from scratch (#20459) and the legacy incremental builder (#20761).

github-actions · 2026-09-05T19:26:41Z

✅ Release-note check exempted

The NO_RELEASE_NOTES label or localization-bot authorship exempts this pull request.

github-actions · 2026-09-05T20:31:41Z

🔍 Tooling Safety Check — Affects-Design-Time
Affects-Design-Time: adds FileChangeWatcher + FSharpReferenceChangeTracker to VS integration that execute at design time

Generated by PR Tooling Safety Check · opus46 4.5M · ◷

xperiandri

Fresh-eyes pass over the whole PR against the current Roslyn FileChangeWatcher / ReferenceFileChangeTracker and against what FCS actually does with references. The shape is right: batched advise/unadvise with the 500 ms window, directory watches for the reference roots, ref-counted per-file watches, free-threaded sink. Three things need fixing, and one framing point matters for the description.

Must fix (inline, with suggestions):

WatchFiles runs are coalesced across sinks and advised with the first one. Latent today, breaks as soon as a second context exists.
FSharpReferenceChangeTracker races Timer.Change against Timer.Dispose and can throw inside the VS file change callback; it also allocates a timer per unwatched path before checking whether anyone watches it.
cache.TryRemove in onWatchedReferenceChanged never reaches consumers (they go through ProjectCache.Projects) and causes a second InvalidateConfiguration at the next recompute, discarding a builder that may already be rebuilt. InvalidateConfiguration alone is the right call.

Framing. As it stands the PR does not change observable latency. On the legacy path getOrCreateBuilder (BackgroundCompiler.fs L492) evaluates IsReferencesInvalidated on every request, and that stats every reference with a fresh TimeStampCache (IncrementalBuild.fs L1245). On the transparent-compiler path the snapshot reuse compares ReferencesOnDisk by stat (WorkspaceExtensions.fs L250) and FromOptions stats on creation. Both notice a rebuilt dll at the next request anyway, and InvalidateConfiguration only swaps in a lazy builder node without computing anything. So "keeps serving options and snapshots computed against the old assembly" is not what happens today. The value of the watcher arrives when it replaces those stat loops: a reference-change notification for the incremental builder on the FCS side (the analogue of useChangeNotifications for sources) and a watcher-invalidated stamp cache for snapshots. Worth presenting this PR as that infrastructure and listing both follow-ups explicitly.

Also inline: diff-based watch updates instead of stop-all/start-all on every options recompute, NUGET_PACKAGES and whether the NuGet cache should be a directory watch at all (Roslyn does not do that), a shared no-op token, the drain loop, logging of swallowed exceptions, the design-review doc, and applyBatch test coverage. The Fixes # (issue, if applicable) placeholder is still in the description.

All suggestions were applied together on the branch and pass dotnet fantomas --check; the changed regions were type-checked and smoke-tested with stand-ins for the VS interop types (sink grouping, drain, debounce and the timer race under 8 concurrent producers, watch diffing).

xperiandri · 2026-09-05T23:05:19Z

Addressed the fresh-eyes review in d8dce7a / 704a0ad; each inline thread has a reply. The description is reframed as you suggested: the watcher is infrastructure, the current stat loops (IsReferencesInvalidated, ReferencesOnDisk) still notice a rebuilt dll on the next request, and the latency win arrives with the two follow-ups now listed explicitly (an FCS-side reference-change notification for the incremental builder, and a watcher-invalidated stamp cache for snapshots), plus script #r/#load watching. The Fixes # placeholder is gone.

xperiandri · 2026-09-06T01:04:01Z

Reworked in c15f47b after checking the premise in VS instead of assuming it.

Breakpoints on both invalidation paths — onWatchedReferenceChanged in the reactor and the isProjectInvalidated branch of tryComputeOptions — with a referenced project rebuilt from the command line: Roslyn's path fires first. ProjectSystemProjectFactory already advises every MetadataReference path, swaps the reference when the file changes and bumps Project.Version, and the reactor recomputes and calls InvalidateConfiguration off the back of that. The tracker gets there later by construction: Roslyn batches over 500 ms, this adds a 2 s debounce on top of the same window.

So the reference subscription is gone and FSharpProjectOptionsReactor is untouched by this PR. What remains is the transport plus its tests, and the description now says plainly that it has no consumer yet and lists the three the workspace genuinely does not cover: script #load sources, the ReferencesOnDisk stamp cache, and an FCS-side notification replacing IsReferencesInvalidated.

Happy to fold the first of those into this PR instead, if you would rather not take a transport on its own.

xperiandri · 2026-09-06T02:05:49Z

The transport now has its consumer, and it is the one that actually removes a stat loop: the snapshot-reuse guard in createProjectSnapshot no longer stats every -r: per new Project instance; it reads stamps that the watcher keeps inside its watch entries (dadf230f5a, 744355228a, 7153224f14, 15bd8ce03e).

The reactor subscription is back, but not for the reason it was removed in c15f47bd52: it registers the -r: set so the stamps exist, and passes no change handler. Invalidating the FCS build stays Roslyn's job, as measured earlier.

Soundness rule, since it is the one thing worth reading closely: a cached stamp lives in the watch entry and is dropped on the raw notification, so it can only be served while a notification can still reach it; anything unwatched is stat'd directly; a mismatch against the snapshot's own stamps drops the project's stamps, so a missed notification costs one re-stat pass, not a rebuild per Project instance.

Description and docs/ide/file-watching.md are updated accordingly; the three remaining stat sites (FromOptions, IsReferencesInvalidated, script #load) are listed as follow-ups.

xperiandri · 2026-09-06T04:06:29Z

CI: every Windows leg failed with FS1116/FS1118 at FileChangeWatcher.fs(31,12) (EndsWithOrdinal), and CheckCodeFormatting flagged WorkspaceExtensions.fs.

Cause: inline members of the internal module Internal.Utilities.Library cannot be inlined into another assembly, InternalsVisibleTo or not — the optimizer drops their optimization data at the assembly boundary, so a consumer compiled with --optimize+ (Release) fails with FS1118, while --optimize- (Debug) compiles and calls the compiled method instead. Reproduced with a two-file library: module internal Lib + [<InternalsVisibleTo>] + let inline f … gives FS1116/FS1118 in the consumer under --optimize+ with both the SDK and the bootstrap compiler; a public module works; --realsig and a signature file make no difference. FSharp.Editor on main uses none of those helpers, which is why it only surfaced here.

Fix: call String.StartsWith/EndsWith with an explicit StringComparison directly in FSharp.Editor (plus the fantomas fix); the illib StartsWithOrdinalIgnoreCase helper stays. Verified locally: FSharp.Editor builds in Release with that change.

A queued advise is not a subscription: `EnqueueWatchingFile` returns before the batched `AdviseDirChangeAsync`/`AdviseFileChangesAsync` call even runs, and `GetLastWriteTimeUtc` was caching the first stat it saw regardless of whether a notification could ever reach that path yet. A change landing in that window - or a permanently failed advise, e.g. a Reference Assemblies directory that does not exist on this machine - left a stale stamp with no way to invalidate it. `IFSharpWatchedFile` now reports `IsActive`, true only once its advise has actually succeeded: a per-file token via its `Cookie`, a directory via a `bool ref` the corresponding `WatchDir` op flips on success. The stamp cache in `FSharpReferenceChangeTracker.GetLastWriteTimeUtc` gates on it, so a pending or failed watch always stats directly, and the first cached read is guaranteed to happen no earlier than the moment a change could have been observed. This also fixes a real bug the review comment's wording pointed at: `applyBatch`'s single try/with wrapped the whole `while` loop, so one failing operation (that nonexistent Reference Assemblies directory, on most dev machines without the .NET Framework SDK) silently dropped every other operation queued in the same batch. Each case now catches its own failure and lets the rest of the batch proceed. Addresses dotnet#20457 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

T-Gro

🤖🕵️ If this fixes an issue or implements an RFC/suggestion, link it (Fixes #... when applicable). Otherwise, give a short management-level summary in simplified technical English: what user scenario improves and what this achieves.

Please apply this PR-description guidance. Remove the implementation inventory already visible in Files, but keep necessary scope, compatibility, and dependency caveats.

FSharpReferenceChangeTracker gets a public Dispose with the interface forwarding to it, the MailboxProcessor pattern, so the reactor disposes it and the agent without casts. The reactor's opens follow the System / FSharp.Compiler / Microsoft / Internal.Utilities grouping.

Inline members of the internal Internal.Utilities.Library module cannot be inlined into another assembly, InternalsVisibleTo or not: the optimizer drops their optimization data at the assembly boundary, so FSharp.Editor fails with FS1116/FS1118 under --optimize+ (every Windows Release leg of the CI). Debug compiled only because --optimize- never tries to inline them. Also formats WorkspaceExtensions.fs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

A queued advise is not a subscription: `EnqueueWatchingFile` returns before the batched `AdviseDirChangeAsync`/`AdviseFileChangesAsync` call even runs, and `GetLastWriteTimeUtc` was caching the first stat it saw regardless of whether a notification could ever reach that path yet. A change landing in that window - or a permanently failed advise, e.g. a Reference Assemblies directory that does not exist on this machine - left a stale stamp with no way to invalidate it. `IFSharpWatchedFile` now reports `IsActive`, true only once its advise has actually succeeded: a per-file token via its `Cookie`, a directory via a `bool ref` the corresponding `WatchDir` op flips on success. The stamp cache in `FSharpReferenceChangeTracker.GetLastWriteTimeUtc` gates on it, so a pending or failed watch always stats directly, and the first cached read is guaranteed to happen no earlier than the moment a change could have been observed. This also fixes a real bug the review comment's wording pointed at: `applyBatch`'s single try/with wrapped the whole `while` loop, so one failing operation (that nonexistent Reference Assemblies directory, on most dev machines without the .NET Framework SDK) silently dropped every other operation queued in the same batch. Each case now catches its own failure and lets the rest of the batch proceed. Addresses dotnet#20457 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

FSharp.Editor's own voption-returning extensions (Common/Extensions.fs) cover this; no reason to allocate an option here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…tive tests MockWatchedFile implements IDisposable (through IFSharpWatchedFile), so constructing it without `new` is FS0760, an error under the repo's warnings-as-errors. That one line failed FSharp.Editor.Tests on all Windows jobs of the last CI run; FSharp.Editor itself compiled fine in Release. The two watcher tests asserted IsActive against wall-clock timing: right after enqueueing (relying on the 100 ms batching window not having elapsed) and right after the second recorded call (the flag is set a few instructions after that call returns). Both could flip on a loaded agent. The first now holds the service back with a TaskCompletionSource, so the advise cannot run until the test releases it; both wait for activation with SpinWait.SpinUntil instead of reading the flag once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Linking the note added a copy of it instead of editing the first, and `main` has since opened 11.0.200 for SDK 11.0.200, leaving 11.0.100 shipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

A watch on one file asked only for size and time, so a reference that did not exist when the watch started - the output of a project not built yet - or one replaced by a rename rather than a write told no one, and the options kept the stamp they had. The callback filter already accepted both kinds, because a directory watch reports them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…r does The reactor stopped invalidating options on a reference change; the note describing the stamp cache is the one that ships. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

watchReferenceFiles runs on the options agent while ClearAllCaches runs on the solution-close thread. Both read, diff and rewrite a project's watched set, so interleaving could stop a path twice or record one that was never started, unbalancing the tracker's ref counts for other projects sharing the dll. The set is a plain Dictionary under one lock now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hing consumes GetLastWriteTimeUtc held the tracker's lock across File.GetLastWriteTimeUtc, so one cold reference on a slow share stalled every other reader, every change notification, every Invalidate and the watch bookkeeping that solution close runs. The stat now happens outside the lock: the entry and its generation are read under it, and the result is stored only if the entry is still the one that was read, its generation has not moved (a change notification and Invalidate both bump it) and its watch is still active. Two cold readers may stat the same path twice; both read the same quiet window. The per-path debounce timers and the onChanged callback go with it. The reactor passed `ignore`, and every consumer reads stamps through IReferenceStamps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

When one reference's time differed from the snapshot's, the snapshot-reuse guard invalidated the stamp of every reference of the project, and the next snapshot stat'd all of them again. ReferenceStamps.changed walks both lists once and drops the stamp of each path whose time differs. A different path or a different length is a different reference set: nothing cached is stale, so nothing is dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

ClearAllCaches runs on the solution-close thread while the agent can still be computing options for a project of the closing solution. That computation then put a cache entry and a set of reference watches back, and nothing was left to release them. PublicationGate is one lock and an epoch. Closing the solution moves the epoch and clears the caches and the watches in the same critical section; a computation reads the epoch when it starts and publishes - cache entry, single-file entry, watches - only if the epoch still stands. A project computation that is refused undoes its FCS side with ClearCache and answers as for a project that is not in the workspace; a refused single-file entry disposes its text view subscription. ClearAllCaches now also disposes the subscriptions of the single-file entries it drops, outside the lock: Dispose is a COM unadvise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Roslyn watches MetadataReferences itself and answers a changed file with a new reference at the same path. When the tracker missed its own notification for that file, the stamp it remembered outlived the swap, and the snapshot-reuse guard kept comparing equal. The reactor now invalidates the stamps of the references the new project added before it recomputes the project's options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The stamps have one reader, the snapshot-reuse guard, which runs only with the transparent compiler and snapshot reuse both on. With either off the reactor still kept a watch per reference per project and nothing ever read a stamp. It now watches only when both are on, and releases a project's watches when it computes options and finds them off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Roslyn watches only MetadataReferences, so a project reference's output dll is watched only here; a #load file is a Roslyn document and needs no watch of ours. The debounce is gone, and the note covers the stat outside the lock, the Roslyn-swap backstop, the publication epoch and when references are watched at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…hreads A full run of the editor tests found two of them unreliable. The mock watcher handed out its list of watched files while the reactor's agent was changing it on another thread, and the enumeration threw. The gate's exclusion test started its two sides with Task.Run and gave the first ten seconds to get going, which a saturated pool does not always grant. The mock now locks around its list, and the tests that need a second thread - the gate's exclusion, the two readers meeting inside a stat, the invalidation that must get through while a stat is blocked - start threads of their own. The one wait left on a pool-raised event, the workspace's project removal, gets a minute. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The reactor watches a project's references only while the transparent compiler and snapshot reuse are both on; the note described the watch as unconditional. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

The watches followed the snapshot-reuse setting only when a project's options were computed. Turning the setting off while the options stayed cached left every watch in place, with nothing reading its stamps, and turning it on left a project unwatched until its next recompute. A request that finds the options cached now looks at whether the project is watched as the setting says, and acts only if it is not: one lookup on the usual path. The setting is read before the gate is taken, since it is a lookup in the workspace's services. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

A request that refreshed a cached script, recomputed an invalidated project or followed a project reference called its own function again, and each call read the epoch anew. When the solution closed in between, the second call read the epoch of the closed solution's successor and published under it: a script entry, or a project's options and watches, for a solution that was gone. The epoch is now a parameter. The request reads it once, when it starts, and the refresh, the recompute, the referenced projects and the PE references they create all publish under that one. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

ClearAllCaches copied the entries and then cleared the dictionary. A document closing at the same moment took an entry out and was about to dispose its text view subscription when ClearAllCaches, which held a copy of it, disposed it too. The second unadvise throws, and an exception in the agent ends the reactor: every later request waits for a reply that does not come. An entry is now taken out with TryRemove, and whoever takes it out is the one that disposes it. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

A whole run of the editor tests on a loaded machine failed the test that waits for a directory-covered watch to become active, which the watcher does on its own batching thread after the batching delay. Ten seconds was too short there. Both waits on that thread - for the service's calls and for a watch to become active - now take a minute, which costs nothing when the test passes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

T-Gro

🤖🕵️ AI review — verify independently.

…p the unused illib helper Comments that restated names are gone; what stays is the contract and the few invariants a name cannot carry. The watcher tests share one tracked temp file and one service-watcher-context setup, the duplicate-dispose test waits for the queue with a sentinel watch instead of a sleep, and the setting-toggle tests request the project as the next request after an edit finds it. The illib StartsWithOrdinalIgnoreCase nobody calls goes with its release note. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

xperiandri · 2026-10-18T10:20:31Z

Addressed the NoBloat / CodeCompaction review in 14913d1611 (plus a merge of main before it, no rebase):

The PR now adds 23 comment lines (3 are license headers), down from 115. What remains is the contract of the file-watching interfaces and the few invariants a name cannot carry: nothing blocking inside the publication gate, a second unadvise ends the agent, the stat runs outside the tracker's lock. The rest is carried by names, for example invalidateStampsOfReplacedMetadataReferences and roslynBatchingDelay.
FileChangeWatcherTests.fs is 558 lines instead of 698. Tracked (temp file, tracker, stamps, stat counter) replaces the repeated temp-file setups, withContext the repeated service, watcher and context construction, assertNotCachedWhenDuringStat the bodies of the three in-flight tests, and assertChanged the five reference-list tests. OwnThread.start is shared with PublicationGateTests.
Dropped what the PR no longer needs: the unused StartsWithOrdinalIgnoreCase in illib (the editor cannot inline it) with its FCS release note, so the PR does not touch src/Compiler any more, and the stray blank line in the Visual Studio release note.

FSharp.Editor.Tests ran twice in full on this commit: 7241 tests each time, 4 failed (two type provider F1 help tests, the type provider signature help test and fsiShouldTriggerCompletionInFsxFile, which go through the legacy incremental builder and type providers).

A queued advise is not a subscription: `EnqueueWatchingFile` returns before the batched `AdviseDirChangeAsync`/`AdviseFileChangesAsync` call even runs, and `GetLastWriteTimeUtc` was caching the first stat it saw regardless of whether a notification could ever reach that path yet. A change landing in that window - or a permanently failed advise, e.g. a Reference Assemblies directory that does not exist on this machine - left a stale stamp with no way to invalidate it. `IFSharpWatchedFile` now reports `IsActive`, true only once its advise has actually succeeded: a per-file token via its `Cookie`, a directory via a `bool ref` the corresponding `WatchDir` op flips on success. The stamp cache in `FSharpReferenceChangeTracker.GetLastWriteTimeUtc` gates on it, so a pending or failed watch always stats directly, and the first cached read is guaranteed to happen no earlier than the moment a change could have been observed. This also fixes a real bug the review comment's wording pointed at: `applyBatch`'s single try/with wrapped the whole `while` loop, so one failing operation (that nonexistent Reference Assemblies directory, on most dev machines without the .NET Framework SDK) silently dropped every other operation queued in the same batch. Each case now catches its own failure and lets the rest of the batch proceed. Addresses dotnet#20457 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Picking each pull request's release note again after it was reworded or given its link left the earlier copies in docs/release-notes/.VisualStudio/18.vNext.md, which merges as a union. Each pull request keeps the bullet its own branch, or main, has now. - The copy without the PR link: dotnet#20409, dotnet#20457, dotnet#20460, dotnet#20482, dotnet#20519. - The second of two identical copies: dotnet#20274, dotnet#20455, dotnet#20468, dotnet#20470, dotnet#20482, dotnet#20499, dotnet#20503, dotnet#20534 and the Navigate To ordering bullet. - The wording a later one replaced: dotnet#20126, dotnet#20274, dotnet#20457, dotnet#20460. The bullets of main are each there once and in main's order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

xperiandri requested a review from a team as a code owner September 5, 2026 19:26

github-project-automation Bot added this to F# Compiler and Tooling Sep 5, 2026

github-project-automation Bot moved this to New in F# Compiler and Tooling Sep 5, 2026