Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debounce diagnostic requests #5089

Merged
merged 3 commits into from
Mar 13, 2022
Merged

Debounce diagnostic requests #5089

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0350ce3
Use debounce to limit how frequently we request diagnostics
JoeRobich Mar 3, 2022
6c306e8
Remove unnecessary timeouts in diagnosticProvider
JoeRobich Mar 4, 2022
786580c
Merge branch 'master' into debounce-diagnostic-requests
nohwnd Mar 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 69 additions & 76 deletions src/features/diagnosticsProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { isVirtualCSharpDocument } from './virtualDocumentTracker';
import { TextDocument } from '../vscodeAdapter';
import OptionProvider from '../observers/OptionProvider';
import { Subject, Subscription } from 'rxjs';
import { throttleTime } from 'rxjs/operators';
import { debounceTime } from 'rxjs/operators';
import { DiagnosticStatus } from '../omnisharp/protocol';
import { LanguageMiddlewareFeature } from '../omnisharp/LanguageMiddlewareFeature';

Expand Down Expand Up @@ -122,7 +122,7 @@ class DiagnosticsProvider extends AbstractSupport {
private _disposable: CompositeDisposable;
private _diagnostics: vscode.DiagnosticCollection;
private _validateCurrentDocumentPipe = new Subject<vscode.TextDocument>();
private _validateAllPipe = new Subject();
private _validateAllPipe = new Subject<string>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was useful for debugging to see what event was causing all diagnostics to be requested.

private _analyzersEnabled: boolean;
private _subscriptions: Subscription[] = [];
private _suppressHiddenDiagnostics: boolean;
Expand All @@ -136,26 +136,24 @@ class DiagnosticsProvider extends AbstractSupport {
this._suppressHiddenDiagnostics = vscode.workspace.getConfiguration('csharp').get('suppressHiddenDiagnostics', true);

this._subscriptions.push(this._validateCurrentDocumentPipe
.asObservable()
.pipe(throttleTime(750))
.subscribe(async x => await this._validateDocument(x)));
.pipe(debounceTime(750))
.subscribe(x => this._validateDocument(x)));

this._subscriptions.push(this._validateAllPipe
.asObservable()
.pipe(throttleTime(3000))
.subscribe(async () => {
.pipe(debounceTime(3000))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throttle and debounce work similarly. Throttle fires on first request then ignores requests for a set amount of time. Debounce will wait for incoming requests to settle down before firing. This is an important distinction because we would want to wait for a users to be finished editing a document before requesting diagnostics.

.subscribe(reason => {
if (this._validationAdvisor.shouldValidateAll()) {
await this._validateEntireWorkspace();
this._validateEntireWorkspace();
}
else if (this._validationAdvisor.shouldValidateFiles()) {
await this._validateOpenDocuments();
this._validateOpenDocuments();
}
}));


this._disposable = new CompositeDisposable(this._diagnostics,
this._server.onPackageRestore(() => this._validateAllPipe.next(), this),
this._server.onProjectChange(() => this._validateAllPipe.next(), this),
this._server.onPackageRestore(() => this._validateAllPipe.next("onPackageRestore"), this),
this._server.onProjectChange(() => this._validateAllPipe.next("onProjectChanged"), this),
this._server.onProjectDiagnosticStatus(this._onProjectAnalysis, this),
vscode.workspace.onDidOpenTextDocument(event => this._onDocumentOpenOrChange(event), this),
vscode.workspace.onDidChangeTextDocument(event => this._onDocumentOpenOrChange(event.document), this),
Expand Down Expand Up @@ -208,12 +206,13 @@ class DiagnosticsProvider extends AbstractSupport {
// This check is just small perf optimization to reduce queries
// for omnisharp with analyzers (which has event to notify about updates.)
if (!this._analyzersEnabled) {
this._validateAllPipe.next();
this._validateAllPipe.next("onDocumentOpenOrChange");
}
}

private _onProjectAnalysis(event: protocol.ProjectDiagnosticStatus) {
if (event.Status == DiagnosticStatus.Ready) {
if (event.Status == DiagnosticStatus.Ready &&
event.ProjectFilePath === "(100 %)") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are currently getting Ready status updates from O# with "(0 %)" regularly. This was causing us to continually request updated diagnostics.

this._validateAllPipe.next();
}
}
Expand All @@ -224,54 +223,50 @@ class DiagnosticsProvider extends AbstractSupport {
}
}

private _validateDocument(document: vscode.TextDocument): NodeJS.Timeout {
private async _validateDocument(document: vscode.TextDocument) {
if (!this._validationAdvisor.shouldValidateFiles()) {
return;
}

return setTimeout(async () => {
let source = new vscode.CancellationTokenSource();
try {
let value = await serverUtils.codeCheck(this._server, { FileName: document.fileName }, source.token);
let quickFixes = value.QuickFixes;
// Easy case: If there are no diagnostics in the file, we can clear it quickly.
if (quickFixes.length === 0) {
if (this._diagnostics.has(document.uri)) {
this._diagnostics.delete(document.uri);
}

return;
}
// No problems published for virtual files
if (isVirtualCSharpDocument(document)) {
return;
}

// No problems published for virtual files
if (isVirtualCSharpDocument(document)) {
return;
let source = new vscode.CancellationTokenSource();
try {
let value = await serverUtils.codeCheck(this._server, { FileName: document.fileName }, source.token);
let quickFixes = value.QuickFixes;
// Easy case: If there are no diagnostics in the file, we can clear it quickly.
if (quickFixes.length === 0) {
if (this._diagnostics.has(document.uri)) {
this._diagnostics.delete(document.uri);
}

// (re)set new diagnostics for this document
let diagnosticsInFile = this._mapQuickFixesAsDiagnosticsInFile(quickFixes);

this._diagnostics.set(document.uri, diagnosticsInFile.map(x => x.diagnostic));
}
catch (error) {
return;
}
}, 2000);

// (re)set new diagnostics for this document
let diagnosticsInFile = this._mapQuickFixesAsDiagnosticsInFile(quickFixes);

this._diagnostics.set(document.uri, diagnosticsInFile.map(x => x.diagnostic));
}
catch (error) {
return;
}
}

// On large workspaces (if maxProjectFileCountForDiagnosticAnalysis) is less than workspace size,
// diagnostic fallback to mode where only open documents are analyzed.
private _validateOpenDocuments(): NodeJS.Timeout {
return setTimeout(async () => {
for (let editor of vscode.window.visibleTextEditors) {
let document = editor.document;
if (this.shouldIgnoreDocument(document)) {
continue;
}

await this._validateDocument(document);
private async _validateOpenDocuments() {
for (let editor of vscode.window.visibleTextEditors) {
let document = editor.document;
if (this.shouldIgnoreDocument(document)) {
continue;
}
}, 3000);

await this._validateDocument(document);
}
}

private _mapQuickFixesAsDiagnosticsInFile(quickFixes: protocol.QuickFix[]): { diagnostic: vscode.Diagnostic, fileName: string }[] {
Expand All @@ -280,41 +275,39 @@ class DiagnosticsProvider extends AbstractSupport {
.filter(diagnosticInFile => diagnosticInFile !== undefined);
}

private _validateEntireWorkspace(): NodeJS.Timeout {
return setTimeout(async () => {
let value = await serverUtils.codeCheck(this._server, { FileName: null }, new vscode.CancellationTokenSource().token);
private async _validateEntireWorkspace() {
let value = await serverUtils.codeCheck(this._server, { FileName: null }, new vscode.CancellationTokenSource().token);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use set Timeouts the debounce has already waited to execute this code.


let quickFixes = value.QuickFixes
.sort((a, b) => a.FileName.localeCompare(b.FileName));
let quickFixes = value.QuickFixes
.sort((a, b) => a.FileName.localeCompare(b.FileName));

let entries: [vscode.Uri, vscode.Diagnostic[]][] = [];
let lastEntry: [vscode.Uri, vscode.Diagnostic[]];
let entries: [vscode.Uri, vscode.Diagnostic[]][] = [];
let lastEntry: [vscode.Uri, vscode.Diagnostic[]];

for (let diagnosticInFile of this._mapQuickFixesAsDiagnosticsInFile(quickFixes)) {
let uri = vscode.Uri.file(diagnosticInFile.fileName);
for (let diagnosticInFile of this._mapQuickFixesAsDiagnosticsInFile(quickFixes)) {
let uri = vscode.Uri.file(diagnosticInFile.fileName);

if (lastEntry && lastEntry[0].toString() === uri.toString()) {
lastEntry[1].push(diagnosticInFile.diagnostic);
} else {
// We're replacing all diagnostics in this file. Pushing an entry with undefined for
// the diagnostics first ensures that the previous diagnostics for this file are
// cleared. Otherwise, new entries will be merged with the old ones.
entries.push([uri, undefined]);
lastEntry = [uri, [diagnosticInFile.diagnostic]];
entries.push(lastEntry);
}
if (lastEntry && lastEntry[0].toString() === uri.toString()) {
lastEntry[1].push(diagnosticInFile.diagnostic);
} else {
// We're replacing all diagnostics in this file. Pushing an entry with undefined for
// the diagnostics first ensures that the previous diagnostics for this file are
// cleared. Otherwise, new entries will be merged with the old ones.
entries.push([uri, undefined]);
lastEntry = [uri, [diagnosticInFile.diagnostic]];
entries.push(lastEntry);
}
}

// Clear diagnostics for files that no longer have any diagnostics.
this._diagnostics.forEach((uri) => {
if (!entries.find(tuple => tuple[0].toString() === uri.toString())) {
this._diagnostics.delete(uri);
}
});
// Clear diagnostics for files that no longer have any diagnostics.
this._diagnostics.forEach((uri) => {
if (!entries.find(tuple => tuple[0].toString() === uri.toString())) {
this._diagnostics.delete(uri);
}
});

// replace all entries
this._diagnostics.set(entries);
}, 3000);
// replace all entries
this._diagnostics.set(entries);
}

private _asDiagnosticInFileIfAny(quickFix: protocol.QuickFix): { diagnostic: vscode.Diagnostic, fileName: string } {
Expand Down