Incremental builder with async lazy evaluation / Reactor removal #11573

TIHan · 2021-05-15T02:03:00Z

The basic idea behind this is to make each file in the dependency graph within incremental builder be asynchronously lazily evaluated; meaning many calls to the same evaluation of a file will not block threads.

Removes the reactor thread entirely.
Removes implicit background building.
Removes the eventually builder.
Adds BuildGraph.

Majority of these changes do not change the signature of the public APIs, but may change the behavior slightly due to removing the implicit background building; it's more predictable that way.

This should also resolve:
#11569

…lder

…ry by default.

…naming from code review

dsyme

Notes from deep review and co-working with @TIHan

dsyme · 2021-06-04T16:50:51Z

src/fsharp/BuildGraph.fs

+    static member AwaitTask(task: Task) =
+        Node(wrapThreadStaticInfo(Async.AwaitTask task))
+
+    static member AwaitWaitHandle(waitHandle: WaitHandle) =


This is only for test purposes - label it as such or rename to AwaitWaitHandleForTesting

src/fsharp/BuildGraph.fsi

dsyme · 2021-06-04T16:58:27Z

src/fsharp/BuildGraph.fsi

+
+    static member RunImmediate : computation: NodeCode<'T> * ?ct: CancellationToken -> 'T
+
+    static member StartAsTask : computation: NodeCode<'T> * ?ct: CancellationToken -> Task<'T>


Rename to StartAsTask_ForTesting

dsyme · 2021-06-04T17:01:38Z

src/fsharp/BuildGraph.fsi

+[<AbstractClass;Sealed>]
+type NodeCode =
+
+    static member RunImmediate : computation: NodeCode<'T> * ?ct: CancellationToken -> 'T


At each callsite ct is None.

My preference: remove the cancellation token argument and rename to RunImmediateWithoutCancellation

Certainly make the ct argument non-optional as it's relaly important to know where we have non-cancellable code

dsyme · 2021-06-04T17:02:22Z

src/fsharp/BuildGraph.fsi

+
+    static member Sequential : computations: NodeCode<'T> seq -> NodeCode<'T []>
+
+    static member AwaitWaitHandle : waitHandle: WaitHandle -> NodeCode<bool>


Rename to AwaitWaitHandle_ForTesting

dsyme · 2021-06-04T17:08:37Z

src/fsharp/BuildGraph.fsi

+
+    member GetValue: unit -> NodeCode<'T>
+
+    member TryGetValue: unit -> 'T voption


Maybe rename to TryPeekValue, since no compute is done

dsyme · 2021-06-04T17:09:56Z

src/fsharp/BuildGraph.fsi

+    /// By default, 'retryCompute' is 'true'.
+    new : computation: NodeCode<'T> -> GraphNode<'T>
+
+    member GetValue: unit -> NodeCode<'T>


Maybe make the computational aspect of this clearer in the name, e.g. ComputeValue or GetOrComputeValue

dsyme · 2021-06-04T17:38:17Z

src/fsharp/CompilerImports.fs

-                RequireCompilationThread ctok // we don't want to do assembly resolution concurrently, we assume MSBuild doesn't handle this
-                TcConfig.TryResolveLibsUsingMSBuildRules (tcConfig, assemblyList, rangeStartup, ResolveAssemblyReferenceMode.ReportErrors)
+                // we don't want to do assembly resolution concurrently, we assume MSBuild doesn't handle this
+                lock assemblyResolutionGate (fun () ->


Here the ultimate need for a lock is the call to tcConfig.legacyReferenceResolver.Impl.Resolve

See comment above. Agreed this looks like a reasonable place to acquire AssemblyResolutionLockToken

dsyme · 2021-06-04T17:40:34Z

src/fsharp/CompilerImports.fs

-
+    static member BuildFrameworkTcImports (tcConfigP: TcConfigProvider, frameworkDLLs, nonFrameworkDLLs) =
+      node {
+        let ctok = CompilationThreadToken()


Change to AssumeCompilationThreadWithoutEvidence()

dsyme · 2021-06-04T20:35:38Z

src/fsharp/service/service.fsi

@@ -257,7 +257,7 @@ type public FSharpChecker =
    /// <param name="filename">The filename for the file.</param>
    /// <param name="options">The options for the project or script, used to determine active --define conditionals and other options relevant to parsing.</param>
    /// <param name="userOpName">An optional string used for tracing compiler operations associated with this request.</param>
-    member GetBackgroundCheckResultsForFileInProject: filename: string * options: FSharpProjectOptions * ?userOpName: string -> Async<FSharpParseFileResults * FSharpCheckFileResults>
+    member GetBackgroundCheckResultsForFileInProject: filename: string * options: FSharpProjectOptions * ?userOpName: string -> Async<(FSharpParseFileResults * FSharpCheckFileResults) option>


We discussed this - no need to catch OperationCanceledException, no need to return options

TIHan · 2021-06-07T02:26:41Z

Great, it passes. :)

dsyme

This is great, I did a careful third online review with @TIHan and we added documentation and made some fixes. There are things to be done as follow up

Using the "save" trigger for scripts is not enough - we should request possible reanalysis each time the script comes into focus, otherwise red-squigglies will not be cleared after background file changes unless the user actually edits and saves. Have sent a repro to Will, he'll do this separately
When it's in main we can do InlineIfLambda on the 'Cancellable' methods like 'bind'
Check some FCS callstacks in the debugger (post hoc is ok)
Document the API changes in the FCS release notes, particularly the removal of the background build

…net#11573) * Adding AsyncLazy * Trying to get rid of cancellable and eventually use in incrementalbuilder * remove a lot of use of eventually and cancellable in incremental build * trying * More refactoring * drastrically less use of ctok * fixing build * Using reactor for foreground checking * more cleanup * more cleanup * Properly set a background build * fixing build * fixing build * trying to fix it * Added Seq.foldAsync * Changes to return 'option' on some service APIs * fixing tests * fixing surface area test * reseting lazy async * minor name change * Trying to cleanup more incremental builder stuff * fixing a few bugs * trying to fix tests * More cleanup * fixing tests * Fixing build * comment updates and adding a finalize * Minor comment update * Using run synchronously so it can perform cancellation * cleanup * need to reply * Reverting back to the previous AsyncLazy behavior * Cleaning up asynclazy * More comment updates * Remove open * Adding constraint * Allow setting cultureinfo for async lazy * Minor update * re-using compilation globals scope correctly * Added foregroundCheckFileCacheAgent * Fixing deadlock * Minor fix * An incremental build will not invalidate itself if a reference has changed * Added 'autoInvalidateConfiguration' option to FSharpChecker. Added 'IsProjectInvalidated' as a service API. * Fixing builder * Fixing up-to-date check * Update surface area. Renamed IsProjectInvalidated to IsProjectReferencesInvalidated * fixing build * Trying to fix tests * skip test * minor cleanup * Updating surface area. Minor cleanup * Update surface area * Use the parseLock instead of a mailbox * Added AsyncLazy.fs and AsyncLazyTests.fs. Combined AsyncLazyWeak and AsyncLazy into one * More cleanup * Trying to minimize diff * Trying to minimize diff * Removing ctok in more places * Updating baseline. Removing Reactor.fs. * Added a comment * More cleanup * Fixing build * Fixing build * Fixing build * Updated baseline * Remove lock * Fixing tests * Trying to fix tests * Fixing tests * Revert that * Fixing tests * Fixing * Minor format change * Fixing tests * no gc.collect * Trying to fix tests * Trying to fix tests * Using logical * trying to fix tests * updating test sdk * Fixing tests. Reverting back test sdk * Fixing tests * Fixing tests * Fixing test * TcImports uses Async instead of Cancellable now * FrameworkImportCache is now will ensure it only computes once for key * Removing some dependency on ctok * Removed lock in compiler assert * Added AsyncErrorLogger * More work on error logger * more error logger work * Fixed build * Should be partly working * Cleaning up AsyncErrorLogger * minor change * minor fix for debug * Using assemblyResolutionGate to lock * fixing async lazy tests * do not make these type provider tests parallel * do not make these type provider tests parallel * make more tests non-parallelizable * Fixing test * Use getOrCreateBuilder * Make type provider tests non-parallel * Allow cancellation exception to be thrown when evaluating raw contents * Refactored AsyncLazy. Added GraphNode and LazyGraphNode. * Fixing GraphNode * Fixing tests. Adding stack trace info. * fixing build * using string * Reactoring build graph * More refactoring * Fixing build * Removing another mutable state * Bigger buffer time in tests * Ported AsyncLazy from Roslyn * Fixing build * Removed RequestCount * Removed Roslyn AsyncLazy * Skipping tests temporarily until linux passes. GraphNode now will retry by default. * slowly re-enable tests * Re-enable build graph tests * Remove Thread.Sleep in tests. Add timeout exceptions * TcImports locking, use lock/token methodology, remove Enventually, renaming from code review * fix build * fix build * Fixing build * Removed API changes that are returning 'option' in FCS * Allow single background check for a script * Fixing build * Fixing build * Better handling of scripts * fix taking semaphore atomically * comment * Check for task completed * Using explicit continuation options * use underscore Co-authored-by: Don Syme <donsyme@fastmail.com>

TIHan added 30 commits May 14, 2021 14:37

Adding AsyncLazy

ba0c05e

Trying to get rid of cancellable and eventually use in incrementalbui…

458c3fc

…lder

remove a lot of use of eventually and cancellable in incremental build

58949ac

trying

9bc6919

More refactoring

0cec5da

drastrically less use of ctok

78fbe4d

fixing build

2c57028

Using reactor for foreground checking

c8bd0a6

more cleanup

afc5da2

more cleanup

49f39b6

Properly set a background build

ac91f7f

fixing build

f128d56

fixing build

82a09a3

trying to fix it

fb4c274

Added Seq.foldAsync

10759ac

Changes to return 'option' on some service APIs

5051b9c

fixing tests

165c035

fixing surface area test

3c82b66

reseting lazy async

24d2a9f

minor name change

312577c

Trying to cleanup more incremental builder stuff

83e57e4

fixing a few bugs

f762278

trying to fix tests

c535671

More cleanup

88faacb

fixing tests

befd8e8

Fixing build

b82da8e

comment updates and adding a finalize

43682f7

Minor comment update

c19dcdd

Using run synchronously so it can perform cancellation

9b6b03a

cleanup

6d47c59

TIHan and others added 12 commits May 27, 2021 16:48

Merge branch 'main' into loop-fix

001b754

Fixing build

5a5ce4d

Removed RequestCount

928af7b

Removed Roslyn AsyncLazy

dde34bf

Skipping tests temporarily until linux passes. GraphNode now will ret…

9f46615

…ry by default.

slowly re-enable tests

4b44434

Re-enable build graph tests

b460498

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

fbc54cb

Remove Thread.Sleep in tests. Add timeout exceptions

31ae3f9

TcImports locking, use lock/token methodology, remove Enventually, re…

734cf42

…naming from code review

fix build

25fe917

fix build

3f49902

dsyme reviewed Jun 4, 2021

View reviewed changes

TIHan added 5 commits June 6, 2021 17:10

Fixing build

e864be7

Removed API changes that are returning 'option' in FCS

be60f1b

Allow single background check for a script

33ae186

Fixing build

50c5732

Fixing build

0f6c807

TIHan and others added 2 commits June 6, 2021 21:55

Better handling of scripts

b3df4a8

fix taking semaphore atomically

35353c8

dsyme approved these changes Jun 7, 2021

View reviewed changes

Don Syme and others added 4 commits June 7, 2021 18:31

comment

98b7d3f

Check for task completed

b18245c

Using explicit continuation options

bfc317a

use underscore

5982dbe

TIHan merged commit 200af47 into dotnet:main Jun 7, 2021

TIHan mentioned this pull request Aug 16, 2021

Plan: Modernizing F# Analysis #11976

Open

10 tasks

TIHan mentioned this pull request Nov 19, 2021

Performance decrease when running FSharpChecker.ParseAndCheckProject #12380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental builder with async lazy evaluation / Reactor removal #11573

Incremental builder with async lazy evaluation / Reactor removal #11573

TIHan commented May 15, 2021 •

edited

Loading

dsyme left a comment

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

dsyme Jun 4, 2021

TIHan commented Jun 7, 2021

dsyme left a comment


		static member RunImmediate : computation: NodeCode<'T> * ?ct: CancellationToken -> 'T

		static member StartAsTask : computation: NodeCode<'T> * ?ct: CancellationToken -> Task<'T>


		static member Sequential : computations: NodeCode<'T> seq -> NodeCode<'T []>

		static member AwaitWaitHandle : waitHandle: WaitHandle -> NodeCode<bool>


		member GetValue: unit -> NodeCode<'T>

		member TryGetValue: unit -> 'T voption

Incremental builder with async lazy evaluation / Reactor removal #11573

Incremental builder with async lazy evaluation / Reactor removal #11573

Conversation

TIHan commented May 15, 2021 • edited Loading

dsyme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TIHan commented Jun 7, 2021

dsyme left a comment

Choose a reason for hiding this comment

TIHan commented May 15, 2021 •

edited

Loading