Make Entity's adhoc members list lazy - #20286
Conversation
…tion cache on first use Every Entity carried two objects that most entities never use. TyconAugmentation.Create allocated an empty ResizeArray for tcaug_adhoc_list, one per entity, and nothing is ever added for an imported type: NewILTycon closes the augmentation and adds no ad-hoc members. There is a single Add site, so the list is now allocated there, behind AddAdhocMember, with AdhocMembers for the readers. Unpickling an empty list stores nothing rather than an empty ResizeArray. entity_il_repr_cache held a cache cell used only by IlxGen, so an analysis-only host allocated one per entity and never read it. CompiledReprCache now creates it on first request, leaving the member's signature unchanged so no caller sees null. Retained memory after ParseAndCheckProject drops 0.05-1.38 MB per project across the measurement suite, and analysis time is unchanged within noise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
T-Gro
left a comment
There was a problem hiding this comment.
The invariant that makes this safe reads cleanly: tcaug_adhoc_list is now always either null or non-empty — Create starts null, AddAdhocMember allocates on first write, unpickling an empty list stores null, and remapping maps null to null. Nothing downstream consumes the list except pickling (through AdhocMembers) and remapping; IlxGen sorts off tcaug_adhoc / MembersOfFSharpTyconSorted, never the list, so the lazily-allocated field has no hot-path reader to trip over.
Lazy CompiledReprCache is equally benign under concurrency: the field is a reference written atomically and cached was already non-atomic, so a racing first-touch costs at most one redundant, deterministic recompute — and the entities most likely to be shared across parallel-checking threads (imported ones) still allocate eagerly in u_entity_spec_data, so the window is tiny.
Built Debug clean, 0 warnings with nullness checking on. Measurement-backed and tightly scoped — lovely change.
One optional follow-up: u_entity_spec_data still does entity_il_repr_cache = newCache() for every unpickled entity, so imported types keep paying for a cache an analysis-only host never reads. Since CompiledReprCache now tolerates null, that site could be null too and reclaim the imported-entity share as well.
Every
Entitycarried two objects most entities never use.tcaug_adhoc_list—TyconAugmentation.Createallocated an emptyResizeArrayper entity, andexactly one site ever adds to it (
PublishValueDefnMaybeInclCompilerGenerated). Nothing is added for animported type:
NewILTyconcloses the augmentation and publishes no ad-hoc members. It is now allocated atthat single site behind
AddAdhocMember, withAdhocMembersfor the two readers, and unpickling an emptylist stores nothing.
entity_il_repr_cache— a cache cell onlyIlxGenreads. An analysis-only host allocates one perentity and never touches it.
CompiledReprCachenow creates it on first request, so the member'ssignature is unchanged and no caller can observe a null.
No FCS public API change; the internal record fields become nullable,
tcaug_adhoc_listbecomes mutable,and
TyconAugmentationgainsAddAdhocMember/AdhocMembers.One project held open (mean of 3 fresh processes)
A whole solution held open at once (mean of 2 passes)
What it removes
gcdump of one held FSharp.Common analysis:
cache<CompiledTypeRepr>List<bool * ValRef>1.32 MB of objects against 1.32 MB measured. The right-hand column is the argument for the change: 95% of
augmentations never receive an ad-hoc member and 89% of entities are never asked for a compiled
representation, because this host only analyses.