Send batch events for DiagnosticsUpdatedArgs

[sharwell]

No description provided.

[CyrusNajmabadi]

FWIW, with the intent being to entirely remove this ysstem, my preference would be to hold off on larger design changes here.

[sharwell]

@CyrusNajmabadi This change affects both IDiagnosticUpdateSource.DiagnosticsUpdated and IDiagnosticService.DiagnosticsUpdated. It is a very high impact change to improve the experience for anyone with either of these services enabled. Are there immediate plans to fully remove both of these?

[ToddGrun]

I've looked through this just a bit and it looks really nice. Do you have any idea on roughly how many diagnostic events (task creations) this prevents? @CyrusNajmabadi -- Is this worth pursuing, or are both of these services slated for removal soon?

[sharwell]

Do you have any idea on roughly how many diagnostic events (task creations) this prevents

We've seen some traces that suggest thousands or tens of thousands over a short period of time for some user scenarios.

[ToddGrun]

AnalyzerService.RaiseDiagnosticsUpdated(argsBuilder.ToImmutableAndClear());

Wanting to make sure this needs to be done inside the loop

[sharwell]

➡️ Yes, we don't want to delay the presentation of diagnostics from analyzer A until some other analyzer B completes. We only batch events in cases where multiple events are ready at the same time.

[ToddGrun]

Makes sense, thanks for the explanation

[ToddGrun]

Probably a dumb question, but would it make sense to try to utilize AsyncBatchingWorkQueue to allow these to get batched together if they do completely quickly? Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion? @mavasani?

[sharwell]

Probably a dumb question, but would it make sense to try to utilize AsyncBatchingWorkQueue to allow these to get batched together if they do completely quickly?

Potentially, yes. In DefaultDiagnosticAnalyzerService.cs I added a comment to that effect.

Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion?

I'm not sure, but it's outside the scope of this change.

[mavasani]

Another dumb question is why these are awaited serially, and not kicked off in a parallel fashion?

There is some history here. Before we moved core analyzer execution pieces to OOP, we wanted to make sure that we don't stress the devenv process by running analyzers concurrently in proc as this was part of continuous background analysis and led to high CPU usage, GC and UI delays. After moving the analyzers to OOP, we do execute all the analyzers in OOP process in parallel. Just the requests to fetch the analysis result still happens serially here, which can potentially be changed but will not likely lead to any significant performance benefit. Note that we do special case the compiler analyzer though and execute it in isolation upfront, rest of the analyzers execute concurrently. For example, see below:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor.cs

Lines 216 to 249 in 49e5f55

	private async Task<ImmutableArray<DiagnosticData>> GetSyntaxDiagnosticsAsync(DiagnosticAnalyzer analyzer, bool isCompilerAnalyzer, CancellationToken cancellationToken)
	{
	// PERF:
	// 1. Compute diagnostics for all analyzers with a single invocation into CompilationWithAnalyzers.
	// This is critical for better analyzer execution performance.
	// 2. Ensure that the compiler analyzer is treated specially and does not block on diagnostic computation
	// for rest of the analyzers. This is needed to ensure faster refresh for compiler diagnostics while typing.
	RoslynDebug.Assert(_compilationWithAnalyzers != null);
	RoslynDebug.Assert(_compilationBasedAnalyzersInAnalysisScope.Contains(analyzer));
	if (isCompilerAnalyzer)
	{
	if (AnalysisScope.TextDocument is not Document)
	{
	return ImmutableArray<DiagnosticData>.Empty;
	}
	return await GetCompilerAnalyzerDiagnosticsAsync(analyzer, AnalysisScope.Span, cancellationToken).ConfigureAwait(false);
	}
	if (_lazySyntaxDiagnostics == null)
	{
	using var _ = TelemetryLogging.LogBlockTimeAggregated(FunctionId.RequestDiagnostics_Summary, $"{nameof(GetSyntaxDiagnosticsAsync)}.{nameof(GetAnalysisResultAsync)}");
	var analysisScope = AnalysisScope.WithAnalyzers(_compilationBasedAnalyzersInAnalysisScope);
	var syntaxDiagnostics = await GetAnalysisResultAsync(analysisScope, cancellationToken).ConfigureAwait(false);
	Interlocked.CompareExchange(ref _lazySyntaxDiagnostics, syntaxDiagnostics, null);
	}
	return _lazySyntaxDiagnostics.TryGetValue(analyzer, out var diagnosticAnalysisResult)
	? diagnosticAnalysisResult.GetDocumentDiagnostics(AnalysisScope.TextDocument.Id, AnalysisScope.Kind)
	: ImmutableArray<DiagnosticData>.Empty;
	}

[CyrusNajmabadi]

Tough call here. I totally see the desire to resolve the problem with insanely bad task-chains and whatnot. But this is also fairly complex in an area where complexity has already been a challenge.

Overall it does look ok to me though. But i'd like at least one more pass from @mavasani to ensure this will be ok. If everyone feels reasonable about this, i'm ok moving forward.

[mavasani]

Thanks for the PR @sharwell. Overall, this looks very promising from performance perspective. I am definitely concerned if this degrades the user experience though. As we delay push notifications after all analyzers have finished executing, all error list refresh operations will be delayed. More specifically, my concern is that the compiler analyzer is generally one of the first ones to execute and we send out push notifications for it first to ensure compiler diagnostics are refreshed as soon as possible and not affected by slower analyzers - we have separate code paths in DocumentAnalysisExecutor to ensure that compiler analyzer is executed upfront as a single analyzer, whose execution and reporting does not get clubbed with other analyzers to help with faster refresh. We definitely need to ensure that this behavior is retained even after your change - maybe treat compiler analyzer specially and raise push requests for it as soon as they are available and don't add it to the batch. That likely makes the code lose readability, but I think we can't assess that unless you have addressed it (maybe it's not as widespread as I think).

Another concern here is that we plan to turn on LSP pull diagnostics by default soon enough, and this change will not get any dogfooding, unless someone explicitly turns off pull diagnostics. Any regression from this change might not get caught until after a release if and when customers try to switch back to non-LSP pull diagnostics mode.

I think both of these concerns are addressable though, especially if we feel this gives a large performance improvement.

[ToddGrun]

LGTM, I'll let Cyrus or Manish do the actual signoff though

[mavasani]

Looks great to me! This should definitely bring a good performance improvement - please do make sure to add some more comments and also locally verify the editing experience on RoslynDevHive to make sure we did not regress the user experience here. Thanks @sharwell!