Skip to content

[MicroPerf] Avoid Choice allocation in fslib entity/val-ref equality - #20350

Merged
T-Gro merged 3 commits into
mainfrom
t-gro-fslib-refeq-alloc
Aug 28, 2026
Merged

[MicroPerf] Avoid Choice allocation in fslib entity/val-ref equality#20350
T-Gro merged 3 commits into
mainfrom
t-gro-fslib-refeq-alloc

Conversation

@T-Gro

@T-Gro T-Gro commented Aug 25, 2026

Copy link
Copy Markdown
Member

fslibEntityRefEq and fslibValRefEq compare a tuple of refs against the two-case total active patterns |ERefLocal|ERefNonLocal| / |VRefLocal|VRefNonLocal|, which return a heap-allocated Choice. These run for every entity/val-ref equality while compiling FSharp.Core, so the Choice allocation dominates. This replaces those matches with [<return: Struct>] inline active patterns that return a ValueOption (unboxed) and read the ref's local/non-local payload directly — same match shape, no heap allocation.

Measured compiling FSharp.Core with the built compiler (dotnet-trace --profile gc-verbose):

                                          main      PR
Choice2Of2[Entity, NonLocalEntityRef]     746 MB     0
fslibEntityRefEq                          380 MB     0
|VRefLocal|VRefNonLocal|                  123 MB     0
|ERefLocal|ERefNonLocal|                   95 MB     0
total allocated                          7985 MB   6955 MB

The emitted FSharp.Core.dll is byte-for-byte identical to main (deterministic build, SHA-256 unchanged) — ref equality feeds optimization and codegen, so this preserves output exactly.

@T-Gro
T-Gro requested a review from a team as a code owner August 25, 2026 15:11
@T-Gro T-Gro added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Aug 25, 2026
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Aug 25, 2026
@baronfel

Copy link
Copy Markdown
Member

Out of curiosity, would making the active patterns here ValueChoice instead have resulted in the same allocation savings? In my ideal world, we could use the nice AP features without paying the costs of them!

@T-Gro
T-Gro marked this pull request as draft August 26, 2026 08:00
@T-Gro
T-Gro force-pushed the t-gro-fslib-refeq-alloc branch from 4c26b20 to 88360f6 Compare August 26, 2026 08:13
@T-Gro

T-Gro commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

@baronfel : We don't have any ValueChoice , but you are right - we probably should!?

EDIT:
I did change it to the ValueOption variant of APs, but that drops exhaustiveness in case of multiple cases.
A value variant for multiple cases aka ValueChoice is still missing.

@T-Gro

T-Gro commented Aug 26, 2026

Copy link
Copy Markdown
Member Author

The problem is the structs might get really big with more cases.
Also, a generic ValueChoice might drop a big optimization potential - for APs where most of the cases (or all) do not carry any data, we might just used a single .Tag integer.

@T-Gro
T-Gro force-pushed the t-gro-fslib-refeq-alloc branch 2 times, most recently from 5ceec7c to d5b5f53 Compare August 26, 2026 08:32
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@T-Gro
T-Gro force-pushed the t-gro-fslib-refeq-alloc branch from d5b5f53 to 37ed1a9 Compare August 26, 2026 08:38
@T-Gro
T-Gro marked this pull request as ready for review August 26, 2026 08:43
@T-Gro
T-Gro requested a review from abonie August 26, 2026 08:43
@T-Gro
T-Gro enabled auto-merge (squash) August 26, 2026 08:45
@T-Gro T-Gro changed the title Avoid Choice allocation in fslib entity/val-ref equality [MicroPerf] Avoid Choice allocation in fslib entity/val-ref equality Aug 27, 2026
perf-bundle and others added 2 commits August 27, 2026 10:30
# Conflicts:
#	src/Compiler/TypedTree/TypedTreeBasics.fs
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes Theme-Performance

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants