Repository navigation
perf(symbols): share symbols per definition, cache queries - #20845
MangelMaxime wants to merge 6 commits into
Conversation
✅ Release notes checked
|
| let memberSymbols = ConcurrentDictionary<Stamp, obj>() | ||
| let entitySymbols = ConcurrentDictionary<Stamp, obj>() |
There was a problem hiding this comment.
There are many symbol environments created (and also kept in IDE scenarios). How much can this add to the retained memory?
There was a problem hiding this comment.
It seems to be very little on the envs an IDE keeps. The member cache is only filled by the expression converter, and ImplementationFile / AssemblyContents build a fresh env per call, so on a file's TypeCheckInfo env it stays empty.
The entity cache fills through DeclaringEntity, TypeDefinition and AttributeType, bounded by the distinct non-generic entities the file touches, and the typed tree already keeps those entities alive.
Measured on FSharpPlus: every file checked with ParseAndCheckFileInProject and all 88 results held, then tooltip-style queries over all 90k symbol uses, twice:
| base | this PR | |
|---|---|---|
| heap after checking 88 files | 342.0 MB | 342.0 MB |
| growth after querying all symbol uses | +0.7 MB | +1.7 MB |
| cache entries | - | 0 members, 1897 entities (max 177 per file) |
About 1 MB for the whole project held open, and the second pass adds nothing. The dictionaries are now created lazily, so an env that never shares a symbol doesn't pay for them.
T-Gro
left a comment
There was a problem hiding this comment.
🤖 🕵️ AI review — verify independently.
| member _.TypeDefinition = | ||
| protect <| fun () -> | ||
| match stripTyparEqns ty with | ||
| | TType_app (tcref, [], _) -> FSharpEntity.Shared(cenv, tcref) |
There was a problem hiding this comment.
🤖 🕵️ [P2] Symbol inspection now throws on a missing transitive dependency — Shared's fallback eagerly reads Typars instead of returning the unresolved entity.
// Library exposes ModuleApi.missing : Dim1Dependency.Gone;
// its transitive dim1-Dependency.dll is unavailable to FCS.
let inspect (library: FSharp.Compiler.Symbols.FSharpAssembly) =
let api =
library.Contents.Entities
|> Seq.find (fun e -> e.DisplayName = "ModuleApi")
let value =
api.MembersFunctionsAndValues
|> Seq.find (fun v -> v.DisplayName = "missing")
value.FullType.TypeDefinition.IsUnresolved
// Base: true; PR: InvalidOperationException.
T-Gro
left a comment
There was a problem hiding this comment.
In general, not just this PR:
Can you pls make PR descriptions more smaller and clearer on what a PR fixes, and less narrating implementation details?
This repo should have a skill for PR descriptions.
(Problem statement + brief problem approach + numerical evidence in case of perf improvements ;; that's all that is needed.)
5f3057a to
97f34c4
Compare
|
Sorry, I missed the PR skill and wanted to give enough context for where the changes came from. The description is rewritten with it now, and the bot's finding is fixed with a regression test. |
Description
Consumers that walk the typed tree through the symbol API (Fable, analyzers, language servers) get a fresh
FSharpMemberOrFunctionOrValueat every call site and a freshFSharpEntityon everyDeclaringEntity,TypeDefinitionorAttributeTypeaccess, and each symbol recomputesCurriedParameterGroups,Attributesand the like on every call. In a Fable compile of its own test suite those queries were 17 % of all allocated bytes.GetHashCodeon a member also hashed itsLogicalName, so dictionaries keyed on members collide on every overload.Within one
SymbolEnv, symbols for the same definition are now the same object (generic entities excepted, since each use gets fresh type arguments), their expensive queries are cached, andGetHashCodeuses the definition stamp, consistent with the existingEquals. No public API change, andfscnever loads the symbol API.SymbolApiBenchmarks.WalkSymbols(added in this PR)tests/Js/Main, symbol API allocationstests/Js/Main, compile timeAcross seven real Fable projects compile time drops 0 to 4 % with byte-identical output.
Checklist
Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated: