Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression #11228

TIHan · 2021-03-12T19:10:52Z

There is a piece of code that reacts to our background compiler's file checked changes to tell Roslyn to re-analyze a file. This bit of code can changed to only do the behavior on script files as Roslyn does the right thing in re-evaluating files whose dependents have changed.

Also, in our current dev16.10 preview, we have a regression where cross-project referencing only "sometimes" work in reacting to changes on dependent projects. This is most likely caused by the changes we have made in removing the reliance on the reactor thread in FCS. This PR fixes this to an extent. There remains one problem where you modify a file and then wait a few seconds before saving, other files won't react to the changes when the file is saved; when a file is saved in Roslyn workspaces, an event does not occur - they only occur when you change the file/source text in the editor itself, or if it changed on-disk outside of VS. When we start removing the dependence on the on-disk file system in FCS, we will be able to fully fix this issue.

The fix for part of the regression is removing the ability to get stale data in what otherwise looks like an up-to-date call. Instead, it will either get the data if the data is still valid by the timestamps on disk, or we use the cacheStamp.

src/fsharp/service/IncrementalBuild.fs

TIHan · 2021-03-12T21:44:03Z

@dsyme - will need you to look at this because this is based on the conversation we had regarding the explicit re-analysis call.

cartermp · 2021-03-17T00:23:20Z

@dsyme I think we need this in. Current dogfood builds of 16.10 constantly peg the CPU. In my case, my CPU is constantly at 15% or higher CPU even if I'm doing nothing and I've only had one file open.

I inspected a trace and found that there is just an insane amount of re-typechecking going on. Something has clearly regressed there:

That's over 17 thousand calls to TcExpr and its children in 39 seconds after leaving a single file in FSharp.Editor open. Clearly something is horribly wrong.

@TIHan's changes fix the issue for me and critically, in-memory cross-project references and all associated tooling all appears to still work.

dsyme · 2021-03-17T19:12:10Z

src/fsharp/service/service.fs

@@ -444,18 +444,17 @@ type BackgroundCompiler(legacyReferenceResolver, projectCacheSize, keepAssemblyC
        }

    member _.GetCachedCheckFileResult(builder: IncrementalBuilder, filename, sourceText: ISourceText, options) =
+        // This is to check if the check results are still valid by their timestamp.
+        // A return of None means the file needs to be re-evaluated as a dependent has changed; therefore, do not get the cached result.
+        match builder.TryGetCheckResultsForFileInProject filename with


Will and I talked this through - this is basically sound and represents a fix, but two problems:

Better if this is just a fix to the existing AreCheckResultsBeforeFileInProjectReady - we only care about the BeforeFileInProject state - this will be a marginal perf improvement hitting the cache more often.

This requires timestamp checks on potentially 1000s of files in large solutions. Currently we do this when caching fails, but never in the process of looking up the cache (which happens very very often, even for basic editing like quick info, when the solution is not changing at all).

For (2) an alternative is to pass in the "GetDependentVersion" from the Roslyn project to act as a first quick positive test for the validity of the cache:

let cacheEntry = lookup cache if (project1.GetDependentVersion == cacheEntry.SavedProjectDependentVersion) THEN use cacheEntry ELSE IF AreCheckResultsBeforeFileInProjectReady... && sourceText.GetHashCode() = cacheEntry.SavedHashCode THEN use cacheEntry ELSE recompute

@dsyme What would the alternative look like for non-Roslyn scenarios? Can this check be a parameter lambda passed to the checker?

@dsyme I've checked and we can compute file/assembly dependencies timestamp for a project efficiently. @TIHan would it be possible to have a parameter with type like FSharpProjectOptions -> DateTime?

I'm not sure what the request is here. You can pass in a hash of the state of the world using the changes in this PR if you wish. If not you will get the old behaviour

So I think what's in is ok. We don't want to complicate things with extra configurable function paramaters

dsyme · 2021-03-17T19:19:36Z

Will and I had a look

Testing: We've had many problems in this area in the past and need a solid manual test matrix that we can each replicate
Scripts: Likely problems with script files (see below)
Perf: Likely problem with too many file stamp time checks when hitting the CheckFileInProject cache, see this comment

