Analyzers being run sequentially unexpectedly

[dsyme]

This is a design issue about whether Roslyn analyzers are run sequentially or in parallel, especially in response to a "Reanalyze" event:

The issue is explained in these two threads:

• https://github.com/dotnet/roslyn/issues/19347#issuecomment-310225540
• https://github.com/Microsoft/visualfsharp/pull/3238#issuecomment-310254015

@Pilchie says

Tagging @heejaechang, but regarding running analyzers serially: IIRC this was a deliberate decision for in-proc analyzers, since running them in parallel ended up generating enough garbage that collections significantly impacted UI thread responsiveness while typing.

My reply:

@Pilchie @heejaechang Could we open a Roslyn issue about this? I think it's a significant issue. For F# we have more or less been assuming that all these analyzers get run in parallel on more or less every code edit (and get cancelled if another edit occurs). We put in some "async sleep" operations to effectively change their priority, delaying the low priority ones. Now that we understand that the document diagnostic analyzers are actually getting run serially in response to "Reanalyze" we will need to reconsider and scale back how much work we do in the analyzers.

[Pilchie]

I think one thing to consider here is whether we can get something like documentanalyzer that is syntax/semantic agnostic that can run in the OOP process, where we are more free to run things in parallel. Also tagging @mavasani and @jinujoseph.

@dsyme I don't think we'll be able to make a change here for analyzers in the VS process without a significant typing responsiveness regression though.

[dsyme]

@Pilchie We'd be happy with just having low/high priority flags on different individual analyzers.

Or just two kinds of DocumentDiagnosticAnalyzer: one low prority, one high priority

[heejaechang]

unfortunately, moving arbitrary thing to run in OOP is little bit tricky. they have to explicitly opt-in (such as making sure they can use whatever they depends on from OOP). if F# want to opt in, I can help you guys on how to do it. you guys don't need to live inside of Roslyn, LUT/Razor already install their own Vsix which load into OOP and share all data (such as workspace/solution) there.

Running analyzers concurrently in proc was never the behavior. I am kind of curious where you got that idea. anyway, with GC, I dont think that will ever happen.

I can think 3 ways to fix this either

1. opt into OOP (once you do, you can do more than analyzers though) or
2. just provide a way to set priority on Document/ProjectDiagnosticAnalyzer or
3. implement automatic ordering of analyzers in real time based on perf gathered

and these can be combined.

[vasily-kirichenko]

OK, this is a real problem in Visual F# Tools. The sequential loop that executes analyzers one by one is this (stateSet represents single analyzer) http://source.roslyn.io/#Microsoft.CodeAnalysis.Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,45:

var stateSets = _stateManager.GetOrUpdateStateSets(document.Project);
var analyzerDriverOpt = await _compilationManager.GetAnalyzerDriverAsync(document.Project, stateSets, cancellationToken).ConfigureAwait(false);

foreach (var stateSet in stateSets)
{
    var analyzer = stateSet.Analyzer;

    var result = await _executor.GetDocumentAnalysisDataAsync(analyzerDriverOpt, document, stateSet, kind, cancellationToken).ConfigureAwait(false);
    if (result.FromCache)
    {
        RaiseDocumentDiagnosticsIfNeeded(document, stateSet, kind, result.Items);
        continue;
    }

    // no cancellation after this point.
    var state = stateSet.GetActiveFileState(document.Id);
    state.Save(kind, result.ToPersistData());

    RaiseDocumentDiagnosticsIfNeeded(document, stateSet, kind, result.OldItems, result.Items);
}

It's simple.

My suggestion:

Please, provide a configuration option (in Roslyn API) that enables running document analyzers in parallel

(in incremental analyzing scenario, not when full project analyzed, it's a different code path).

[vasily-kirichenko]

/cc @Pilchie

[sharwell]

One of my relatively short term goals is eliminating the lack of parallelism in both the analyzers and the analyzer driver. If allocations are a problem, then we need to make the analysis allocate less, but making everything slow and sequential is just, well, bad.

The primary blocking item here is performance and scale testing infrastructure for analyzers and the analyzer driver. I believe this will be a major discussion point internally over the next week.

[heejaechang]

until we can prove running analyzer in parallel doesn't make VS unusable, this won't be happening. analyzer is not the only thing VS care. one feature shouldn't overrule all other features. (we already experiment with it and typing responsiveness perf got way worse than now)

[heejaechang]

I thought F#'s issue is lack of priority not the parallelism. didn't you guys already experiment and then conclude that delay or the concurrency is not actually problem but the order of analyzer being ran was the problem? (slow, heavy analyzer being ran before all other cheap fast analyzer?)

[heejaechang]

if you really want concurrency, then you should opt into OOP. analyzers are running concurrently in OOP already.

[vasily-kirichenko]

We cannot opt into OOP because it's a huge task. OK, if you so strict about allowing parallel running for F#, adding optional priority to analyzers would be great, this would allow us removing our artificial delays from our heavy-and-no-so-important analyzers.

[heejaechang]

@vasily-kirichenko I added a way to order analyzers.

by the way, I think it ("We cannot opt into OOP because it's a huge task.") is not fair that since it is hard for F# to move OOP, asking Roslyn to make analyzers run concurrently in in-proc. which we for same reason (huge task to make it possible in usable sense) explicitly cut it.

we explicitly created OOP for the people who want to run things concurrently without worrying too much of allocation impact on VS. it is running low priority to make sure it again won't bring VS down. we made sure it pause when things like building runs to make sure VS operates reasonably even with heavy work in OOP. if F# needs to run a work heavy enough that must be run concurrently and sequentially can't cut it. such work load should move to OOP, I believe.

since F# doesn't dominate Roslyn or VS experience, I think F# should go along with others so that overall experience of VS feels better not just F#.

[mavasani]

I agree with @heejaechang here. No matter how much we improve the performance of analyzer driver and/or analyzers, I don't believe we should run analyzers in parallel in the devenv process (in-proc). We have made multiple attempts at this and every time it has shown to increase allocations, GC, contentions, etc. and has affected Visual Studio performance. This might not be the case for F# right now, but implementing this request is opening the doors for similar issues in the future. We should move all possible background analysis to OOP.

[JarLob]

What about hanging analyzer that does not give a chance to other analyzers to run? #23879 Running them all at the same time may be not the best solution, but some UI indication is needed to notify a user that: a) some analyzer is still being running b) all registered analyzers didn't run yet

[heejaechang]

@JarLob for now, "..." in error list indicates that information. but probably we need to adapt "IVsTaskStatusCenterService" to show more useful data to users.