Refactoring of IncrementalBuilder

[majocha]

I refactored BoundModel by removing TcInfoState. Now there is no PartialState / FullState, we instead create separate GraphNodes computing TcInfo and TcInfoExtras. When computing TcInfo this always skips implementations whenever possible. Computing TcInfoExtras OTOH always does full type check of the file, so previously skipped implementations can be checked in parallel.

I made it more explicit when the skipping of implementation during parsing and type check happens. Separate functions return parsing result placeholder and partial type check info for skipped implementations.

Build graph is now updated in a single pass mapFold in computeStampedFileNames. I find this easier to comprehend, but that's individual matter probably.

A Slot record is introduced to hold together parsing and typechecking GraphNodes for particular file slots.

Removed parsing cache because it's now irrelevant. Parsing step has it's own GraphNode which ensures we never reparse unless the client or the file system notifies there was source change.

enablePartialTypeChecking setting is also now mostly irrelevant, as TcInfoExtras are computed on demand. Although it's easy to change this behavior to parallelized eager full type check when enablePartialTypeChecking = false. I'm not sure what would be best here.

This performs somewhat better in cases when there are backing signatures. There is still room for performance gain if we do the parsing eagerly instead of on-demand, and even more if we could use build graph from #14494.

Some benchmarks:

main:

|                       Method | FastFindReferences | EmptyCache |         Mean |      Error |     StdDev |       Gen0 |      Gen1 |   Allocated |
|----------------------------- |------------------- |----------- |-------------:|-----------:|-----------:|-----------:|----------:|------------:|
| FindAllReferences_MediumCase |              False |      False |     93.77 ms |   1.428 ms |   1.266 ms |          - |         - |     2.53 MB |
| FindAllReferences_MediumCase |              False |       True | 10,664.60 ms | 168.664 ms | 149.516 ms | 11000.0000 | 2000.0000 | 12900.45 MB |
| FindAllReferences_MediumCase |               True |      False |    238.00 ms |   4.718 ms |   5.966 ms |  1000.0000 |         - |   173.17 MB |
| FindAllReferences_MediumCase |               True |       True |  5,518.70 ms |  47.706 ms |  42.291 ms |  9000.0000 | 2000.0000 |  3557.88 MB |

this PR:

|                       Method | FastFindReferences | EmptyCache |         Mean |      Error |     StdDev |       Median |      Gen0 |      Gen1 |     Allocated |
|----------------------------- |------------------- |----------- |-------------:|-----------:|-----------:|-------------:|----------:|----------:|--------------:|
| FindAllReferences_MediumCase |              False |      False |     89.89 ms |   0.671 ms |   0.594 ms |     90.07 ms |         - |         - |    2942.72 KB |
| FindAllReferences_MediumCase |              False |       True | 10,705.55 ms | 209.875 ms | 294.216 ms | 10,532.61 ms | 6000.0000 | 2000.0000 | 7980685.08 KB |
| FindAllReferences_MediumCase |               True |      False |     17.20 ms |   0.370 ms |   1.018 ms |     16.94 ms |         - |         - |     856.41 KB |
| FindAllReferences_MediumCase |               True |       True |  5,297.71 ms |  37.674 ms |  35.240 ms |  5,300.07 ms | 3000.0000 | 1000.0000 | 3215518.17 KB |

There is some win with FastFindReferences, it seems.

more benchmarks in comments.

[majocha]

This seems to give approx 10% gain in ParseProjectWithChangingMiddleFile benchmark compared to main.

[majocha]

I'll continue with this, but first I need some more tests, see #14921, because I can completely break partial type checking and this is still green.

[majocha]

This also needs debouncing when used with live buffers, or it will spam lots of non-cancellable parse tasks when typing. I'll do it in another PR for VS.

[majocha]

So it looks like it was a glitch of Visual Studio loading projects after all.

[majocha]

@vzarytovskii, I think I finally managed to capture some more representative GC data.
Repeated edits in one file when observing another in FSharp.Editor.sln, to make the background compiler churn.
So this is main:

this PR:

While there is less activity over all, it seems a lot more survive to gen2.

[vzarytovskii]

@majocha hm, interesting, it seems that pauses are also longer in general.

[majocha]

@majocha hm, interesting, it seems that pauses are also longer in general.

Exactly, I'm seeing about 3x more pauses > 200ms in the stats.
Some large chunks of data being retained that are not in main? I'll try to look into it.

[majocha]

@vzarytovskii so this is funny, actually forcing tcInfoExtras to compute

    let createBoundModelGraphNode (prevBoundModel: GraphNode<BoundModel>) syntaxTree =
        GraphNode(node {
            let! prevBoundModel = prevBoundModel.GetOrComputeValue()
            prevBoundModel.GetOrComputeTcInfoExtras() |> Async.AwaitNodeCode |> Async.Ignore |> Async.Start
            return! prevBoundModel.Next(syntaxTree)
        })

make the pauses go away, and the profile looks like this.

[vzarytovskii]

@vzarytovskii so this is funny, actually forcing tcInfoExtras to compute

    let createBoundModelGraphNode (prevBoundModel: GraphNode<BoundModel>) syntaxTree =
        GraphNode(node {
            let! prevBoundModel = prevBoundModel.GetOrComputeValue()
            prevBoundModel.GetOrComputeTcInfoExtras() |> Async.AwaitNodeCode |> Async.Ignore |> Async.Start
            return! prevBoundModel.Next(syntaxTree)
        })

make the pauses go away, and the profile looks like this.

Hm, well, this is interesting.

[majocha]

Looks like tcInfoExtras is much more compact than the actual type check results, so it's better to compute it as soon as we have the type check, to reclaim memory.
My idea of holding actual type check results in a GraphNode was worse.

I updated the code and it's now as performant or better than main in VS. I'll have to redo the benchmarks. Let's see if it's still green.

[majocha]

I redid some benchmarks and the gains with .fsi files and fast find references are still good. I installed this in VS and It feels fine.

[TIHan]

This is great work @majocha.

Glad to see there is progress being made in IncrementalBuilder. It used to be one of the more difficult areas to make changes in.

[0101]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[0101]

@majocha I started seeing some UI hangs recently in VS, specifically when finding references. Dump of the process shows that it's waiting on a task that involves this particular RunSynchronously. In this case checking project stamp through IProjectReference.

Since this is on some paths that can be hit from different jobs quite frequently in parallel (when I was finding references semantic classification, document highlights and quick info services were also probably running), we might need to do something about it.

I saw that IProjectReference.TryGetLogicalTimeStamp is not async, but maybe we could just make it async? Are there any other reasons we need this to be synchronous?

[majocha]

This is synchronous to keep type signature of call sites, only because it was all synchronous before. IIRC there was a temporary state computed instead of currentState, sometimes duplicating work.
I'll take a look. If this can't be made async we can try reverting this back to computing temp state.

[majocha]

@0101 I think the problem is that currentState is guarded by semaphore while the tempState was not.

[majocha]

Low cost attempt to fix this: #15146

[0101]

Thanks for looking into it!

[0101]

But also now it should be quite easy to make it async - I'm going to try that.

[majocha]

I have a very faint suspicion, because I experienced weird behaviour recently, but in CI. I'm probably worng but maybe it's the same thing? Find references is very parallel from the get go with all the Task.WhenAll and so happens that this GetHashCode() here:

fsharp/vsintegration/src/FSharp.Editor/LanguageService/FSharpProjectOptionsManager.fs

Line 331 in 9627d33

Stamp = Some(int64 (ver.GetHashCode()))

is very susceptible to collisions when you create many at the same time and we use it for identity. This is grasping at straws but I'd put None there just to try if it helps.

[0101]

Hmm, I'm not even sure what's the point of that. If we have 2 project options where everything is the same except stamp - why do we care which one we use?

[majocha]

The situation I had on mind is different project options getting the same stamp, corrupting service caches. But this is very far fetched.

Is there a way to try to trigger this bug? Just spam shift-f12 in VisualFSharp.sln?

[0101]

Oh I see, I read the description of Stamp wrong. But then I would expect just flat out errors or crashes if that happened.

I think best chances to reproduce this are shift-F12 closely before or after changing some related files, or maybe newly opening them.

[safesparrow]

Removed parsing cache because it's now irrelevant. Parsing step has it's own GraphNode which ensures we never reparse unless the client or the file system notifies there was source change.

@majocha Does that handle cases where project is reloaded without any changes that affect parsing results? Does it handle all types of parsing requests made by the client, either direct or indirect, or only the ones done by the incremental builder?

My suspicion is a fully working cache would still be beneficial in several cases.

[majocha]

Exactly my thoughts, after looking into it some more. The cache in question was handling only incremental builder requests and lifetime was tied to incremental builder instances( pretty much works the same now), but incremental builder instances are efemeral by design, if I understand it correctly.
Otoh IBs provide a built in caching through graphnodes, and it was easy to reuse. It probably would be appropriate to cache parsing on a lower level than that.

[majocha]

Especially when considering dependency graph based type checking, it would be crucial to not reparse the whole project each time the graph needs to be obtained.