Skip to content

Stabilize parallel optimizer Val iteration for deterministic synthetic names #19810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
T-Gro wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Stabilize parallel optimizer Val iteration for deterministic synthetic names #19810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/release-notes/.FSharp.Compiler.Service/11.0.100.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
### Fixed

* Improve determinism under parallel optimization: `Optimize/DetupleArgs` and `Optimize/InnerLambdasToTopLevelFuncs` now walk their `Val` sets in stable source-position order before calling into `NiceNameGenerator`, so compiler-generated names like `func1@1-30` no longer vary across builds due to `Val.Stamp` assignment races during parallel parse/type-check. ([Issue #19732](https://github.com/dotnet/fsharp/issues/19732))
* Honor `--nowarn` and `--warnaserror` for warnings emitted during command-line option parsing ([Issue #19576](https://github.com/dotnet/fsharp/issues/19576), [PR #19776](https://github.com/dotnet/fsharp/pull/19776))
* Fix `[<return: X>]` prefix attributes being silently dropped on class members, and fix false-positive `AllowMultiple=false` errors when `[<X>]` and `[<return: X>]` are applied to the same binding. ([Issue #17904](https://github.com/dotnet/fsharp/issues/17904), [Issue #19020](https://github.com/dotnet/fsharp/issues/19020), [PR #19738](https://github.com/dotnet/fsharp/pull/19738))
* Fix attributes on return type of unparenthesized tuple methods being silently dropped from IL. ([Issue #462](https://github.com/dotnet/fsharp/issues/462), [PR #19714](https://github.com/dotnet/fsharp/pull/19714))
Expand Down
1 change: 0 additions & 1 deletion src/Compiler/Driver/OptimizeInputs.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,6 @@ let ApplyAllOptimizations

let results, optEnvFirstLoop =
match tcConfig.optSettings.processingMode with
Copy link
Copy Markdown
Member Author

@T-Gro T-Gro May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Diagnostics / maintainability] Dropping the Parallel optimization breaks determinism comment removes the only in-code signpost telling future readers why iteration order matters in the optimizer. After this PR the invariant becomes implicit in two List.sortWith calls in DetupleArgs / InnerLambdasToTopLevelFuncs. Please leave a short note here (or above ParallelOptimization.optimizeFilesInParallel) explaining that determinism now relies on those two sort sites and pointing to issue #19732, so the next person adding a NiceNameGenerator call from a parallel pass knows to sort first.

// Parallel optimization breaks determinism - turn it off in deterministic builds.
| Optimizer.OptimizationProcessingMode.Parallel ->
let results, optEnvFirstPhase =
ParallelOptimization.optimizeFilesInParallel optEnv phases implFiles
Expand Down
13 changes: 12 additions & 1 deletion src/Compiler/Optimize/DetupleArgs.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,18 @@ let determineTransforms g (z: Results) =
let callPatterns = sitesCPs sites // callPatterns from sites
decideTransform g z f callPatterns (m, tps, vss, retTy) // make transform (if required)

let vtransforms = Zmap.chooseL selectTransform z.Uses
// Walk z.Uses in stable source-position order so the contained call to
Copy link
Copy Markdown
Member Author

@T-Gro T-Gro May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Optimizer correctness — confirm] The old Zmap.chooseL traverses in descending-key (reverse stamp) order; the new code traverses in ascending source-position order. The only intended side effect is the NiceNameGenerator.FreshCompilerGeneratedName call inside decideTransform (line 555). Please double-check that selectTransform / decideTransform do not read any optimizer-global mutable state whose contents depend on prior call order in this pass (e.g. an inlining-decision memo keyed by previously-rewritten Vals). A quick read suggests no such dependency, but worth confirming before relying on order-of-iteration being a free parameter.

// NiceNameGenerator (via decideTransform) sees the same call order across
// runs, regardless of Val.Stamp values assigned during parallel parse.
// See https://github.com/dotnet/fsharp/issues/19732.
let vtransforms =
Zmap.toList z.Uses
|> List.sortWith (fun (v1: Val, _) (v2: Val, _) ->
let r1, r2 = v1.Range, v2.Range
compare
struct (r1.FileIndex, r1.StartLine, r1.StartColumn, v1.LogicalName)
struct (r2.FileIndex, r2.StartLine, r2.StartColumn, v2.LogicalName))
|> List.choose (fun (f, sites) -> selectTransform f sites)
Copy link
Copy Markdown
Member Author

@T-Gro T-Gro May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code quality] The four-tuple source-position key is now duplicated verbatim across two optimizer passes (and any future pass that calls NiceNameGenerator over a Val collection will need the same dance). Extract a single helper — e.g. let valSourceOrderKey (v: Val) = struct (v.Range.FileIndex, v.Range.StartLine, v.Range.StartColumn, v.LogicalName) near valOrder — and reuse from both sites so the invariant is encoded in one place.

let vtransforms = Zmap.ofList valOrder vtransforms
vtransforms

Expand Down
11 changes: 10 additions & 1 deletion src/Compiler/Optimize/InnerLambdasToTopLevelFuncs.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,7 +844,16 @@ let CreateNewValuesForTLR g tlrS arityM fclassM envPackM =
let fHat = mkLocalNameTypeArity f.IsCompilerGenerated m fHatName fHatTy (Some fHatArity)
fHat

let fs = Zset.elements tlrS
// Sort by source position + logical name so the call order into
// NiceNameGenerator is stable regardless of Val.Stamp assignment race
// during parallel type checking. See https://github.com/dotnet/fsharp/issues/19732.
let fs =
Zset.elements tlrS
|> List.sortWith (fun (v1: Val) (v2: Val) ->
let r1, r2 = v1.Range, v2.Range
compare
struct (r1.FileIndex, r1.StartLine, r1.StartColumn, v1.LogicalName)
struct (r2.FileIndex, r2.StartLine, r2.StartColumn, v2.LogicalName))
Copy link
Copy Markdown
Member Author

@T-Gro T-Gro May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Concurrency / determinism] List.sortWith is stable (uses Array.stableSortInPlaceWith), so ties on (FileIndex, StartLine, StartColumn, LogicalName) fall back to the input order — which is Zset.elements tlrS, iterating in valOrder = Val.Stamp order. Stamps are precisely the racy quantity this PR is trying to neutralise, so any two TLR-eligible Vals sharing the sort key (e.g. distinct specializations from the same inlined source location) will reintroduce the nondeterminism the fix is meant to remove. Consider including range.EndLine/EndColumn (and ideally a deterministic secondary discriminator independent of stamp) in the key. Same applies to the DetupleArgs site.

Copy link
Copy Markdown
Member Author

@T-Gro T-Gro May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Test coverage] No regression test in the PR — the only guard is the external eng/test-determinism.ps1 CI script. That script catches any nondeterminism in a full build, but won't pin this specific invariant if a future refactor reintroduces stamp-order iteration in only one of the two passes (the other pass might still mask the diff). A small targeted test that feeds a canonical input through determineTransforms / CreateNewValuesForTLR twice with permuted stamp orders and asserts equal generated-name sequences would lock the contract at the unit level.

let ffHats = List.map (fun f -> f, createFHat f) fs
let fHatM = Zmap.ofList valOrder ffHats
fHatM
Expand Down
Loading