Cache parsing results in incremental build #14852

majocha · 2023-03-06T13:42:11Z

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 · 2023-03-06T14:10:36Z

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

majocha · 2023-03-06T14:25:31Z

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 · 2023-03-06T15:08:45Z

FCS solution, after typecheck:

majocha · 2023-03-07T10:01:58Z

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 · 2023-03-07T10:39:46Z

@majocha NotifyFileChanged 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 · 2023-03-07T10:47:07Z

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 · 2023-03-07T17:27:23Z

Let's see what breaks.

…fsharp into cache-parse-incremental

majocha · 2023-03-08T11:45:22Z

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 · 2023-03-08T12:03:34Z

@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.

src/Compiler/Service/IncrementalBuild.fs

majocha · 2023-03-08T16:19:01Z

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

vzarytovskii · 2023-03-08T16:37:50Z

@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 · 2023-03-10T09:29:37Z

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

majocha · 2023-03-10T11:20:58Z

@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 🙂.

src/Compiler/Service/IncrementalBuild.fs

…fsharp into cache-parse-incremental

majocha · 2023-03-10T16:50:53Z

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 · 2023-03-13T11:48:43Z

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!

src/Compiler/Service/service.fs

safesparrow · 2023-03-13T14:25:16Z

@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 · 2023-03-13T14:27:51Z

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 · 2023-03-13T14:37:23Z

@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 · 2023-03-14T11:26:09Z

@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!