Testing

We need a very careful manual test matrix for this (ideally documented under tests/walkthroughs)

script files (with #load and #r - editing, creating, deleting these)
- open script file with #load "b.fs", wait for no errors, make change in b.fs with external tool, see if errors appear when switching back to a.fs. Similarly other changes like deleting b.fs (causing errors), creating b.fs (clearing errors)
in-project files
- external change to file in project
- external change to file in referenced project (with in-memory cross-project references)
- external build/create/delete of DLLs for referenced projects (without in-memory cross-project references)
- external build/create/delete of directly referenced DLLs (with/without in-memory cross-project references)
- changes to signature files
- changes to implementation files

Scripts and Reanalyze() triggers

A Reanalyze call (either by Roslyn or us) is needed when the "before file" logical state of a file changes
Our understanding is that Roslyn correctly makes these calls for all in-project files. Our test matrix needs to confirm this.
- Roslyn has a "SolutionCrawler"
- Our understanding is that Roslyn keeps file-watchers for metadata references and files-in-solution for actual projects. We believe these are adequate to trigger reanalysis events within projects
Our understanding is that Roslyn doesn't make these calls for scripts. How can it? The script is in the "Misc files project" that knows nothing about dependencies.
Typical Scenario: Script references DLLs built by a project. Has red squigglies. Start build of solution from command line.
Go back to script. Expect red squigglies to disappear (because a Reanalyze happens)
Some ideas for how to arrange for these events to be triggered ourself:
1. The current solution is the BeforeFileChecked event being removed here. This relies on something happening in the script (e.g. quick info)
  to trigger ImplicitBackgroundProjectBuild work, which then does the timestamp check, raises the event and
  the reanalysis happens.
2. A background async loop in FSharp.Editor of our own that continually recomputes the scriptProject.BeforeFileStateToken() and if
  it has changed then Reanalyze; OR
3. Somehow set up a Roslyn miscellaneous project that accurately represents the dependencies of the script project
  Then Roslyn would schedule the Reanalyze for us when the FileNotify on the DLL changes.
4. On each user action (focus on document, quick info etc) spawn a single background async check as in (2)

Will will probably go for option (1).

dsyme · 2021-03-17T19:24:29Z

@cartermp Will and I discussed the perf things you've been seeing

Opening a file in FSharp.Editor will result in a typecheck of the entire compiler. This could easily be 17K calls. We're not too worried by this if it was a perf measurement from startup (first open of file) or similar
We're most concerned about the case where you saw ongoing CPU usage even after leaving VS to acquiesce.
Could you write us a step-by-step repro please?

cartermp · 2021-03-17T19:29:35Z

The repro on my machine is:

Open VisualFSharp.sln
Open DocumentDiagnosticAnalyzer.fs
Wait for colors
Keep waiting

I consistently observe 15% CPU at all times with this repro and latest dogfood build of 16.10. If I use 16.9 or if I have @TIHan's fix, I do not observe the constant CPU usage.

dsyme · 2021-03-17T19:31:40Z

I consistently observe 15% CPU at all times with this repro and latest dogfood build of 16.10. If I use 16.9 or if I have @TIHan's fix, I do not observe the constant CPU usage.

OK cool thanks. Odd as nothing in this change should specifically fix this AFAICS....

Is that dogfood the internal preview feed?

cartermp · 2021-03-17T19:43:44Z

Yep, internal feed.

…Editor to only call the FSharpChecker extensions for checking a file.

cartermp · 2021-03-17T21:59:26Z

I think that we should also check against issues like this: #6646

To make sure we haven't regressed.

auduchinok · 2021-03-19T14:35:14Z

src/fsharp/service/service.fs

+            let cacheStamp = defaultArg cacheStamp (sourceText.GetHashCode() |> int64)
+
+            // Check the cache. We can only use cached results when there is no work to do to bring the background builder up-to-date
+            let cachedResults = parseCacheLock.AcquireLock (fun ltok -> checkFileInProjectCache.TryGet(ltok, (filename, cacheStamp, options)))


@TIHan This check doesn't work properly for me: changes in prior files don't make subsequent files get reanalyzed when cacheStamp is not used. Consider the following repro steps for project with two files:

Parse and check File2.fs (File1.fs is checked as a dependency, results for File1 and File2 get cached)

Change File1.fs and parse and check it

Parse and check File2.fs again, it'll have the same stamp (since its source hasn't changed), so it will use the old results

