Skip to content

IL: share ILCallingConv instances - #20254

Open
auduchinok wants to merge 7 commits into
dotnet:mainfrom
auduchinok:il-callingConv
Open

IL: share ILCallingConv instances#20254
auduchinok wants to merge 7 commits into
dotnet:mainfrom
auduchinok:il-callingConv

Conversation

@auduchinok

Copy link
Copy Markdown
Member

This adds ILCallingConv.Create, backed by a table of all 18 combinations, and uses it from the reader.

In a heap snapshot of ParseAndCheckProject over a 57-file project with 489 references, ILCallingConv went from 60,239 live objects (1.84 MB) to 18.

Retained memory after ParseAndCheckProject:

Project Before (MB) After (MB) Diff
consoleapp 33.84 33.65 −0.19 (−0.55%)
Fantomas.Core 109.37 109.25 −0.12 (−0.11%)
Fantomas.Core.Tests 141.89 141.60 −0.29 (−0.20%)
Fantomas.Benchmarks 64.81 64.66 −0.15 (−0.24%)
FSharp.Common 297.61 296.38 −1.23 (−0.41%)
FCS 1271.39 1270.38 −1.01 (−0.08%)

@github-actions

github-actions Bot 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 path Release notes path Description
`src/Compiler` docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

auduchinok and others added 5 commits August 13, 2026 13:28
There are only 18 combinations of ILThisConvention and ILArgConvention.
Pre-create all of them and hand out the shared instance from
ILCallingConv.Create, so reading a method signature no longer allocates
a Callconv. The two existing statics (Instance, Static) are placed in
the table too, so every use shares one instance per combination.

The index is computed with explicit matches rather than by casting the
union tags, so adding a case to either union is a compile error here
instead of a bad index.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Aug 13, 2026
@github-actions github-actions Bot added the ⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen label Aug 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Tooling Safety Check — Affects-Compiler-Output
Affects-Compiler-Output: modifies ilwrite.fs/ilreflect.fs IL emission paths

Generated by PR Tooling Safety Check · opus46 3.2M ·

@T-Gro T-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.

Really nice, self-contained win — 60k live ILCallingConv objects collapsing to 18 for effectively free is exactly the allocation the metadata reader should never have been making. The semantics are preserved end to end: structural equality means the shared instances compare identically to freshly-allocated ones, the pickle byte format is untouched (p_ILCallConv still writes ThisConv/BasicConv), and every out-of-module Callconv(...) construction and match was migrated to Create / member access, so sealing the case as private is airtight. I double-checked there are no stragglers — the only Callconv(...) sites left are inside il.fs, and the ProvidedTypes.fs / absil.fsi hits are unrelated self-contained copies.

Two tiny things before merge:

  1. The release-note link is missing its closing paren: .../pull/20254) needs a second ) to close the wrapping (, otherwise it renders as literal text unlike the sibling entries.

  2. The comment on Get says adding a union case is "a compile error here rather than a bad index." That holds for the match completeness, but thisIdx * 6 + argIdx is a separate magic number: if a new ILArgConvention case is added, the incomplete-match error forces an update to argIdx, yet nothing forces the 6 to track argConvs.Length — so the table build (i % argConvs.Length) and the lookup could silently disagree. Deriving the stride from one shared let would make the safety net actually match the comment.

Neither blocks — the change is correct as-is. A one-liner asserting obj.ReferenceEquals(ILCallingConv.Create(ILThisConvention.Instance, ILArgConvention.Default), ILCallingConv.Instance) would lock the sharing in against future regressions, but that's optional.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 17, 2026
@T-Gro
T-Gro self-requested a review August 17, 2026 08:27
@auduchinok

Copy link
Copy Markdown
Member Author

This is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ Affects-Compiler-Output Tooling check: PR touches IL emission or codegen AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants