Calculate Entity.PublicPath instead of storing - #20285
Merged
Merged
Conversation
…of storing it entity_pubpath duplicated data the entity already had: every construction site set it to exactly the enclosing compilation path plus the entity's own logical name, as the REVIEW comment on the field had noted. Storing it cost an option, a PubPath and a fresh string[] per entity — about 100 bytes each, 2.6 MB on a 489-reference project — and the pickled form was read back into a field nothing needed. PublicPath becomes a struct over the enclosing CompilationPath and the name, so Entity.PublicPath can produce one without allocating, and the field is gone. The struct carries custom equality over the mangled path and the name only: derived equality would also compare the enclosing path's ILScopeRef and SyntaxAccess, which is stricter than the flat string[] comparison it replaces. Accessors return voption so a struct payload does not force an allocation per lookup. The pickle format is unchanged: writers derive the flat path, readers consume the index and discard it, so the pubpath table no longer builds PublicPath values. Derivation is exact at all four construction sites, and the only writes to entity_cpath and entity_logical_name after construction are in Link, which copies both from one source entity, so a derived path cannot drift from a stored one. Retained memory after ParseAndCheckProject drops 0.12-2.62 MB per project (-0.18% to -0.84%) across the measurement suite. 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
|
T-Gro
self-requested a review
August 19, 2026 09:27
T-Gro
approved these changes
Aug 20, 2026
T-Gro
left a comment
Member
There was a problem hiding this comment.
🤖🕵️ LGTM ✅
Dimensions covered (expand)
- Invariant equivalence: computed
PublicPath⇔ former storedentity_pubpath - Custom equality / hashing semantics on the new struct
- Pickle format stability (read + write, both directions)
- Allocation trade-off (retained ↓ vs transient ↑)
- Self-host / build validation
T-Gro
reviewed
Aug 20, 2026
auduchinok
force-pushed
the
il-entityPubPath
branch
from
August 20, 2026 12:19
74bca10 to
ba4843b
Compare
pubPathEq ended in a call to PublicPath.Equals, and fslibRefEq in one to EqualsFullPath. A body ending in a struct member call is not inferred to make no critical tailcalls, and that inference propagates to callers: primEntityRefEq gained a .tail prefix, and from there so did tyconRefEq, tcrefAEquiv, HasHeadType and the TypeTesters predicates. A tail-prefixed call is never inlined, which cost 12% checking a two-project graph. Comparing the access paths in a plain function keeps the inference, and builds neither a MangledPath list nor a FullPath array. PublicPath no longer carries equality at all. Its only comparison was an implicit = on a PublicPath voption in Exprs.fs, which boxed both sides; that now calls pubPathEq, so structural equality on a PublicPath is a compile error rather than a silently boxing comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSWUAYfpj82BHvzpkUCy8Z
T-Gro
reviewed
Aug 21, 2026
T-Gro
approved these changes
Aug 21, 2026
T-Gro
enabled auto-merge (squash)
August 21, 2026 11:30
Contributor
|
🔍 Tooling Safety Check — Affects-Bootstrap, Affects-Compiler-Output
|
auto-merge was automatically disabled
August 26, 2026 11:00
Head branch was pushed to by a user without write access
T-Gro
approved these changes
Aug 26, 2026
This was referenced Aug 26, 2026
nojaf
pushed a commit
to nojaf/fsharp
that referenced
this pull request
Aug 26, 2026
* IL: derive an entity's public path from its compilation path instead of storing it entity_pubpath duplicated data the entity already had: every construction site set it to exactly the enclosing compilation path plus the entity's own logical name, as the REVIEW comment on the field had noted. Storing it cost an option, a PubPath and a fresh string[] per entity — about 100 bytes each, 2.6 MB on a 489-reference project — and the pickled form was read back into a field nothing needed. PublicPath becomes a struct over the enclosing CompilationPath and the name, so Entity.PublicPath can produce one without allocating, and the field is gone. The struct carries custom equality over the mangled path and the name only: derived equality would also compare the enclosing path's ILScopeRef and SyntaxAccess, which is stricter than the flat string[] comparison it replaces. Accessors return voption so a struct payload does not force an allocation per lookup. The pickle format is unchanged: writers derive the flat path, readers consume the index and discard it, so the pubpath table no longer builds PublicPath values. Derivation is exact at all four construction sites, and the only writes to entity_cpath and entity_logical_name after construction are in Link, which copies both from one source entity, so a derived path cannot drift from a stored one. Retained memory after ParseAndCheckProject drops 0.12-2.62 MB per project (-0.18% to -0.84%) across the measurement suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Release notes * Release notes * Review * Compare public paths with a function, not a struct member pubPathEq ended in a call to PublicPath.Equals, and fslibRefEq in one to EqualsFullPath. A body ending in a struct member call is not inferred to make no critical tailcalls, and that inference propagates to callers: primEntityRefEq gained a .tail prefix, and from there so did tyconRefEq, tcrefAEquiv, HasHeadType and the TypeTesters predicates. A tail-prefixed call is never inlined, which cost 12% checking a two-project graph. Comparing the access paths in a plain function keeps the inference, and builds neither a MangledPath list nor a FullPath array. PublicPath no longer carries equality at all. Its only comparison was an implicit = on a PublicPath voption in Exprs.fs, which boxed both sides; that now calls pubPathEq, so structural equality on a PublicPath is a compile error rather than a silently boxing comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSWUAYfpj82BHvzpkUCy8Z * Fantomas --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tomas Grosup <Tomas.Grosup@gmail.com>
T-Gro
pushed a commit
that referenced
this pull request
Aug 27, 2026
fslibRefEq compared nlr1.Path against path2.FullPath, but PublicPath.FullPath (after #20285 derives paths instead of storing them) builds a MangledPath list plus a string[] on every call. This runs in the fslib-compile entity/val-ref equality hot path. Walk the enclosing AccessPath as a list while indexing nlr1.Path as an array instead: same O(N), zero allocation, mirroring the pubPathEq treatment #20285 already applied to its sibling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
T-Gro
added a commit
that referenced
this pull request
Aug 27, 2026
* Rotate [<return: X>] attributes during binding normalization Since #19738 the parser moved attributes written as [<return: X>] in front of a binding out of SynBinding.attributes and into SynValInfo.retInfo. The untyped tree then reported no attributes for source that visibly has one, which every consumer of the parse tree sees: formatters, analyzers, source generators and refactoring tools. The move was also lossy. The attribute list range narrowed from the [< >] span to the attribute alone, and all return attributes were collected into a single synthesized list, so [<return: A; return: B>] and [<return: A>][<return: B>] produced identical trees. Neither can be printed back to its original form. Do the rotation in BindingNormalization.NormalizeBinding instead, the single funnel from SynBinding to NormalizedBinding and already a lowering step. Everything downstream still reads retInfo as the single source of truth, so the fixes for #17904 and #19020 are unchanged, but the parse tree again says what was written. Add parse baselines for both the common [<return: Struct>] partial active pattern and for attribute grouping, neither of which had any coverage before. * Add release note * Calculate Entity.PublicPath instead of storing (#20285) * IL: derive an entity's public path from its compilation path instead of storing it entity_pubpath duplicated data the entity already had: every construction site set it to exactly the enclosing compilation path plus the entity's own logical name, as the REVIEW comment on the field had noted. Storing it cost an option, a PubPath and a fresh string[] per entity — about 100 bytes each, 2.6 MB on a 489-reference project — and the pickled form was read back into a field nothing needed. PublicPath becomes a struct over the enclosing CompilationPath and the name, so Entity.PublicPath can produce one without allocating, and the field is gone. The struct carries custom equality over the mangled path and the name only: derived equality would also compare the enclosing path's ILScopeRef and SyntaxAccess, which is stricter than the flat string[] comparison it replaces. Accessors return voption so a struct payload does not force an allocation per lookup. The pickle format is unchanged: writers derive the flat path, readers consume the index and discard it, so the pubpath table no longer builds PublicPath values. Derivation is exact at all four construction sites, and the only writes to entity_cpath and entity_logical_name after construction are in Link, which copies both from one source entity, so a derived path cannot drift from a stored one. Retained memory after ParseAndCheckProject drops 0.12-2.62 MB per project (-0.18% to -0.84%) across the measurement suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Release notes * Release notes * Review * Compare public paths with a function, not a struct member pubPathEq ended in a call to PublicPath.Equals, and fslibRefEq in one to EqualsFullPath. A body ending in a struct member call is not inferred to make no critical tailcalls, and that inference propagates to callers: primEntityRefEq gained a .tail prefix, and from there so did tyconRefEq, tcrefAEquiv, HasHeadType and the TypeTesters predicates. A tail-prefixed call is never inlined, which cost 12% checking a two-project graph. Comparing the access paths in a plain function keeps the inference, and builds neither a MangledPath list nor a FullPath array. PublicPath no longer carries equality at all. Its only comparison was an implicit = on a PublicPath voption in Exprs.fs, which boxed both sides; that now calls pubPathEq, so structural equality on a PublicPath is a compile error rather than a silently boxing comparison. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSWUAYfpj82BHvzpkUCy8Z * Fantomas --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tomas Grosup <Tomas.Grosup@gmail.com> * Cleanup: use `FindByNameAndArity` guard in `CheckILBaseCall` to avoid exception-based control flow for inherited IL methods (#20272) * Fix inherited IL base-call crash and add release notes * Address review: inherit the IL method in the 20264 test, resolve via FindByNameAndArity, and keep a try/with around signature matching. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * PR review fix * Clarify IL base-call check notes, split #20264 test Release notes now specify skipping `resolveILMethodRefWithRescope` only when the method isn't on the immediate IL type, not via exception flow. The `SimpleInteropTests.fs` test for #20264 is split: one test checks inherited non-abstract IL base method calls on generics succeed, the other checks abstract base method calls on generics fail with FS1201. Test names and code are now clearer and more targeted. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Reject bitwise operators on char-backed enums (#11785) (#20322) * Remove always-on IndexerNotationWithoutDot language feature flag (#20319) * Flatten always-on IndexerNotationWithoutDot: remove flag and collapse enforcement sites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove orphaned IndexerNotationWithoutDot diagnostic strings and regenerate xlf Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Sync xlf files: remove trans-units for FSComp keys deleted in this PR --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: T-Gro <t-gro@users.noreply.github.com> * Replace the stringified pattern-match memo key with a typed one (#20337) * Replace the stringified pattern-match memo key with a typed one The memo added in #20244 keyed residual match states by concatenating paths, pattern node ids and bound expressions into a string, then compared those strings. Replace it with structural keys: - PathKey / BoundExprKey / FrontierKey / MemoKey instead of string concatenation, so equality is structural rather than textual. - Record field and union case keys now carry the declaring tycon stamp, so same-named fields of different types can no longer fuse. - MemoEntry replaces the (int ref * Lazy<bool> * Lazy<_>) tuple. - The diagnostics/codegen distinction is a JoinPromotion argument rather than being inferred from warnOnIncomplete, which happened to coincide. Behaviour preserving: emitted assembly size and full IL fingerprint are identical to main for the issue #18425 repro at N=8..40. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Add release note Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Keep the memo machinery private and drop a redundant pass CompilePatternBasic is only called from CompilePattern in this file and is not in the signature file, so it and JoinPromotion can be private like the key types already are. Also name the pattern node id in FrontierKey (matching the existing ClauseNumber alias) and build the bound-expression key list in one pass instead of Map.toList followed by List.map. Map enumerates in ascending key order, so the key is unchanged; verified IL-identical. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Document the key types State the safety invariant on the path key directly (equal keys imply pathEq, so being finer only costs memo misses), and give FrontierKey and MemoEntry the same brief purpose comments the neighbouring types have. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Record what the size guard actually catches Red-green checked by disabling promotion: the N=32 input then runs for 383s and is OOM-killed rather than emitting something slightly over the bound, so the exact constant does not matter. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Drop the release note This is an internal refactoring with no observable behaviour change, and the latent over-fusion it fixes was introduced by #20244 in this same release, so no shipped compiler could exhibit it. Labelled NO_RELEASE_NOTES. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb * Add postmortem --------- Co-authored-by: Eugene Auduchinok <eugene.auduchinok@jetbrains.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tomas Grosup <Tomas.Grosup@gmail.com> Co-authored-by: Andrii Chebukin <XperiAndri@Outlook.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Edgar Gonzalez <edgargonzalez.info@gmail.com> Co-authored-by: Copilot <copilot@github.com> Co-authored-by: T-Gro <t-gro@users.noreply.github.com> Copilot-Session: 86c47998-fe39-4b68-8b80-c72b72fc9efb
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.
PublicPathbecomes a struct over the enclosingCompilationPathand the name, soEntity.PublicPathderives one without allocating and the field goes away. Two consequences:
ILScopeRefandSyntaxAccess— stricter than the flatstring[]comparison it replaces. It compares the mangled pathand name only, so
pubPathEqkeeps its meaning;EqualsFullPathcompares against the flat form withoutbuilding it.
voptionaccessors, so a struct payload does not allocate per lookup.Pickle format unchanged: writers derive the flat path, readers consume the index and discard it.
One project held open (mean of 3 fresh processes)
A whole solution held open at once (mean of 2 passes)
The saving is per entity, so it multiplies with the number of projects each importing their own copy.