Skip to content

Experiment: limited use synchronous stack guard #18966

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

Closed
wants to merge 2 commits into from
Closed

Experiment: limited use synchronous stack guard #18966

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2cab3db
only push after maxDepth reached
majocha Oct 4, 2025
5f9054f
Merge branch 'main' into worklist
majocha Oct 7, 2025
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
33 changes: 31 additions & 2 deletions src/Compiler/Checking/FindUnsolved.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,42 @@ type env = | NoEnv

let FindUnsolvedStackGuardDepth = StackGuard.GetDepthOption "FindUnsolved"

type SyncStackGuard(maxDepth) =
Copy link
Member

@T-Gro T-Gro Oct 6, 2025

Choose a reason for hiding this comment

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

Its definitely an elegant way of changing behavior while minimizing risks and diff size.

A downside compared to hand-written, non-generic, usage directly in the processing function (e.g. just storing a Stack<Expr>) is repeated closure for effectively the same argument set.

If that does not turn up being a visible issue, this could be a solution 👍

Copy link
Contributor Author

@majocha majocha Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

With slight adjustment It is possible to make it work with single closure and a Stack<Expr> at the cost of more obscure application, for example:

SyncStackGuard.Guard cenv.stackGuard expr <| fun expr ->

instead of current, nicer

cenv.stackGuard.Guard <| fun () ->

EDIT: But that would still allocate a closure on each call. Just like the original StackGuard (?)

Copy link
Member

@T-Gro T-Gro Oct 6, 2025

Choose a reason for hiding this comment

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

Yes, a closure would still be there, just without the surrounding params.
In that case I think the proposed version with same API, i.e. cenv.stackGuard.Guard <| fun () -> is OK 👍

let mutable stack = Unchecked.defaultof<_>
let mutable depth = 0
let mutable running = false

let run f =
running <- true
try
stack <- System.Collections.Generic.Stack<_>()
stack.Push f
while stack.Count > 0 do
let f = stack.Pop()
f()
finally
running <- false

let push f =
if not running then run f else stack.Push f

member _.Guard f =
depth <- depth + 1
try
if depth % maxDepth = 0 then
push f
else
f()
finally
depth <- depth - 1

/// The environment and collector
type cenv =
{ g: TcGlobals
amap: Import.ImportMap
denv: DisplayEnv
mutable unsolved: Typars
stackGuard: StackGuard }
stackGuard: SyncStackGuard }

override _.ToString() = "<cenv>"

Expand Down Expand Up @@ -318,7 +347,7 @@ let UnsolvedTyparsOfModuleDef g amap denv mdef extraAttribs =
amap=amap
denv=denv
unsolved = []
stackGuard = StackGuard(FindUnsolvedStackGuardDepth, "UnsolvedTyparsOfModuleDef") }
stackGuard = SyncStackGuard FindUnsolvedStackGuardDepth }
accModuleOrNamespaceDef cenv NoEnv mdef
accAttribs cenv NoEnv extraAttribs
List.rev cenv.unsolved
Expand Down
Loading