AsyncLocal part 2 - replace all ThreadStatics and remove NodeCode and node CE

[majocha]

This supersedes or follows up on #16602

Introduction of Transparent Compiler revealed some instability w.r.t. DiagnosticsThreadStatics:
#16576 (comment)
#16576
#16589

Turns out we used [<ThreadStatic>] to keep a compilation-global DiagnosticsLogger, and a custom NodeCode CE to flow it across threads during execution.
AsyncLocal is a BCL class that provides exactly that functionality: flowing state along the async execution path.

This replaces the ThreadStatic diagnostics state and Cancellable state with AsyncLocal.

NodeCode<'T> is no longer needed, so we remove it.

However NodeCode had some methods with subtly different behavior than Async.
Those are reproduced as Async type extensions in BuildGraph.fs.

• NodeCode.Sequential ->Async.SequentialImmediate. (Because normal Async.Sequential queues tasks in the thread pool).
• NodeCode.RunImmediateWithoutCancellation ->Async.RunImmediateWithoutCancellation.

Some NodeCode methods effectively created a compilation scope, restoring previous global DiagnosticsLogger on finish:

• NodeCode.FromCancellable
• NodeCode.AwaitAsync
• NodeCode.AwaitTask

This doesn't seem to be covered by the test suite at the moment. (All the tests pass with or without) but the following extensions wrap the computation in use _ = new CompilationGlobalsScope(... preserving and restoring globals:

• Async.FromCancellableWithScope
• Async.CompilationScope

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/8.0.300.md
vsintegration/src	docs/release-notes/.VisualStudio/17.10.md

[majocha]

Yes this is still stuck on the same problem in CI.
I looked for performance regressions, even tried to rewrite the locking in BuildGraph (cause maybe just maybe it exhausts allowed handles when executing those 10000 simultanous GetOrComputeValue).

It looks like it freezes on a certain test in Linux / MacOS and something wierd also happens on Windows.

ETA: The problem was Async.Sequential queues work in threadpool and this somehow causes a deadlock (probably around tcImportsLock) in some of fsharpqa Import tests involving fsi and #r requiring a dll.

[majocha]

Where can I find the test that outputs stuff like

Import (ReferenceArchAndPlatformArchMismatch: anycpu/anycpu) -- passed
Import (ReferenceArchAndPlatformArchMismatch: anycpu/x86) -- passed
Import (ReferenceArchAndPlatformArchMismatch: anycpu/x64) -- passed
Import (ReferenceArchAndPlatformArchMismatch: anycpu/Itanium) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x86/anycpu) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x86/x86) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x86/x64) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x86/Itanium) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x64/anycpu) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x64/x86) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x64/x64) -- passed
Import (ReferenceArchAndPlatformArchMismatch: x64/Itanium) -- passed
Import (ReferenceArchAndPlatformArchMismatch: Itanium/anycpu) -- passed
Import (ReferenceArchAndPlatformArchMismatch: Itanium/x86) -- passed
Import (ReferenceArchAndPlatformArchMismatch: Itanium/x64) -- passed
Import (ReferenceArchAndPlatformArchMismatch: Itanium/Itanium) -- passed

?

OK I got it, it's in FsharpQA env.lst. Hmmm.

[vzarytovskii]

@majocha if you'd like to, we can merge the first one, and then postpone this one a bit, I can go and try continue replacing with cold task-based one.

[majocha]

@vzarytovskii I'm fine with it. I think as is, it still improves things with Transparent Compiler.

[majocha]

OK, so this is green.

It appears all of the problems with this stemmed from my oversight.

NodeCode had subtly different behavior for functions like Sequential. SImply replacing it with Async.Sequential introduced some weird test failures that made me scratch my head for a week.

@vzarytovskii I think we can after all take this PR in favor of the #16602

I'll iron things out later. The diff is not small but reasonably easy to review.

[majocha]

Looks like it's still good. I installed the above fix in VS and Transparent Compiler works.

[majocha]

Another diagnostics push to null, find usages with Transparent Compiler in VS:

at FSharp.Compiler.DiagnosticsLogger.UninitializedDiagnosticsLogger@347.DiagnosticSink(PhasedDiagnostic diagnostic, FSharpDiagnosticSeverity severity) in E:\repos\fsharp\src\Compiler\Facilities\DiagnosticsLogger.fs:line 349
at FSharp.Compiler.CompilerOptions.reportDeprecatedOption@367-1.Invoke(FSharpOption1 errOpt) in E:\repos\fsharp\src\Compiler\Driver\CompilerOptions.fs:line 369 at FSharp.Compiler.CompilerOptions.attempt@373.Invoke(FSharpList1 l) in E:\repos\fsharp\src\Compiler\Driver\CompilerOptions.fs:line 379
at FSharp.Compiler.CompilerOptions.processArg@333.Invoke(FSharpList1 args) in E:\repos\fsharp\src\Compiler\Driver\CompilerOptions.fs:line 503 at FSharp.Compiler.CompilerOptions.ParseCompilerOptions(FSharpFunc2 collectOtherArgument, FSharpList1 blocks, FSharpList1 args) in E:\repos\fsharp\src\Compiler\Driver\CompilerOptions.fs:line 506
at FSharp.Compiler.CompilerOptions.ApplyCommandLineArgs(TcConfigBuilder tcConfigB, FSharpList1 sourceFiles, FSharpList1 argv) in E:\repos\fsharp\src\Compiler\Driver\CompilerOptions.fs:line 2317

I think there's no DiagnosticsScope here:

fsharp/src/Compiler/Service/TransparentCompiler.fs

Lines 641 to 643 in 3ac064e

	// Apply command-line arguments and collect more source files if they are in the arguments
	let sourceFilesNew =
	ApplyCommandLineArgs(tcConfigB, projectSnapshot.SourceFileNames, commandLineArgs)

