Cache parsing results in incremental build

[majocha]

Addresses #14848.
In this implementation IncrementalBuilder keeps one most recent result of parsing for each file.
Cached items are removed on source change of corresponding file.
This works the same with live buffers and the normal on disk mechanism.

Memory impact does not look bad, for example FCS solution open in Visual Studio:

note: with enablePartialTypeChecking it does not cache files backed with signature.

This is not a new feature. In IncrementalBuild.fs there was code for similar caching before, but it stopped functioning at some point, I guess? Probably a good idea to put a small test for this, but I'm not sure how to do it.

This can be toggled in performance settings in VS:

to do:

• add option to enable / disable
• mention this feature in docs
• add a test

[T-Gro]

Can we estimate the memory impact of this, e.g. on FCS solution ?

[majocha]

Can we estimate the memory impact of this, e.g. on FCS solution ?

Hard to tell how much it allocate, but there seems to be less GC pressure.

Above - main, below this PR.
Both FCS.sln, editing SemanticClassification.fs, with ServiceAnalysis.fs open in editor.

I guess we can finetune the cache to our liking.

[majocha]

FCS solution, after typecheck:

[majocha]

OK, so IncrementalBuilder currently don't seem to have a clear notion of source text change. NotifyFileChanged is being called internally in situations where there obviously is no source change, complicating things and leading to cascades of reparsing everything when typing in editor.

A vague idea:
We keep FSharpSource objects encapsulating access to ISourceText but they are long lived and kept in IncrementalBuilderInitialState. Probably they should be tracked by IncrementalBuilderState, carrying some source text version information. That would also give us something to just weakly attach cached parse results to, without care of keying and lifetime.

[0101]

@majochaNotifyFileChanged is part of this experimental feature: #14076 and can be altered or even removed altogether if we find a better solution. If it's being called where we can obviously tell there's no change we can probably just not call it. Also it shouldn't be called at all by default unless someone opts-in to this feature. So if your improvements work fine when this is disabled it's still good and we can figure out this part later.

[majocha]

Thanks I'll take a look. Yeah, I'm running with live buffers because its 2023 after all 🙂. Possibly some problems with excessive parsing that people report are related to that.

[majocha]

Let's see what breaks.

[majocha]

I'm making some changes to better grasp the code, I will reduce the diff later.

This is slowly taking shape, and feels a bit better when editing with live buffers. Should now work also with the dead ones 🙂.

@0101, about the live buffers: When the user types fast it fires a lot of NotifyFileChanged calls. I wonder whether some debounce / cancellation on the client is needed or does the builder deal with it smartly on it's own?

[0101]

@0101, about the live buffers: When the user types fast it fires a lot of NotifyFileChanged calls. I wonder whether some debounce / cancellation on the client is needed or does the builder deal with it smartly on it's own?

Calling NotifyFileChanged shouldn't be very expensive, it just updates an array in the incremental builder state. Although it does involve a lock and some NodeCode operations. Might be worth to try adding some debounce to it and making a benchmark for it to see if it makes a difference.

[majocha]

Almost done, let's see if it's still green.

[vzarytovskii]

@majocha This looks good so far. Can I please ask you to make sure that the new behaviour is docummented somewhere under docs in the repo?

[majocha]

OK testing this need some more thought. We should actually count cache hits.

[majocha]

@T-Gro, I added some tests that actually count cache hits with telemetry.
This is definitely not something I'm familiar with, so please say if you see anything dumb 🙂.

[majocha]

Tests done, docs done. This is ready for review when green.

[psfinaki]

@majocha thanks for your awesome work. Please also add a resource here, that's for the purpose of searching this setting in the VS options window.

@auduchinok@safesparrow are you fine with merging this?

[vzarytovskii]

That is great, thanks for it. I think we should merge and insert it, so we can start testing it.

[0101]

This looks really good!

[safesparrow]

@safesparrow are you fine with merging this?

I'm ok with this if it either doesn't make caching other usecases harder in the future (it likely doesn't) or already works for all IDEs including Rider.

Last time I tested this it didn't work for Rider, but I didn't test the latest version.

[majocha]

Please also add a resource here, that's for the purpose of searching this setting in the VS options window.

@psfinaki, cool, it shows up in search now.

[majocha]

@safesparrow this should work now for all cases involving incremental builder, whether ISourceText is used or not. There are still cases in VS (Rider probably too) where some ParseInput calls are being made that are not going through IncrementalBuilder. Maybe it's viable to direct these calls to IncrementalBuilder.GetParseResultsForFile instead, I don't know, it's something to investigate...

[T-Gro]

@T-Gro, I added some tests that actually count cache hits with telemetry. This is definitely not something I'm familiar with, so please say if you see anything dumb 🙂.

This looks good, thank you!

[dawedawe]

FYI: We modified a local Rider instance to use this and we can see the cache being hit in some sequences of user interactions.

Taking the Fantomas repository as an example, it can be seen how opening some files is filling up the cache. Especially the files in our .deps folder :)
Opening depending files or building then hits the cache. I need to dig deeper to really see what kind of user interaction sequences with the IDE are profiting from this, but it looks quite good already.
Thanks a lot for this work!

[safesparrow]

I tested it in Rider and can see that files in between the modified file and the file that gets opened/checked are not reparsed.
So in the scenario below:

changing A.fs and opening C.fs does not reparse B.fs (which is good).

There are some other Rider issues/reasons that cause many parsing requests, as described in JetBrains/resharper-fsharp#492 - that might explain the nonconsistent caching behaviour you've seen @dawedawe . But I think the feature is working as expected in the scenario where a chain of files are checked due to a change at the start of the chain.