* Cache parsing results in incremental build (#14852) * First take on the F# telemetry (#14889) * Array.Parallel - search functions (tryFindIndex,tryFind,tryPick) (#14827) * Searching functions for Array.Parallel added * with [<Experimental("Experimental library feature, requires '--langversion:preview'")>] annotation to give us space to change/remove this API in the future if needed * Add `GraphNode.FromResult` (#14894) * Add GraphNode.FromResult * fantomas * Fix missing reference (#14892) * Fix missing reference * undo whitespace change --------- Co-authored-by: Jakub Majocha <1760221+majocha@users.noreply.github.com> Co-authored-by: Petr <psfinaki@users.noreply.github.com> Co-authored-by: Tomas Grosup <tomasgrosup@microsoft.com> Co-authored-by: Petr Pokorny <petrpokorny@microsoft.com> Co-authored-by: Kevin Ransom (msft) <codecutter@hotmail.com>

dawedawe · 2023-03-15T17:10:41Z

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 · 2023-03-17T20:52:23Z

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.

first working attempt

ec16126

majocha requested a review from a team as a code owner March 6, 2023 13:42

majocha marked this pull request as draft March 6, 2023 13:42

majocha added 2 commits March 7, 2023 10:28

try cwt

0b9a331

Merge branch 'main' into cache-parse-incremental

66f3dc4

majocha and others added 3 commits March 7, 2023 12:15

there is no source change?

d32e0fd

FSharpSources in state, with weakly attached parsed inputs

a4443d0

Merge branch 'main' into cache-parse-incremental

92bec18

majocha and others added 5 commits March 8, 2023 11:20

invalidate correctly

afe51a0

Merge branch 'main' into cache-parse-incremental

d1495c4

some stupid debounce

93f76d7

fix test

547768c

Merge branch 'cache-parse-incremental' of https://github.com/majocha/…

6249d61

…fsharp into cache-parse-incremental

safesparrow reviewed Mar 8, 2023

View reviewed changes

src/Compiler/Service/IncrementalBuild.fs Show resolved Hide resolved

majocha added 3 commits March 8, 2023 14:26

fix wip

aac93c5

fixit

057834d

revert to notifiedstamps, explicit invalidation

64315bb

majocha and others added 2 commits March 8, 2023 17:44

some cleanup

f923d6d

Merge branch 'main' into cache-parse-incremental

48fff10

telemetry based tests

f05e0e2

safesparrow reviewed Mar 10, 2023

View reviewed changes

src/Compiler/Service/IncrementalBuild.fs Outdated Show resolved Hide resolved

majocha and others added 5 commits March 10, 2023 16:14

add docs

ff204d4

Merge branch 'main' into cache-parse-incremental

d0776dd

feedback, naming

8487db2

Merge branch 'main' into cache-parse-incremental

4c440d1

Merge branch 'cache-parse-incremental' of https://github.com/majocha/…

8603a61

…fsharp into cache-parse-incremental

majocha added 2 commits March 10, 2023 17:52

naming

4deea21

oops

1ea1090

majocha mentioned this pull request Mar 10, 2023

Compiler service: parallel parsing. #14851

Open

psfinaki reviewed Mar 13, 2023

View reviewed changes

vzarytovskii approved these changes Mar 13, 2023

View reviewed changes

0101 approved these changes Mar 13, 2023

View reviewed changes

src/Compiler/Service/service.fs Show resolved Hide resolved

feedback

60e0dcf

majocha added 2 commits March 13, 2023 15:38

Merge branch 'main' into cache-parse-incremental

7da24c5

Merge branch 'main' into cache-parse-incremental

47e748c

psfinaki merged commit fdba974 into dotnet:main Mar 14, 2023

majocha deleted the cache-parse-incremental branch March 14, 2023 10:18

kant2002 pushed a commit to kant2002/fsharp that referenced this pull request Apr 1, 2023

Cache parsing results in incremental build (dotnet#14852)

ceaff9b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache parsing results in incremental build #14852

Cache parsing results in incremental build #14852

majocha commented Mar 6, 2023 •

edited

Loading

T-Gro commented Mar 6, 2023

majocha commented Mar 6, 2023 •

edited

Loading

majocha commented Mar 6, 2023

majocha commented Mar 7, 2023 •

edited

Loading

0101 commented Mar 7, 2023

majocha commented Mar 7, 2023

majocha commented Mar 7, 2023

majocha commented Mar 8, 2023

0101 commented Mar 8, 2023

majocha commented Mar 8, 2023

vzarytovskii commented Mar 8, 2023

majocha commented Mar 10, 2023

majocha commented Mar 10, 2023

majocha commented Mar 10, 2023

psfinaki left a comment

vzarytovskii commented Mar 13, 2023

0101 left a comment

safesparrow commented Mar 13, 2023 •

edited

Loading

majocha commented Mar 13, 2023

majocha commented Mar 13, 2023

T-Gro commented Mar 14, 2023

dawedawe commented Mar 15, 2023

safesparrow commented Mar 17, 2023

Cache parsing results in incremental build #14852

Cache parsing results in incremental build #14852

Conversation

majocha commented Mar 6, 2023 • edited Loading

T-Gro commented Mar 6, 2023

majocha commented Mar 6, 2023 • edited Loading

majocha commented Mar 6, 2023

majocha commented Mar 7, 2023 • edited Loading

0101 commented Mar 7, 2023

majocha commented Mar 7, 2023

majocha commented Mar 7, 2023

majocha commented Mar 8, 2023

0101 commented Mar 8, 2023

majocha commented Mar 8, 2023

vzarytovskii commented Mar 8, 2023

majocha commented Mar 10, 2023

majocha commented Mar 10, 2023

majocha commented Mar 10, 2023

psfinaki left a comment

Choose a reason for hiding this comment

vzarytovskii commented Mar 13, 2023

0101 left a comment

Choose a reason for hiding this comment

safesparrow commented Mar 13, 2023 • edited Loading

majocha commented Mar 13, 2023

majocha commented Mar 13, 2023

T-Gro commented Mar 14, 2023

dawedawe commented Mar 15, 2023

safesparrow commented Mar 17, 2023

majocha commented Mar 6, 2023 •

edited

Loading

majocha commented Mar 6, 2023 •

edited

Loading

majocha commented Mar 7, 2023 •

edited

Loading

safesparrow commented Mar 13, 2023 •

edited

Loading