Cut allocations in the VS project options caches and drop a boxing check - #20413
Merged
T-Gro merged 7 commits intoSep 7, 2026
Merged
Conversation
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
xperiandri
force-pushed
the
perf/allocation-and-nullness-cleanups
branch
from
September 2, 2026 10:21
e74989f to
83da275
Compare
T-Gro
approved these changes
Sep 4, 2026
T-Gro
left a comment
Member
There was a problem hiding this comment.
Three correct, low-risk allocation cleanups — the struct-tuple swap is safe under ConcurrentDictionary's node replacement and the nullable-field change is semantically identical. No open threads. I'm fine taking the small FSharpCheckerResults.fs change along with the VS cache work rather than splitting it. Thanks!
3 tasks
xperiandri
added a commit
to xperiandri/fsharp
that referenced
this pull request
Sep 5, 2026
`FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on dotnet#20413 and the discussion in dotnet#20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
xperiandri
added a commit
to xperiandri/fsharp
that referenced
this pull request
Sep 5, 2026
`FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on dotnet#20413 and the discussion in dotnet#20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit b19bedc)
`result` was declared non-nullable and initialised with `Unchecked.defaultof<_>`, so every read of the fast path had to `box` it just to test for null. Declaring it `ILModuleReader | null` lets the match narrow the type directly, dropping the boxes and the `Unchecked` initialiser.
…utPath` `IProjectSite.CompilationBinOutputPath` is probed for the `-o:` flag on every project site mapping and its result is immediately converted to a string or null. Add an `Array.tryPickV` next to the existing `chooseV` and return `string voption`, so neither the pick nor the result allocates.
The reactor's `commandLineOptions` and project options `cache` dictionaries, and the `AsyncReplyChannel` payloads of `TryGetOptionsByDocument`/ `TryGetOptionsByProject`, all carried reference tuples that were allocated on every cache write and every reply. They never escape the reactor, so make them `struct` tuples.
…tion` `Project.fs` implements `Microsoft.VisualStudio.FSharp.Editor.IProjectSite` in two more places (`CreateRunningProjectSite` and `CreateStaticProjectSite`) that the earlier `voption` change to `IProjectSite.CompilationBinOutputPath` missed, breaking the build with FS0001.
ILVerify flags FSharp.Compiler.IlxGen::HashRangeSorted with a StackUnexpected error in Release builds on this branch that does not match the checked-in baseline, even though the exact same error text is already present in that baseline as a known pre-existing issue. Isolating the change (including testing byte-identical origin/main source through the same local Release build) could not reproduce a difference tied to this commit's content, but the CI-observed mismatch persisted across a partial revert. The allocation savings here are marginal; not worth risking invalid IL in a shipped compiler DLL over, so this reverts to the original List-based HashRangeSorted and the original `@` concatenations in TypeDefBuilder.Close and GenTypeDef.
xperiandri
force-pushed
the
perf/allocation-and-nullness-cleanups
branch
from
September 5, 2026 21:01
83da275 to
ae11e8e
Compare
xperiandri
added a commit
to xperiandri/fsharp
that referenced
this pull request
Sep 7, 2026
`FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on dotnet#20413 and the discussion in dotnet#20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit b19bedc)
This was referenced Sep 8, 2026
xperiandri
added a commit
to xperiandri/fsharp
that referenced
this pull request
Sep 11, 2026
`FSharpProjectOptionsReactor.singleFileCache` stored each script or single-file entry as a 5-tuple `Project * VersionStamp * FSharpParsingOptions * FSharpProjectOptions * ConnectionPointSubscription`, destructured positionally at every consumer. It is now a private record, `SingleFileCacheEntry`, so the consumers name the fields they use and `addToCacheAndSubscribe` is a copy-and-update of the incoming entry. A reference record was chosen deliberately: a struct entry saves one heap object per cache write but, on .NET Framework where FSharp.Editor runs, copies 48 bytes out of the dictionary on every hit and forces `ConcurrentDictionary` to allocate a new node on update. The benchmark in the PR shows the reference record matching the reference tuple in time and allocations on both runtimes. Follows up on dotnet#20413 and the discussion in dotnet#20274. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit b19bedc)
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Three independent allocation cleanups, one per commit, no behaviour change.
DelayedILModuleReader.result— declare it nullable instead of boxingThe field was declared non-nullable and initialised with
Unchecked.defaultof<_>, so both the fast path and the inside-the-lock recheck had toboxit just to test for null. Typing itILModuleReader | nullletsmatchnarrow the type directly and drops the boxes.This is confined to the private field; the public
DelayedILModuleReadersurface is unchanged.Array.tryPickV+IProjectSite.CompilationBinOutputPathCompilationBinOutputPathis probed for the-o:flag on every project-site mapping and its result is immediately turned into a string ornull. Added anArray.tryPickVnext to the existingArray.chooseVand changed the (internal)IProjectSitemember tostring voption, so neither the pick nor the result allocates. Call sites move fromOption.toObjtoValueOption.toObj.IProjectSitehere isMicrosoft.VisualStudio.FSharp.Editor.IProjectSite, which isinternal; the identically-shapedMicrosoft.VisualStudio.FSharp.LanguageService.IProjectSiteused by the legacy project system and Salsa is untouched.Struct tuples for the project options caches
FSharpProjectOptionsReactor'scommandLineOptionsand project optionscachedictionaries, and theAsyncReplyChannelpayloads ofTryGetOptionsByDocument/TryGetOptionsByProject, all carried reference tuples allocated on every cache write and every reply. None of them escape the reactor, so they are nowstructtuples.Validation
FSharp.Compiler.ServiceandFSharp.Editorboth build clean inDebug.Update: this PR originally also included a small
IlxGen.fschange (HashRangeSortedand two@concatenations rewritten to avoid intermediate lists). It's been reverted (see the "Revert the IlxGen.fs list-creation micro-optimization" commit): CI'sILVerifyjob flaggedFSharp.Compiler.IlxGen::HashRangeSortedwith aStackUnexpectederror in Release builds that didn't match the checked-in baseline, even though the identical error text is already present in that baseline as a known, pre-existing issue. I could not pin down the exact mechanism locally (a byte-identicalorigin/mainIlxGen.fsreproduces the same baselined error through the same build path, but a partial revert didn't clear the CI mismatch either), and the allocation savings there were marginal — not worth the risk of shipping IL thatILVerifydisagrees with in a shipped compiler DLL. Happy to revisit that piece separately with more thorough (bootstrap-build-equivalent) local verification.