run analyzers on multiple threads if allowed to #2285

TomasEkeli · 2021-12-02T20:45:03Z

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.

there is now a new option in the roslyn-extensions-option: ThreadsToUseForAnalyzers - which defaults to half the number of processors on the machine. there are two workers started, one for background and one for foreground. so this *might* actually use all the cores? in my usage it seems not to.

dnfadmin · 2021-12-02T20:45:17Z

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)); 
        
           }

src/OmniSharp.Shared/Options/RoslynExtensionsOptions.cs

Co-authored-by: David Zidar <[email protected]>

TomasEkeli · 2021-12-03T07:10:19Z

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

same logic as for the diagnostic worker with analyzers.

since it's used for diagnostic with and without analyzers the name should reflect this

DavidZidar · 2021-12-03T13:55:51Z

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 · 2021-12-03T15:32:01Z

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 · 2021-12-03T16:29:32Z

i've set it to 30s default.

i should never try to do this stuff from github

- Parallelize at the document level instead of only at the project level (so user benefits also with few projects but many files) - Since currently analyzing project no longer has any real meaning, add new status events to convey "analyzing n remaining files", updates every 50 documents - Retain old status events for backward compat with clients - Use 75% of available cores instead of half

DaRosenberg · 2021-12-19T02:00:00Z

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

hard-coding to a number of documents (e.g. 50 or 24) will either send updates very rarely giving large jumps, or very frequently (i.e. many times within one percentage-point). this change calculates how many documents correspond to 1%, and send an event every time that has been reached. if that is less than every 10 documents, it events on every tenth document. this avoids unnecessary updates and updates as often as relevant.

TomasEkeli · 2022-01-04T21:51:09Z

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 · 2022-01-12T21:03:47Z

@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 · 2022-01-12T22:16:50Z

@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 · 2022-01-13T10:34:16Z

@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 · 2022-01-18T23:04:38Z

CC: @filipw, in case you have any concerns

filipw · 2022-01-20T18:46:26Z

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 · 2022-01-23T18:33:03Z

i'v updated the wiki

filipw · 2022-01-24T18:35:52Z

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 · 2022-01-24T18:57:39Z

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

filipw · 2022-01-24T20:42:58Z

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 · 2022-01-24T23:46:40Z

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

DaRosenberg · 2022-01-24T23:57:20Z

@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 · 2022-01-25T00:03:27Z

@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!

DavidZidar reviewed Dec 3, 2021

View reviewed changes

src/OmniSharp.Shared/Options/RoslynExtensionsOptions.cs Outdated Show resolved Hide resolved

Update src/OmniSharp.Shared/Options/RoslynExtensionsOptions.cs

f445915

Co-authored-by: David Zidar <[email protected]>

TomasEkeli added 4 commits December 3, 2021 08:15

Merge branch 'master' into master

5733124

use the new name of the option

fbfd7c0

parallelize the csharp diagnostic worker

cb3a4fc

same logic as for the diagnostic worker with analyzers.

rename option

a7c8c04

since it's used for diagnostic with and without analyzers the name should reflect this

set default timeout wait time for analysis to 30s

145a904

TomasEkeli and others added 7 commits December 4, 2021 17:20

Merge branch 'master' into master

34f8150

Merge branch 'master' into master

1a5a5fe

really fix the build-error

01010e5

i should never try to do this stuff from github

Merge branch 'master' into master

88419d2

Merge branch 'OmniSharp:master' into master

280f4a7

Merge branch 'master' into master

f1440fd

DaRosenberg mentioned this pull request Dec 19, 2021

Adds parallel execution of background Roslyn analyzers #2312

Closed

DaRosenberg mentioned this pull request Dec 19, 2021

Analyzing projects is extremely slow compared to MSBuild or Visual Studio #2241

Closed

DaRosenberg and others added 2 commits December 19, 2021 14:19

Improved progress event code

1108e4c

TomasEkeli force-pushed the master branch from 2904424 to fb65560 Compare December 19, 2021 19:56

DaRosenberg and others added 4 commits December 19, 2021 23:31

Added parallelization for non-queue-based project analysis

5d7798f

Fixed a potential race condition in diagnostics emission

e982ab7

Removed duplicated code

d4a582f

totally unrelated, but irritating

fd25217

Merge branch 'master' into master

3bd9fd6

Merge branch 'master' into master

c191ee7

JoeRobich approved these changes Jan 13, 2022

View reviewed changes

Merge branch 'master' into master

21be466

JoeRobich merged commit b4042a7 into OmniSharp:master Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run analyzers on multiple threads if allowed to #2285

run analyzers on multiple threads if allowed to #2285

TomasEkeli commented Dec 2, 2021 •

edited

Loading

dnfadmin commented Dec 2, 2021 •

edited

Loading

DavidZidar left a comment

TomasEkeli commented Dec 3, 2021

DavidZidar commented Dec 3, 2021

TomasEkeli commented Dec 3, 2021

TomasEkeli commented Dec 3, 2021

DaRosenberg commented Dec 19, 2021

TomasEkeli commented Jan 4, 2022

JoeRobich commented Jan 12, 2022

TomasEkeli commented Jan 12, 2022 •

edited

Loading

DaRosenberg commented Jan 13, 2022

JoeRobich left a comment

JoeRobich commented Jan 18, 2022

filipw commented Jan 20, 2022

TomasEkeli commented Jan 23, 2022

filipw commented Jan 24, 2022

DaRosenberg commented Jan 24, 2022

filipw commented Jan 24, 2022

DavidZidar commented Jan 24, 2022

DaRosenberg commented Jan 24, 2022 •

edited

Loading

DavidZidar commented Jan 25, 2022

	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));
	}

run analyzers on multiple threads if allowed to #2285

run analyzers on multiple threads if allowed to #2285

Conversation

TomasEkeli commented Dec 2, 2021 • edited Loading

background

configuration

observability

documentation

dnfadmin commented Dec 2, 2021 • edited Loading

DavidZidar left a comment

Choose a reason for hiding this comment

TomasEkeli commented Dec 3, 2021

DavidZidar commented Dec 3, 2021

TomasEkeli commented Dec 3, 2021

TomasEkeli commented Dec 3, 2021

DaRosenberg commented Dec 19, 2021

TomasEkeli commented Jan 4, 2022

JoeRobich commented Jan 12, 2022

TomasEkeli commented Jan 12, 2022 • edited Loading

DaRosenberg commented Jan 13, 2022

JoeRobich left a comment

Choose a reason for hiding this comment

JoeRobich commented Jan 18, 2022

filipw commented Jan 20, 2022

TomasEkeli commented Jan 23, 2022

filipw commented Jan 24, 2022

DaRosenberg commented Jan 24, 2022

filipw commented Jan 24, 2022

DavidZidar commented Jan 24, 2022

DaRosenberg commented Jan 24, 2022 • edited Loading

DavidZidar commented Jan 25, 2022

TomasEkeli commented Dec 2, 2021 •

edited

Loading

dnfadmin commented Dec 2, 2021 •

edited

Loading

TomasEkeli commented Jan 12, 2022 •

edited

Loading

DaRosenberg commented Jan 24, 2022 •

edited

Loading