Stabilize parallel optimizer Val iteration for deterministic synthetic names#19810
Stabilize parallel optimizer Val iteration for deterministic synthetic names#19810T-Gro wants to merge 1 commit into
Conversation
…19732) Optimize/DetupleArgs.determineTransforms and Optimize/InnerLambdasToTopLevelFuncs.CreateNewValuesForTLR walked Val sets in Val.Stamp order. Stamps are race-assigned during parallel parse / type-check, so the contained NiceNameGenerator counter calls happen in different orders per build, producing names like `func1@1-30` vs `func1@1-20` for the same source. Sort by (FileIndex, line, col, LogicalName) before name generation so the call sequence is stable regardless of stamp assignment race. Also drops the stale OptimizeInputs.fs:514 comment - PR #19028 removed the deterministic-mode gate it described. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
| 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)) |
There was a problem hiding this comment.
[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.
| 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) |
There was a problem hiding this comment.
[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.
| @@ -511,7 +511,6 @@ let ApplyAllOptimizations | |||
|
|
|||
| let results, optEnvFirstLoop = | |||
| match tcConfig.optSettings.processingMode with | |||
There was a problem hiding this comment.
[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.
| 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)) |
There was a problem hiding this comment.
[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.
| 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 |
There was a problem hiding this comment.
[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.
Partial fix for #19732.
Optimize/DetupleArgs.determineTransformsandOptimize/InnerLambdasToTopLevelFuncs.CreateNewValuesForTLRwalked theirValsets inVal.Stamporder. Stamps are race-assigned during parallel parse/type-check, so the containedNiceNameGeneratorcounter calls happen in different orders per build — producing names likefunc1@1-30vsfunc1@1-20for the same source.Fix: sort by
(FileIndex, StartLine, StartColumn, LogicalName)before name generation, so the call sequence is stable regardless of stamp assignment race. Follows the pattern established in #19028 (post-pass sort, notcConfig.deterministicgating).Out of scope
Additional races remain in IlxGen (e.g.
TypeDefsBuilder.AddTypeDefInterlocked.Increment(&countUp)race noted at the comment onIlxGen.fs:2093). Fixing theAddTypeDefordering changes ~190 EmittedIL baselines and only marginally improves a custom in-process determinism test, so it is deferred. The real regression guard remainseng/test-determinism.ps1in CI.