Cancellable: built-in stack guard

[majocha]

Experimentally moved stack guard functionality intoCancellable.run to make recursion in cancellable generally safe.

If this works, it would allow to simplify some code in CheckDeclarations.fs.

[github-actions]

❗ 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/9.0.300.md	No release notes found or release notes format is not correct

[vzarytovskii]

What's the codegen for calling cancellable from itself with let!, return!? Stackguard will have to wrap the cancellable in this case (either properly inline or be part of the method).

[vzarytovskii]

Also, if it's universally in the cancellable, it might be quite an overhead. I doubt that that closure will be just inlined.

[majocha]

Yes, good questions. For now, it just passes around the depth as int. Maybe it would be possible to branch out on it into fast inlined run and slower SwitchToNewThread?

[majocha]

Ok, I did some tests on the side to make sure this prevents stackoverflow both in mutually recursive and self-recursive computations, benchmarking with FCSSourceFiles does not show any performance degradation.

But! I have a feeling this is not a correct approach, and a trampoline should be used instead, especially given the mutual recursion requirement.

[vzarytovskii]

Ok, I did some tests on the side to make sure this prevents stackoverflow both in mutually recursive and self-recursive computations, benchmarking with FCSSourceFiles does not show any performance degradation.

But! I have a feeling this is not a correct approach, and a trampoline should be used instead, especially given the mutual recursion requirement.

Out of curiosity, how did you test it? Because it will likely be fine on the CoreCLR, since JIT does a good job on the tail-prefixed calls. Full CLR is where it was causing problems.

[majocha]

Out of curiosity, how did you test it? Because it will likely be fine on the CoreCLR, since JIT does a good job on the tail-prefixed calls. Full CLR is where it was causing problems.

I just copied the whole Cancellable.fs to a fresh console app and wrote a few dumb mutually recursive functions like

let rec test x =
    cancellable {
        if x >= 400_000 then
            printfn "done!"
            return 0
        else
            let! x = test (x + 1)
            return x
    }

let rec mut1 x =
    cancellable {
        if x >= 500_000 then
            printfn "done!"
            return 0
        else
            let! x = mut2 (x + 1)
            return x
    }
and mut2 x =
    cancellable {
        let! x = mut1 (x + 1)
        return x
    }

And made sure it fails before and succeeds with new code.

[majocha]

I think the code in this PR works in practice for some limited depth of recursion, but is incorrect generally. The threads are started before the previous one finishes. If I put a million recursive calls in a test, the whole thing slows down to a crawl.

[majocha]

I've been thinking a little bit, so I just write it down here.
There were some stackoverflow problems in CheckDeclarations.fs. My understanding of it goes like that:
The lack of tailcall optimization in the cancellable builder, solved by rewriting the cancellable expression into compiler optimized function:

fsharp/src/Compiler/Checking/CheckDeclarations.fs

Line 5479 in 58560f8

and [<TailCall>] TcModuleOrNamespaceElementsNonMutRec cenv parent typeNames endm (defsSoFar, env, envAtEnd) (moreDefs: SynModuleDecl list) (ct: CancellationToken) =

This didn't fully fix things, because there was also some non-tailcall recursion. That's why GuardCancellable was added, e.g.:

fsharp/src/Compiler/Checking/CheckDeclarations.fs

Lines 5351 to 5352 in 58560f8

	TcModuleOrNamespaceElements cenv (Parent (mkLocalModuleRef moduleEntity)) endm envForModule xml None [] moduleDefs
	|> cenv.stackGuard.GuardCancellable

It seems this all can be handled more generally using trampoline. For example, async builder does this on every bind:

fsharp/src/FSharp.Core/async.fs

Lines 247 to 253 in 58560f8

	/// Call a continuation, but first check if an async computation should trampoline on its synchronous stack.
	member inline _.HijackCheckThenCall (cont: 'T -> AsyncReturn) res =
	if trampoline.IncrementBindCount() then
	trampoline.Set(fun () -> cont res)
	else
	// NOTE: this must be a tailcall
	cont res

Maybe resumable code would simplify the implementation: coroutine with tailcalls example.

I don't think I have the skills, but reimplementing cancellable like this, just to see how it performs looks like a fun thing to try.