Skip to content

IL: cache the ILTypeRef of a type def - #20259

Merged
T-Gro merged 4 commits into
dotnet:mainfrom
auduchinok:il-typeDefAsTypeRef
Aug 24, 2026
Merged

IL: cache the ILTypeRef of a type def#20259
T-Gro merged 4 commits into
dotnet:mainfrom
auduchinok:il-typeDefAsTypeRef

Conversation

@auduchinok

Copy link
Copy Markdown
Member

seekReadTypeDefAsTypeRef has no cache. Every call re-reads the TypeDef row, rebuilds the type name,
walks the entire enclosing chain for a nested type, and allocates a fresh ILTypeRef.

Attributing calls to readBlobHeapAsTypeName on a 57-file project with 489 references, 28,073 of 92,665
came through this path — 30%, second only to type-def realisation itself.

This routes it through a per-reader cache keyed on the TypeDef row index, like the adjacent
cacheTypeRef. The result depends only on that row, all of it immutable metadata.

Project Before (MB) After (MB) Diff
consoleapp 33.45 33.28 −0.17 (−0.52%)
Fantomas.Core 107.95 107.72 −0.23 (−0.21%)
Fantomas.Core.Tests 140.55 140.20 −0.35 (−0.25%)
Fantomas.Benchmarks 64.32 64.27 −0.06 (−0.09%)
FSharp.Common 263.81 261.74 −2.08 (−0.79%)
fcs 1230.46 1228.58 −1.88 (−0.15%)

@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

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

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

Verified locally: the cache is keyed on the TypeDef row index and the result depends only on that immutable row (name, namespace, and the enclosing chain, which itself now routes back through the cache for nested types), so this is safe to memoize. Leaving it ungated on reduceMemoryUsage matches cacheTypeRef — the ILTypeRef is retained by the returned structures regardless, so deduping lowers retained memory rather than trading it for speed. All call sites go through the wrapper, and ./Build.cmd -c Debug is clean (0 warnings).

One optional nit: the initial capacity is 0, whereas the sibling caches seed it (getNumRows TableNames.TypeDef / 20 + 1). A hint would save a few rehashes on the hot path, but it's immaterial to correctness.

LGTM.

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Aug 14, 2026
@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Aug 14, 2026
@T-Gro
T-Gro self-requested a review August 14, 2026 13:16
auduchinok and others added 3 commits August 19, 2026 14:04
seekReadTypeDefAsTypeRef had no cache, so every reference to a type def re-read
its row, rebuilt its name, walked the whole enclosing chain if nested, and
allocated a fresh ILTypeRef. Nothing upstream absorbed that: seekReadTypeDefAsType
is gated on reduceMemoryUsage and so is disabled in every long-running host.

30% of the calls to readBlobHeapAsTypeName came through this path on a 57-file
project with 489 references. Retained memory after ParseAndCheckProject drops
2.08 MB there and 1.88 MB on the compiler's own project.

Not gated on reduceMemoryUsage, like cacheTypeRef and cacheStringHeap: the cache
lowers retained memory rather than trading memory for speed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@auduchinok
auduchinok force-pushed the il-typeDefAsTypeRef branch from 6bf03ba to 87e0434 Compare August 19, 2026 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants