run analyzers on multiple threads if allowed to

[TomasEkeli]

fixes #2241

background

running analyzers is is currently sequential and can take a long time on big projects. this change can help by running
in parallel, particularly if the machine has many cores.

configuration

introduces a new option the user can set in omnisharp.json: RoslynExtensionsOptions.DiagnosticWorkersThreadCount.

• controls how many threads the diagnostic workers will use for analysis. this allows users to control how much of their available compute they want to give the analysers access to.
• any value up (exclusive) the number of cores on their computer would be good.
• setting this to 1 will make the analysis run in one worker (i.e. sequentially, like before this patch)
• not setting this value will default to using 75% of the available cores, or 1 if that is all that is available

observability

the analysers report their status as a percentage done of the total number of documents to analyse. this is done instead of listing the currently analysed project, and gives the user a better experience. thanks to @DaRosenberg for this, and help throughout!

in my experience this decreases the wall-time of analysers by a good amount at the expense of running the users' computer hotter. exposing it as a configuration allows the users to make their own trade-off.

documentation

the documentation for omnisharp.json in the wiki should be updated once this is merged.

[dnfadmin]

All CLA requirements met.

[DavidZidar]

Seems like a simple solution, I think it might work. When I did my quick hack however I did get some timeout errors, have you noticed anything like that? I probably got timeouts because I was going full blast with no "throttler" though.

Also, there's a similar foreach loop here, do you think it might need the same treatment?

omnisharp-roslyn/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs

Lines 149 to 157 in 6a5ccc4

	foreach (var document in documents)
	{
	if (document?.Project?.Name == null)
	continue;
	var projectName = document.Project.Name;
	var diagnostics = await GetDiagnosticsForDocument(document, projectName);
	results.Add(new DocumentDiagnostics(document.Id, document.FilePath, document.Project.Id, document.Project.Name, diagnostics));
	}

[TomasEkeli]

Seems like a simple solution, I think it might work. When I did my quick hack however I did get some timeout errors, have you noticed anything like that? I probably got timeouts because I was going full blast with no "throttler" though.

Also, there's a similar foreach loop here, do you think it might need the same treatment?

omnisharp-roslyn/src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorker.cs

Lines 149 to 157 in 6a5ccc4

	foreach (var document in documents)
	{
	if (document?.Project?.Name == null)
	continue;
	var projectName = document.Project.Name;
	var diagnostics = await GetDiagnosticsForDocument(document, projectName);
	results.Add(new DocumentDiagnostics(document.Id, document.FilePath, document.Project.Id, document.Project.Name, diagnostics));
	}

nice catch, I'll look into it

[DavidZidar]

I just pulled your branch and tried it out and it is indeed much faster, but I'm still getting the timeout errors I mentioned and tested this on a small/medium sized project on a 16 core Ryzen 5950X.

They look like this in the OmniSharp log (I truncated the list of analyzers because it is very long):

[fail]: OmniSharp.Roslyn.CSharp.Services.Diagnostics.CSharpDiagnosticWorkerWithAnalyzers
        Analysis of document SomeController.cs failed or cancelled by timeout: The operation was canceled., analysers: Microsoft.CodeAnalysis.UseSystemHashCode.UseSystemHashCodeDiagnosticAnalyzer, Microsoft.CodeAnalysis.UseExplicitTupleName.UseExplicitTupleNameDiagnosticAnalyzer, Microsoft.CodeAnalysis.MakeFieldReadonly.MakeFieldReadonlyDiagnosticAnalyzer, Microsoft.CodeAnalysis.Formatting.FormattingDiagnosticAnalyzer, ...

The timeout seems to be this one which is only 10 seconds, I'm thinking the default value would have to be increased when running things in parallel. Maybe 30 seconds?

omnisharp-roslyn/src/OmniSharp.Shared/Options/RoslynExtensionsOptions.cs

Line 14 in 6a5ccc4

public int DocumentAnalysisTimeoutMs { get; set; } = 10 * 1000;

[TomasEkeli]

ah, I've had that at 30 seconds for a long time, that's probably why I wasn't seeing the timeouts.

is there a problem if we just increase the default to that?

[TomasEkeli]

i've set it to 30s default.

[DaRosenberg]

I opened an alternative PR #2312 with some further improvements upon this work.

[TomasEkeli]

can we get a review on this @filipw or someone with write access? just picked filip to mention as he's the top contributor and probably know who would be right.

[JoeRobich]

@TomasEkeli Can you update the PR description to describe the changes the code is currently making. I didn't see a new RoslynOption for number of analyzer threads for instance.

There are certainly multiple places we could add parallelism to the Analyzer runner. In AnalyzeDocument() we could set concurrentAnalysis to true when getting the CompilationWithAnalyzers. However, with that approach, we don't have any knobs to control how many resources it will consume. I think the approach in this PR is OK, since it gives O# the ability to decide how to use resources.

[TomasEkeli]

@TomasEkeli Can you update the PR description to describe the changes the code is currently making. I didn't see a new RoslynOption for number of analyzer threads for instance.

There are certainly multiple places we could add parallelism to the Analyzer runner. In AnalyzeDocument() we could set concurrentAnalysis to true when getting the CompilationWithAnalyzers. However, with that approach, we don't have any knobs to control how many resources it will consume. I think the approach in this PR is OK, since it gives O# the ability to decide how to use resources.

i've updated the pull-request description, @JoeRobich - it was a little outdated. sorry about that, and i hope it's clearer now.

[DaRosenberg]

@JoeRobich We did consider setting concurrentAnalysis to true as well, but decided not to because we found this mention:

https://www.csharpcodi.com/csharp-examples/Microsoft.CodeAnalysis.Diagnostics.EngineV2.DiagnosticIncrementalAnalyzer.CompilationManager.GetAnalyzerExceptionFilter(Project)/

// in IDE, we always set concurrentAnalysis == false otherwise, we can get into thread starvation due to
// async being used with syncronous blocking concurrency.

Not sure if that's applicable at all in the OmniSharp context given that it's in a separate process from the IDE, but decided not the risk it nonetheless.

[JoeRobich]

Thanks!

[JoeRobich]

CC: @filipw, in case you have any concerns

[filipw]

I think this should be OK because it is gated. Also, normally we hide such features behind feature flags, but since the entire analysis is behind a feature flag already, it is fine. I am a bit worried that it can lead to state corruption on the client side but we can give it a try.

Thanks a lot for your work.

[TomasEkeli]

i'v updated the wiki

[filipw]

I am a bit worried that it can lead to state corruption on the client side but we can give it a try.

Unfortunately, looking at the feedback in dotnet/vscode-csharp#5017 from users using the latest build from master, it seems like there is indeed a regression where diagnostics retrieval crashes with a null reference.

[DaRosenberg]

@filipw I can take a look later tonight. What's the correct process here, open another PR against master with the fix?

[filipw]

thanks, yes that is correct. Every merge to master is published as beta prerelease and users who have "omnisharp.path": "latest" set in their VS Code receive that build then on next VS Code restart. This usually allows catching regressions early 🙂

[DavidZidar]

@filipw I'm fairly sure I found the issue and opened a PR with a fix #2333

[DaRosenberg]

@DavidZidar Yep, that's the same improvement I had previously made in the CSharpDiagnosticWorkerWithAnalyzers when I started helping out on this PR. I wasn't aware that the CSharpDiagnosticWorker had been changed also, otherwise I would have done the same there..

Anyway, kudos for tracking it down and fixing! 👍🏻

[DavidZidar]

@DaRosenberg Ah, and a good improvement it was. :) I reviewed the code at the time but failed to consider concurrency. And thanks for the initial investigation on how to reproduce the issue, it really helped!