The AreCheckResultsBeforeFileInProjectReady should be returning false in your case, perhaps I'll force clear the cache since it's technically invalid at that point.

Should we able to use CheckFileInProjectAllowingStaleCachedResults for invalid results? It might be OK for some features.

I think that's fair, but I don't know if FCS should dictate when to use stale cached results as each editor will be different.

CheckFileInProjectAllowingStaleCachedResults looks like it's considered Obsolete and encourages us to use CheckFileInProject. I think we should consider not making it Obsolete and just have a separate call to get cached stale results.

dsyme · 2021-03-23T01:16:41Z

src/fsharp/service/service.fs

+        // If a cache stamp is not provided, we need to do a full-up-to-date check on the timestamps for the dependencies.
+        let recheckDeps = cacheStamp.IsNone
+        let cacheStamp = defaultArg cacheStamp (sourceText.GetHashCode() |> int64)
+        if recheckDeps && not (builder.AreCheckResultsBeforeFileInProjectReady filename) then 


This doesn't yet convince me. If cacheStamp.IsNone I now believe we should basically do as we did before.

We must avoid calling AreCheckResultsBeforeFileInProjectReady before the lookup since it is an expensive operation.

We should look up the cache using sourceText.GetHashCode() and re-check validity of the entry after a successful lookup, just as we did before. If invalid we can remove the entry

The previous code's use of GetCheckResultsBeforeFileInProjectEvenIfStale used tcPrior.TimeStamp = priorTimeStamp as an extra way of avoiding a AreCheckResultsBeforeFileInProjectReady call. That should stay in, it is a correct optimization. If the stamps don't match then there's no chance AreCheckResultsBeforeFileInProjectReady will succeed.

That is, for the case where cacheStamp.IsNone I'm now convinced the previous code was both correct and efficient (given that AreCheckResultsBeforeFileInProjectReady is fixed as part of this PR) - so the basic structure of what was there should stay in place. Adding the cache removal makes sense

I see. Now that AreCheckResultsBeforeFileInProjectReady is fixed, we should keep the previous behavior when the cacheStamp is None.

…mp is None

…a lock around it.

TIHan · 2021-03-24T02:20:55Z

This is ready.

TIHan added 3 commits March 12, 2021 01:05

Initial analysis fix

c858314

Minor cleanup

38822d2

Minor cleanup

641c086

cartermp approved these changes Mar 12, 2021

View reviewed changes

src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved

TIHan requested a review from dsyme March 12, 2021 21:44

dsyme reviewed Mar 17, 2021

View reviewed changes

Added re-analysis back in just for scripts

7399456

Added cacheStamp parameter to check file FCS APIs. Refactored FSharp.…

c7d1b4f

…Editor to only call the FSharpChecker extensions for checking a file.

TIHan changed the title ~~Remove re-analysis check in VS - Fixed a cross-project reference regression~~ Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression Mar 17, 2021

TIHan added 7 commits March 17, 2021 15:08

Merge remote-tracking branch 'remote/main' into analysis-fix

6d65484

Fixing build

adab61d

Fixing tests

e1e4434

Fixing tests

d729592

Fixing build

edf0cb1

Fixing tests

3af59e6

Fixing tests

a70be9c

auduchinok reviewed Mar 19, 2021

View reviewed changes

TIHan added 5 commits March 19, 2021 10:09

Trying to fix tests

0183a26

Force remove cache

c53322f

Trying to fix test

163fbcb

Trying to fix

d45e796

Updating test again

9f92c0b

TIHan added 7 commits March 19, 2021 16:16

Still trying to fix tests

6eb5e1c

Merge remote-tracking branch 'remote/main' into analysis-fix

9df8808

Fixing build

3364dc2

Trying to fix tests

895cf2d

Passing filepath to creating a test document

9064891

fixing build

c7d1561

Still trying to fix tests

20c1313

runfoapp bot mentioned this pull request Mar 22, 2021

VerifyArea test failing #11213

Closed

auduchinok mentioned this pull request Mar 22, 2021

ParseAndCheckFileInProject may return stale results #11291

Closed

TIHan added 8 commits March 22, 2021 11:05

Use SourceCodeKind.Regular

57b34e3

Fixing tests

8cc0ca3

Minor update to verify no errors

4f886e0

Creating version stamps with date times for tests

f902148

still trying to fix tests

a4a0a11

Make projects unique in testing diagnostics

c9e7a47

Trying to pass tests

b9a1780

This should fix the rest

a584404

TIHan closed this Mar 22, 2021

TIHan reopened this Mar 22, 2021

dsyme reviewed Mar 23, 2021

View reviewed changes

TIHan added 5 commits March 22, 2021 20:17

Adding back original logic for GetCachedCheckFileResult when cacheSta…

657ea0e

…mp is None

fixing tests

f8ae29d

fixing tests

4e45d47

Got a CancellationTokenSource has been disposed in a VS test. Adding …

4672cd1

…a lock around it.

Fixing lock

48cfe41

dsyme approved these changes Mar 24, 2021

View reviewed changes

dsyme merged commit 36b286b into dotnet:main Mar 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression #11228

Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression #11228

TIHan commented Mar 12, 2021 •

edited

Loading

TIHan commented Mar 12, 2021

cartermp commented Mar 17, 2021 •

edited

Loading

dsyme Mar 17, 2021

auduchinok Mar 17, 2021

auduchinok Mar 23, 2021 •

edited

Loading

dsyme Mar 25, 2021 •

edited

Loading

dsyme commented Mar 17, 2021 •

edited

Loading

dsyme commented Mar 17, 2021

cartermp commented Mar 17, 2021

dsyme commented Mar 17, 2021 •

edited

Loading

cartermp commented Mar 17, 2021

cartermp commented Mar 17, 2021

auduchinok Mar 19, 2021 •

edited

Loading

TIHan Mar 19, 2021

auduchinok Mar 22, 2021

TIHan Mar 22, 2021

TIHan Mar 22, 2021 •

edited

Loading

dsyme Mar 23, 2021 •

edited

Loading

TIHan Mar 23, 2021

TIHan commented Mar 24, 2021

Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression #11228

Remove re-analysis check for non-scripts in VS - Fixed a cross-project reference regression #11228

Conversation

TIHan commented Mar 12, 2021 • edited Loading

TIHan commented Mar 12, 2021

cartermp commented Mar 17, 2021 • edited Loading

dsyme Mar 17, 2021

Choose a reason for hiding this comment

auduchinok Mar 17, 2021

Choose a reason for hiding this comment

auduchinok Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

dsyme Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

dsyme commented Mar 17, 2021 • edited Loading

Testing

Scripts and Reanalyze() triggers

dsyme commented Mar 17, 2021

cartermp commented Mar 17, 2021

dsyme commented Mar 17, 2021 • edited Loading

cartermp commented Mar 17, 2021

cartermp commented Mar 17, 2021

auduchinok Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

TIHan Mar 19, 2021

Choose a reason for hiding this comment

auduchinok Mar 22, 2021

Choose a reason for hiding this comment

TIHan Mar 22, 2021

Choose a reason for hiding this comment

TIHan Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

dsyme Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

TIHan Mar 23, 2021

Choose a reason for hiding this comment

TIHan commented Mar 24, 2021

TIHan commented Mar 12, 2021 •

edited

Loading

cartermp commented Mar 17, 2021 •

edited

Loading

auduchinok Mar 23, 2021 •

edited

Loading

dsyme Mar 25, 2021 •

edited

Loading

dsyme commented Mar 17, 2021 •

edited

Loading

dsyme commented Mar 17, 2021 •

edited

Loading

auduchinok Mar 19, 2021 •

edited

Loading

TIHan Mar 22, 2021 •

edited

Loading

dsyme Mar 23, 2021 •

edited

Loading