[0101]

Another diagnostics push to null, find usages with Transparent Compiler in VS:

Hmm, it's a question what should we do in that case.

I suppose it would be nice when you search for usages, that triggers a check of some file which produces diagnostics, for them to show up in the IDE. I don't even know if we can do that in current VS extension. But eventually this could go to LSP push diagnostics.

For now we can probably just add a note about it and rely on your fix with discard error logger.

[majocha]

For now we can probably just add a note about it and rely on your fix with discard error logger.

Hmm. This one doesn't seem to go through the cache.

[0101]

Oh yeah, here where it says we might need to deal with exceptions? 😅 I guess we do...

fsharp/src/Compiler/Service/TransparentCompiler.fs

Lines 1801 to 1802 in 3ac064e

	// TODO: might need to deal with exceptions here:
	let tcConfigB, sourceFileNames, _ = ComputeTcConfigBuilder projectSnapshot

[majocha]

We can do for now:

            // TODO: might need to deal with exceptions here:
            let tcConfigB, sourceFileNames, _ =
                DiagnosticsLogger.suppressErrorReporting <| fun () ->
                    ComputeTcConfigBuilder projectSnapshot

[majocha]

Probably these could be just utility functions in DiagnosticsLogger.fs:

• Async.FromCancellableWithScope
• Async.CompilationScope

As they have nothing to do with Async in general.

Also, naming is hard.

[majocha]

A quick TransparentCompilerBenchmark shows the performance is mostly unchanged.

asynclocal
| Method          | UseTransparentCompiler | ProjectType     | Mean       | Error     | StdDev    | Gen0       | Completed Work Items | Lock Contentions | Gen1      | Allocated |
|---------------- |----------------------- |---------------- |-----------:|----------:|----------:|-----------:|---------------------:|-----------------:|----------:|----------:|
| ExampleWorkflow | False                  | DependencyChain | 3,045.4 ms |  66.11 ms |  10.23 ms | 19000.0000 |             157.0000 |                - | 2000.0000 |   5.62 GB |
| ExampleWorkflow | False                  | DependentGroups | 3,050.4 ms |  43.11 ms |   6.67 ms | 19000.0000 |             150.0000 |                - | 2000.0000 |   5.58 GB |
| ExampleWorkflow | False                  | ParallelGroups  | 3,286.8 ms |  55.72 ms |   8.62 ms | 20000.0000 |             151.0000 |                - | 3000.0000 |   5.79 GB |
| ExampleWorkflow | True                   | DependencyChain | 2,287.5 ms | 190.75 ms |  10.46 ms |  2000.0000 |             862.0000 |                - | 1000.0000 |   2.84 GB |
| ExampleWorkflow | True                   | DependentGroups |   977.4 ms | 704.55 ms | 109.03 ms |  2000.0000 |             824.0000 |                - | 1000.0000 |   2.75 GB |
| ExampleWorkflow | True                   | ParallelGroups  |   909.6 ms | 660.61 ms | 102.23 ms |  2000.0000 |             859.0000 |                - | 1000.0000 |   2.98 GB |

main
| Method          | UseTransparentCompiler | ProjectType     | Mean       | Error    | StdDev    | Gen0       | Completed Work Items | Lock Contentions | Gen1      | Allocated |
|---------------- |----------------------- |---------------- |-----------:|---------:|----------:|-----------:|---------------------:|-----------------:|----------:|----------:|
| ExampleWorkflow | False                  | DependencyChain | 3,496.0 ms | 851.2 ms | 131.73 ms | 19000.0000 |             156.0000 |                - | 2000.0000 |   5.61 GB |
| ExampleWorkflow | False                  | DependentGroups | 3,330.6 ms | 196.5 ms |  30.40 ms | 19000.0000 |             151.0000 |                - | 2000.0000 |   5.58 GB |
| ExampleWorkflow | False                  | ParallelGroups  | 3,379.3 ms | 410.1 ms |  63.46 ms | 20000.0000 |             151.0000 |                - | 4000.0000 |   5.78 GB |
| ExampleWorkflow | True                   | DependencyChain | 2,328.7 ms | 493.9 ms |  27.07 ms |  2000.0000 |             839.0000 |                - | 1000.0000 |   2.84 GB |
| ExampleWorkflow | True                   | DependentGroups |   934.5 ms | 583.5 ms |  90.30 ms |  2000.0000 |             868.0000 |                - | 1000.0000 |   2.74 GB |
| ExampleWorkflow | True                   | ParallelGroups  |   951.4 ms | 799.7 ms | 123.76 ms |  2000.0000 |             901.0000 |                - | 1000.0000 |   2.98 GB |

[0101]

We should probably figure out some more testing scenarios before we merge this.

[majocha]

Yep, also there's a fail in CI just now. Either a bad merge or indicative of something else.

[majocha]

I have a wacky idea. We could try to record changes of global (threadstatic) diagnostic logger from some compiler runs to get some kind of baseline. But that would not be deterministic. Not in a linear way, we would have to record all the forks in execution anytime there is some parallelization. That would give us some kind of tree-like baseline to compare against.

Another, simpler idea is to start clean from main, put the AsyncLocal state alongside the current ThreadStatic, kind of shadowing it, and just throw or assert whenever they diverge. this one is easier. I kind of have an idea how to do it.
Something like this: https://github.com/majocha/fsharp/tree/asynclocal-compare

[0101]

Good idea. And you pretty much have it ready. Please try it and let us know. I think we can just keep that in a separate branch.

[majocha]

I think we could make a draft and use that branch to systematically weed out all the weirdness, nulls etc. Eventually when all the tests pass and it runs without throwing assertions we'll know we're good.

[majocha]

See #16701