Skip to content

Conversation

@vzarytovskii
Copy link
Member

Experiment of using explicit heap-located stack instead of (non-tail) recursion for FindUnsolved to get rid of quite expensive StackGuard.

This is a WIP, please note that code is imperative (not piping into Option.iter or List.iter) on purpose so it is easier for me to debug and step through. If this works out I can convert to whatever style is preferred

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

⚠️ 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

@vzarytovskii vzarytovskii changed the title WIP: Remove recursion and StackGuard, use explicit mutable stack WIP: Remove (non-tail) recursion and StackGuard from FindUnsolved.fs, use explicit mutable stack Oct 1, 2025
@vzarytovskii
Copy link
Member Author

@majocha @T-Gro fyi

@vzarytovskii vzarytovskii requested a review from Copilot October 1, 2025 16:39
@vzarytovskii vzarytovskii marked this pull request as ready for review October 1, 2025 18:04
@vzarytovskii vzarytovskii requested a review from a team as a code owner October 1, 2025 18:04
@vzarytovskii vzarytovskii added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Oct 1, 2025
@vzarytovskii
Copy link
Member Author

@majocha not entirely sure what would good test be, or how to repeat tests you were performing in the StackGuard stats PR.

@majocha
Copy link
Contributor

majocha commented Oct 1, 2025

It was building and editing some repos, including fantomas, IcedTasks and this one here. I'll try to find out which one was especially heavy with FindUnsolved stackguard.

Yeah, it was IcedTasks all the Expecto tests, with lots of CEs. Very slow to edit in VS.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@vzarytovskii
Copy link
Member Author

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Bruh. Do better.

@majocha
Copy link
Contributor

majocha commented Oct 1, 2025

this is on current main, FCS net 10.0 outputs in binlog with simply
Build.cmd -c Release

    StackGuard jumps:
    ----------------------------------------------------------------
    |         caller         |           source            | jumps |
    |------------------------|-----------------------------|-------|
    | OptimizeExpr           |           Optimizer.fs:2343 |  1630 |
    | RewriteExpr            |        TypedTreeOps.fs:9703 |    46 |
    | accFreeInExprNonLinear |        TypedTreeOps.fs:5433 |   938 |
    | pushShadowedLocals     |           ilwritepdb.fs:959 |     2 |
    | CheckExpr              | PostInferenceChecks.fs:1152 |    51 |
    | CheckExpr              |       TailCallChecks.fs:315 |   102 |
    | accExpr                |          FindUnsolved.fs:48 |   866 |
    | remapExprImpl          |        TypedTreeOps.fs:6008 |  5120 |
    | exprF                  |        TypedTreeOps.fs:7448 | 12641 |
    ----------------------------------------------------------------

To see the the stats from incremental checking you currently need to launch debug build of VS. One thing I missed yesterday is that the debug build sets default stackguard threshold at 50, opposed to 100 on release. It seems FindUnsolved sails just below that 100 in many cases.

But I have a hunch these sample sources in the CE Benchmark might be useful:
image

Yep, this is what happens when I simply open CE200xnest5.fs in VS:
image

type env = | NoEnv

let FindUnsolvedStackGuardDepth = StackGuard.GetDepthOption "FindUnsolved"
type [<NoEquality; NoComparison>] WorkItem =
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use just Expr as the work item, without the wrapper type?
Loop in accExpr and pass a push to other functions. Instead of calling back to accExpr they cut short the recursion by pushing exprs?

let rec accExpr (cenv: cenv) (env: env) expr =
  let stack = Stack<Expr>()
  let push (e: Expr) = stack.Push e
  push expr

  while stack.Count > 0 do

    let expr = stack.Pop() |> stripExpr
    match expr with
    | Expr.Sequential (e1, e2, _, _) ->
        push e1
        push e2

    | Expr.Let (bind, body, _, _) ->
        accBind cenv env bind push
        push body

    //    ....etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose. I wanted to be more explicit

Copy link
Member

Choose a reason for hiding this comment

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

Using Expr directly feels more direct to me as well - firstly we avoid the need to maintain a parallel hierarchy of types, second it also avoids another allocation per iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even simpler, take a look please #18966

Copy link
Member Author

Choose a reason for hiding this comment

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

That'll probably work, though subjectively a bit harder to understand than explicit one (everything is hidden behind special stackguard, but same goes for normal stackguard).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's a drop-in replacement, but only for unit returning functions, so basically only here.

I tried to apply this or come up with something similar for the biggest offender, ExprFolders but the code around exprF is so general and indirect already, it fries my brain.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Oct 6, 2025

I'm afraid, I don't have free time to tinker with it anymore (this month), feel free to pick it up (should be an easy change to address the comment), or use the specialized stack guard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants