IL: fix the per-reader string cache sizing - #20261
Open
auduchinok wants to merge 1 commit into
Open
Conversation
ILMetadataReader keeps two string tables per referenced assembly. Both were sized from something unrelated to how many strings are actually read, and the second one had nothing to cache. cacheStringHeap was sized stringsStreamSize / 50 + 1, i.e. from the length of the #Strings stream. Only a small fraction of a #Strings heap is ever read, so the table sat around 11% full: one nearly-empty table per reference. It is now sized to grow. memoizeString had a single caller, the ns + "." + name concatenation in readBlobHeapAsTypeName. Every caller of that function is already cached or one-shot per row (typeDefReader, seekReadTypeDefAsTypeRefUncached, seekReadTypeRefUncached, and the exported-type readers), so the concatenation happens about once per typedef, typeref or exported-type row, and the table could only pay when two different rows produced identical text: the same name under a different resolution scope, or a type forwarder. Measured within-assembly retained string duplication is 0.00 MB, so it collapsed nothing, while holding every namespaced type name alive for the reader's lifetime as both key and value. Removed. Retained memory after ParseAndCheckProject drops 1.5-10.1 MB per project (-1.4% to -6.4%) across ten projects, and total allocation drops 1-34 MB. The saving scales with the number of referenced assemblies rather than project size, since the cost was two tables per reader, so the smallest subject gains most in relative terms and the one with 489 references gains most in absolute terms. Analysis time is unchanged within measurement noise. Tables.memoize still has a caller in ilmorph.fs, so it stays. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
T-Gro
reviewed
Aug 17, 2026
T-Gro
left a comment
Member
There was a problem hiding this comment.
Review — LGTM
Careful, measurement-backed memory work. I traced the change end to end and it holds up.
Correctness
- Removing
memoizeStringis provably safe.ILTypeRefhashes and compares by value (ILTypeRef.ComputeHashuseshash name;Equalsusesx.Name = y.Name), so the identity change from dropping interning cannot affect any dictionary key, lookup, or equality anywhere downstream. The only observable effect is that a type read once asILTypeDef.Nameand once asILTypeRef.Namebecomes two equal-but-distinct instances — exactly the ~0.24 MB you quantified. - All callers of
readBlobHeapAsTypeNamecheck out.typeDefReader/seekReadTypeDefAsTypeRefUncached/seekReadTypeRefUncachedall sit behind caches, and the exported-type readers (seekReadNestedExportedTypes,seekReadTopExportedTypes) are one-shot per row inside a lazy. So the intern table could only ever pay on cross-row duplicates, which your data shows are dominated by the single structural pair. The removed field and its only assignment are both gone, and no reference survives anywhere in the tree. cacheStringHeapsizing →0is fine.mkCacheGeneric falsekeeps the cache alive (the "lots of cache hits, never optimize away" invariant is preserved); only the initialConcurrentDictionarycapacity changes, and 0 is valid and simply grows. Renaming the now-unusedstringsStreamSizeto_stringsStreamSizeis correct —stringsStreamPhysicalLocremains the only field consumed from that stream.
Nice properties
- The saving scales with reference count, not project size, which is why the write-up's smallest subject wins relatively and the 489-reference
FSharp.Commonwins absolutely. That framing matches the code: it's two tables per reader. - Net negative diff, no new abstractions, and the trade-off (spend 0.24 MB of duplicate names to reclaim ~3.0 MB of fixed-capacity buckets) is clearly the right direction.
One nit (non-blocking)
- The release-note entry is missing the trailing PR link that every sibling line carries:
* IL: size the string heap cache to grow and remove the type name intern table ([PR #20261](https://github.com/dotnet/fsharp/pull/20261))
Great, well-justified change.
T-Gro
self-requested a review
August 17, 2026 08:28
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.
ILMetadataReaderkeeps two string tables per referenced assembly. Both were sized from somethingunrelated to how many strings are actually read.
cacheStringHeapwas sizedstringsStreamSize / 50 + 1, i.e. from the length of the#Stringsstream.Only a small fraction of a
#Stringsheap is ever read, so the table sat around 11% full — onenearly-empty table per reference. It is now sized to grow.
memoizeStringinterned one computed string: thens + "." + nameconcatenation inreadBlobHeapAsTypeName. Every caller of that function is already cached or one-shot per row(
typeDefReader,seekReadTypeDefAsTypeRefUncached,seekReadTypeRefUncached, and the exported-typereaders), so the table could only pay when two different rows produce identical text. It went through
Tables.memoize, whose fixed 1000-entry capacity cost about 3.0 MB in buckets across a large referenceset while collapsing at most 0.24 MB of duplicate names — so the table is removed rather than resized.
Tables.memoizekeeps itsilmorph.fscaller.Retained memory after
ParseAndCheckProject, mean of 3 runs in fresh processes per project:The saving is two tables per reader, so it scales with the number of referenced assemblies rather than
project size: the smallest subject gains most in relative terms, and FSharp.Common, with 489 references,
most in absolute terms. Total allocation drops 1-34 MB per project. Analysis time is unchanged within
measurement noise.
Without the intern table, ~2,077 duplicate name strings survive per FSharp.Common analysis (~0.24 MB,
an upper bound). 98% are one structural pair: the same type def read once as
ILTypeDef.Nameand onceas
ILTypeRef.Name. That is the 0.24 MB the table used to collapse, and it is well below what the tableitself cost.