[Tooling Optimization] Skip background type-checking on implementation files if signature files exist #10199

TIHan · 2020-09-28T23:49:55Z

Well over a year ago, I experimented with an optimization trick that sped up tooling significantly. Here, I'm trying to see if it's worth actually getting it in.

When editing then saving files, we always have to re-type-check everything - sometimes entire project(s) even on a single file change. This means we are constantly building a lot of large structures and throwing them away in a short amount of time. This takes a lot of CPU time and memory.

The optimization basically stops the background type-checking of implementation files if a backing signature(.fsi) file exists. This should be safe to do as the signature file will always be the public-facing API over the implementation file; therefore, the implementation file doesn't matter in most cases. You will still have to type-check an implementation file if you open it, or perform a find-all references.

The optimization only affects projects who use signature files. But for FSharp.Compiler.Private, it gives a very significant speed up in order to get colorization, tool-tips, etc.

The following are some results from starting up VisualFSharp.sln:

FSharp.Compiler.Private - service.fs
- Without Optimization: 60-65 seconds to colorization
- With Optimization: 18-23 seconds to colorization
FSharp.Editor - FSharpProjectOptionsManager.fs
- Without Optimization: 62-67 seconds to colorization
- With Optimization: 18-25 seconds to colorization

This is very significant. At the very least, it would boost tooling performance when working in the compiler.

Remaining and currently known tasks:

Lazily type-check implementation files when we perform a find-all references.
(Stretch) Lazily parse implementation files - extra perf gain; if we don't have to type-check it, we might not have to parse it.
(Stretch) Prevent parse/type-check of other files when modifying implementation files with backing signature files; allows modifying implementation files without ever having to analyze any other file or project. Skip parse/type-check of other files when modifying implementation files with backing signature files #10210 Will make a separate PR for that change, it's the inverse of this optimization.

Implementation details:

Adds a new flag to FSharpChecker, enablePartialTypeChecking - this will enable the optimization described above.

saul · 2020-09-29T11:05:03Z

This would be absolutely brilliant to get merged in - we make heavy use of signature files in our libraries and would love to see how this improves our IDE experience.

TIHan · 2020-09-29T14:46:57Z

@saul, here is a VSIX you can try out: VisualFSharpFull.zip

The state of the current change does what it is intended to do; it should give you an indication of how well it improves your experience. Some tooling will not work, such as find-all refs, but I think go-to-def should still be functional.

src/fsharp/CompilerConfig.fs

src/fsharp/ParseAndCheckInputs.fs

src/fsharp/service/service.fs

src/fsharp/service/IncrementalBuild.fs

TIHan · 2020-10-02T20:36:32Z

@jonsequitur @cartermp I can say that this is done now.

cartermp

The overall approach seems reasonable. Just a quick check - do the following work without issue?

Find Refs
Rename
Autocompletion
Lightbulb suggesting names after a misspelling
Add open code fix to something that was defined in another file or project
etc.

If there's high confidence in this kind of stuff then I think this is good. I'm eager to get it into dogfood builds to evaluate it across a larger codebase.

cartermp · 2020-10-02T22:59:19Z

src/fsharp/service/service.fs

@@ -687,75 +709,90 @@ type BackgroundCompiler(legacyReferenceResolver, projectCacheSize, keepAssemblyC
    /// Fetch the check information from the background compiler (which checks w.r.t. the FileSystem API)
    member __.GetBackgroundCheckResultsForFileInProject(filename, options, userOpName) =
        reactor.EnqueueAndAwaitOpAsync(userOpName, "GetBackgroundCheckResultsForFileInProject", filename, fun ctok -> 
-          cancellable {
+            cancellable {


this tabbing over now makes the inner scope the same, which looks worse :/

Yup, I see why.

cartermp · 2020-10-02T23:01:48Z

src/fsharp/service/IncrementalBuild.fs

@@ -1022,16 +1023,91 @@ module IncrementalBuilderEventTesting =

 module Tc = FSharp.Compiler.TypeChecker

+[<AutoOpen>]
+module IncrementalBuildSyntaxTree =


I know this is internal, but I think there needs to be some comments explaining why we need our own abstraction over a syntax tree for the incremental builder. If I were to need to change something in here, I'd have nothing to look at for context on why this exists.

We need some kind of object type that wraps the information to lazily parse the source text to get a ParsedInput(which is the root of the syntax tree). The name SyntaxTree feels correct here.

cartermp · 2020-10-02T23:03:12Z

src/fsharp/service/IncrementalBuild.fs


-/// Accumulated results of type checking.
+    [<Sealed>]
+    type SyntaxTree (tcConfig: TcConfig, fileParsed: Event<string>, lexResourceManager, sourceRange: range, filename: string, isLastCompiland) =


Should this also be private?

I don't think so. We need access to SyntaxTree when the enclosing module is auto-opened.

I actually do not want the IncrementalBuildSyntaxTree module; instead we should just define SyntaxTree without putting in an auto-opened module. The reason why we have this module is because SyntaxTree is already a module in the compiler, so I can't define the type with that name unless I put it in another module, and then open that.

KevinRansom

I love the change, and it looks good. I have some style questions and naming nonsense. But other than that it looks great.

KevinRansom · 2020-10-05T00:23:34Z

src/fsharp/ParseAndCheckInputs.fs

-              let! topAttrs, implFile, _implFileHiddenType, tcEnvAtEnd, createsGeneratedProvidedTypes = 
-                  TypeCheckOneImplFile (tcGlobals, tcState.tcsNiceNameGen, amap, tcState.tcsCcu, checkForErrors, conditionalDefines, tcSink, tcConfig.internalTestSpanStackReferring) tcImplEnv rootSigOpt file
+              let typeCheckOne = 
+                  if skipImplIfSigExists && hadSig then


Just out of curiosity, is hadsig the right name? It seems as if it should be hasSig.

hadSig refers to if the impl file "had" a sig before we type-checked. hasSig might mean if the impl has any kind of signature, whether it be generated after a type-check or from a '.fsi' file.

Honestly, hasSig might also be correct depending on how you look at it.

The most accurate name would be hasSigFromSigFile.

KevinRansom · 2020-10-05T00:30:45Z

src/fsharp/service/IncrementalBuild.fs

@@ -1022,16 +1023,91 @@ module IncrementalBuilderEventTesting =

 module Tc = FSharp.Compiler.TypeChecker

+[<AutoOpen>]


AutoOpen really? seems a bit broad.

I wouldn't say it's broad; there is only one item in the module, which is SyntaxTree.

KevinRansom · 2020-10-05T00:49:40Z

src/fsharp/service/IncrementalBuild.fs

+            return state.Partial
+        }
+
+    member this.TcInfoFull =


it's not clear to me what this name means? the value gets an unnamed tuple of TcInfo, TcInfoOptional.

perhaps the name should be this.TcInfoAndTcInfoOptional ... which is yuk!!! but at least it tells us what we are getting.

oh I don't know.

Ah yes, this name is not great, but I'm struggling to figure out what I should call it.

Basically, whenever I try to access this property, it's going to try to type-check the file to get the full and rich information that would be needed for tooling. TcInfoAndTcInfoOptional is accurate and might be better though.

KevinRansom · 2020-10-05T00:54:57Z

src/fsharp/service/service.fs

                let parseResults = FSharpParseFileResults(creationErrors, None, true, [| |])
                let typedResults = FSharpCheckFileResults.MakeEmpty(filename, creationErrors, reactorOps, keepAssemblyContents)
                return (parseResults, typedResults)
            | Some builder -> 
                let! (parseTreeOpt, _, _, untypedErrors) = builder.GetParseResultsForFile (ctok, filename)
                let! tcProj = builder.GetCheckResultsAfterFileInProject (ctok, filename)
+
+                let tcInfo, tcInfoOptional = tcProj.TcInfoFull ctok


How do you determine whether to space separate functions or have them right next to each other? I struggle with this too. I wondered why line 742, 733 and 735 are empty lines.

In this case, the call TcInfoFull is really important to see clearly as it can do a lot of computation depending on if enablePartialTypeChecking in FSharpChecker is set to true.

Lines 733/735 - the spaces should be removed.

KevinRansom · 2020-10-05T00:55:55Z

src/fsharp/service/service.fs

+              let! (tcProj, ilAssemRef, tcAssemblyDataOpt, tcAssemblyExprOpt) = builder.GetCheckResultsAndImplementationsForProject(ctok)
+              let errorOptions = tcProj.TcConfig.errorSeverityOptions
+              let fileName = TcGlobals.DummyFileNameForRangesWithoutASpecificLocation
+


818 and 820 are empty, the remainder not, I wondered why?

Same reasoning I mentioned above.

TIHan · 2020-10-06T19:57:14Z

@cartermp All tooling should still work as before. There is a performance difference with find-all refs as we now have to re-type-check files that only have partial data(or no symbols recorded).

When this optimization is enabled, background type-checking will not record any symbols on a file, making it partial. We do this to avoid extra work when it isn't needed.

…n files if signature files exist (dotnet#10199) * Initial work to enable FSI optimizations in tooling * Remove extra type-check function * Not working but initial work to lazily eval * some work * simplify * simplify again * Using Async * work * work * builds * almost there * Minor change * minor refactor * working, but not quite optimized * Fix dependency files * Refactor types * Added 'enableLazyTypeChecking'. * Enable lazy type checking in VS * Added documentation * Added SyntaxTreeAccumulator * prevent parsing impl files if possible * Trying to fix tests * Refactor * Throw if enablePartialChecking and keepAssemblyContents are both enabled * minor comment update * minor refactor * Trying to pass tests * Only getting assembly data * Remove unnecessary change * Added some documentation * Minor comment update * Renamed quickCheck to skipImplIfSigExists * Minor update * Feedback

TIHan added 2 commits September 28, 2020 16:21

Initial work to enable FSI optimizations in tooling

13aa2e0

Remove extra type-check function

2368052

TIHan changed the title ~~[WIP][Tooling Optimization] Stop background type-checking implementation files if signature file exists~~ [WIP][Tooling Optimization] Stop background type-checking on implementation files if signature files exist Sep 28, 2020

TIHan changed the title ~~[WIP][Tooling Optimization] Stop background type-checking on implementation files if signature files exist~~ [WIP][Tooling Optimization] Prevent background type-checking on implementation files if signature files exist Sep 29, 2020

TIHan added 24 commits September 29, 2020 18:27

Not working but initial work to lazily eval

300664c

some work

f158eaa

simplify

e83ebda

simplify again

62c0519

Using Async

00a35bc

work

db4f018

work

92c7cc8

builds

4c3c258

almost there

46b1117

Minor change

33481c2

minor refactor

315be1f

working, but not quite optimized

116b0b7

Fix dependency files

3fa36f6

Refactor types

79db9ed

Added 'enableLazyTypeChecking'.

8350e37

Enable lazy type checking in VS

75f8099

Added documentation

7be6e07

Added SyntaxTreeAccumulator

57611bc

prevent parsing impl files if possible

29d25f2

Trying to fix tests

be77e72

Refactor

6d29b09

Throw if enablePartialChecking and keepAssemblyContents are both enabled

ef9149d

minor comment update

ab54e62

minor refactor

8f16c45