Skip to content

IL: cache C# extension methods per CCU - #20256

Merged
T-Gro merged 2 commits into
dotnet:mainfrom
auduchinok:il-csExtensions-cache
Aug 19, 2026
Merged

IL: cache C# extension methods per CCU#20256
T-Gro merged 2 commits into
dotnet:mainfrom
auduchinok:il-csExtensions-cache

Conversation

@auduchinok

@auduchinokauduchinok commented Aug 13, 2026

Copy link
Copy Markdown
Member

GetCSharpStyleIndexedExtensionMembersForTyconRef had no cache, so every open of a static class
rebuilt a MethInfo chain for each of its extension methods and re-imported their type parameters. On a
57-file project with 489 references that meant 98,986 MethInfos describing only 6,782 distinct
extension methods on 1,190 static classes.

ProjectBefore (MB)After (MB)Diff %
consoleapp33.8333.66−0.18 (−0.52%)
Fantomas.Core109.43108.46−0.96 (−0.88%)
Fantomas.Core.Tests141.86141.35−0.51 (−0.36%)
Fantomas.Benchmarks64.8464.89+0.05 (+0.07%)
FSharp.Common297.67265.87−31.80 (−10.68%)
FCS1271.081230.85−40.23 (−3.16%)

@github-actions

github-actionsBot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
`src/Compiler`docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actionsgithub-actionsBot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 13, 2026

@T-GroT-Gro left a comment

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.

Review: caching C# extension methods per CCU ✅

Really nice, well-scoped optimization. I built it locally (Build.cmd -c Debug, 0 warnings/0 errors) and ran Language.ExtensionMethodTests28/28 passing. The design is sound and the code comments do a great job explaining the why. Approving.

Why this is correct

  • Local vs imported split is the right call. Only imported classes are cached (if tcrefOfStaticClass.IsLocalRef then compute-uncached). A local static class is still gaining members while its own file is checked, so it must not be memoized — the comment nails this.
  • Cache lifetime is tied to CcuData. It lives and dies with the referenced assembly's CCU, so it's naturally invalidated when the project/TcImports is rebuilt. No stale-entry risk across incremental builds.
  • Shared MethInfos are safe.MethInfo for imported IL methods is effectively immutable, and framework CCUs (e.g. System.Linq.Enumerable, a huge source of these) are shared with a consistent TcGlobals via FrameworkImportsCache, so reusing them across name-resolution envs and parallel file checking is fine. ConcurrentDictionary + TryGetValue/TryAdd makes concurrent computation idempotent (double-compute at worst, never corruption).
  • Priority is deliberately not cached.NextExtensionMethodPriority() is still called per open and applied outside the cached shape, preserving the original ordering semantics. The None/Some (Some/None) mapping is faithfully reproduced via (TyconRef option * MethInfo).
  • Conservative on import failure. Marking importFailed and skipping the TryAdd (while still returning the partial shape) means a transient import failure won't be baked in permanently — same worst-case cost as before, no regression.
  • isApplicable is checked before touching the CCU/cache, so non-extension classes pay nothing extra.
  • All four CcuData construction sites are updated, and the .fsi field order matches the .fs — build confirms.

Minor / optional (non-blocking)

  • ConcurrentDictionary(1, 0) uses concurrencyLevel = 1, which serializes the (rare) writes on a single lock. Reads are still lock-free, and writes happen at most once per static class, so this is fine — just flagging that the default ctor would behave near-identically if you'd rather not hardcode.
  • Empty-but-applicable classes now skip NextExtensionMethodPriority() (the counter no longer advances for them). That's benign — priority is only observable when members exist — worth a mental note only.
  • Typing the field as obj + :?> on read is a pragmatic way around the MethInfo-declared-later ordering, and the .fsi comment explains it well. No change needed.

The FCS (−3.16%) and FSharp.Common (−10.68%) allocation wins are excellent for essentially zero downside.

@github-project-automationgithub-project-automationBot moved this from New to In Progress in F# Compiler and ToolingAug 13, 2026
@T-Gro
T-Gro self-requested a review August 13, 2026 16:10
@T-GroT-Gro added the AI-reviewed PR reviewed by AI review council label Aug 13, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewedPR reviewed by AI review councilAI-Tooling-Check-Scanned-CleanTooling check: diff analyzed, no interesting infrastructure files

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants

@auduchinok@T-Gro