Skip to content

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Oct 4, 2025

Description

Synchronous stack guard in FindUnsolved.fs.
This only works on a unit-returning function.

@majocha majocha requested a review from a team as a code owner October 4, 2025 21:01
@majocha majocha marked this pull request as draft October 4, 2025 21:02
Copy link
Contributor

github-actions bot commented Oct 4, 2025

❗ Release notes required

@majocha,

Caution

No release notes found for the changed paths (see table below).

Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format.

The following format is recommended for this repository:

* <Informative description>. ([PR #XXXXX](https://github.com/dotnet/fsharp/pull/XXXXX))

See examples in the files, listed in the table below or in th full documentation at https://fsharp.github.io/fsharp-compiler-docs/release-notes/About.html.

If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

You can open this PR in browser to add release notes: open in github.dev

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.0.md No release notes found or release notes format is not correct

@majocha majocha changed the title Experiment: limited use worklist-based stack guard Experiment: limited use synchronous stack guard Oct 5, 2025

let FindUnsolvedStackGuardDepth = StackGuard.GetDepthOption "FindUnsolved"

type SyncStackGuard(maxDepth) =
Copy link
Member

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

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

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 👍

@majocha
Copy link
Contributor Author

majocha commented Oct 9, 2025

I think we can put this on hold, in favor of #18971
Synchronous stack guard would be more useful in hotter paths of TypedTreeOps.fs, like accFreeInExprNonLinear, and ExprFolder but that is a more involved change.

@majocha majocha closed this Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants