Experiment: replace cancellable with async

[majocha]

#18238 (comment)

[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]

In a couple of places, because of cancellable, we had to use stackguard. Since async handles tail recursion rather well, we might want to rethink it.

[vzarytovskii]

Also, we need to review places we used cancellable (and now async), if there are any routines which are fully sync, and now will have quite a bit of overhead of creating async.

[majocha]

So this surprisingly passed :) and it seems the test run wasn't noticeably slower. Allocations is probably another story.

In a couple of places, because of cancellable, we had to use stackguard. Since async handles tail recursion rather well, we might want to rethink it.

Yes, those I just removed for now.

Also, we need to review places we used cancellable (and now async), if there are any routines which are fully sync, and now will have quite a bit of overhead of creating async.

Looks like the performance critical place where cancellable was in use is the group of mutually recursive functions in CheckDeclarations.fs. At first glance it is possible to make them fully sync and just add a manual cancel check here and there. OTOH async here handles both cancellation and potential stackoverflow problems.

[majocha]

FCSSourceFiles benchmark this PR:

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
ParseAndCheckFileInProject	1.656 s	0.0151 s	0.0141 s	10000.0000	5000.0000	1000.0000	2.02 GB

vs main:

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
ParseAndCheckFileInProject	1.635 s	0.0207 s	0.0194 s	10000.0000	5000.0000	1000.0000	1.96 GB

[vzarytovskii]

Just removing stack guards might be dangerous, as it can result in SOs on desktop still.

[majocha]

Just removing stack guards might be dangerous, as it can result in SOs on desktop still.

I mean all the recursive calls are now async let! and return!
I thought the trampoline in async handles this reliably?

[vzarytovskii]

It should, yes. We just want to be sure that every path will result in proper recursion

[T-Gro]

This looks too easy to miss in future updates/additions to the service layer.
Is there something we can do to prevent such mistakes?

[majocha]

Enforce it with types, which means use a specialized CE internally again :)

[T-Gro]

This does look a lot better now!

[majocha]

As usual I'm dogfooding this by running a vsix and it seems fine. But I'm kind of worried about performance. In the benchmark the speed is similar and about 3% more allocation but what about really large codebases of the kind that required the stack guard there? Will it slow down to a crawl? Eat up all of the RAM?

[T-Gro]

There are tests which include very large files (I think part of Fsharp.Suite), and some auto-generated massive tests.
We should be OK to lean on those.

[vzarytovskii]

As usual I'm dogfooding this by running a vsix and it seems fine. But I'm kind of worried about performance. In the benchmark the speed is similar and about 3% more allocation but what about really large codebases of the kind that required the stack guard there? Will it slow down to a crawl? Eat up all of the RAM?

Also should keep in mind that allocations grow when you just work with your codebase, as changes trigger typechecks. My usual "benchmark" is to change something in FCS while working on vsix, multiple times.

[majocha]

After some testing, this feels definitely slower / laggier in VS especially with many documents open.
I'm thinking maybe modernizing the cancellable CE would be better. Using resumable code, maybe somehow make the stack guard built in, too?

[T-Gro]

I wonder if we could make

fsharp/vsintegration/src/FSharp.Editor/Common/CancellableTasks.fs

Line 4 in 72ab794

// Which was originally written in 2016 by Robert Peele ([email protected])

that would be a no-op in case of not needing to create any Task.

[majocha]

As first approximation we could just add overloads to cancellableTask builder to consume cancellable seamlessly.