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.

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

Conversation

JoeRobich
Copy link
Member

May contribute to #5085

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.

}
}

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.

.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.

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

@JoeRobich JoeRobich marked this pull request as ready for review March 5, 2022 01:07
@JoeRobich
Copy link
Member Author

Please take a look @nohwnd

@lonix1
Copy link

lonix1 commented Mar 13, 2022

The latest version is completely unusable as everything is so slow.

I assume this fix will only be released next month. Until then, is there a prerelease version we can use? We'll provide feedback if it solves the problem.

@333fred
Copy link
Member

333fred commented Mar 13, 2022

Until then, is there a prerelease version we can use?

@lonix1 if you set your o# version to latest, it should pick up the prerelease build with this change as soon as it finishes building.

@filipw
Copy link
Contributor

filipw commented Mar 14, 2022

Until then, is there a prerelease version we can use?

@lonix1 if you set your o# version to latest, it should pick up the prerelease build with this change as soon as it finishes building.

This change will not show up when you set "omnisharp.path":"latest" since it's an extension-level change. It will require a new prerelease VSIX to be published here first https://github.com/OmniSharp/omnisharp-vscode/releases

@333fred
Copy link
Member

333fred commented Mar 14, 2022

Oh right, I forgot which repo I was on 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants