-
Notifications
You must be signed in to change notification settings - Fork 419
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
Async diagnostics analyzer work queue #2351
Async diagnostics analyzer work queue #2351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@neoGeneva Thanks for this enhancement. Just a few nits and suggestions. We should be able to get this in for the next release.
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/AsyncAnalyzerWorkQueue.cs
Outdated
Show resolved
Hide resolved
tests/OmniSharp.Roslyn.CSharp.Tests/AsyncAnalyzerWorkerQueueFacts.cs
Outdated
Show resolved
Hide resolved
Hey @JoeRobich, thanks for reviewing these changes! I've been working on a separate version of this pull request locally, but it's been a while and I want to make sure that any bug fixes I might have are included this pull too, so once I've done that (and the changes you've requested here), I'll update my branch and let you know, probably later this week. |
Hi @neoGeneva, I am hoping to get a new release together next week and wanted to include these changes. Would it be ok if I updated this PR with my suggestions and take it as is? |
Hey @JoeRobich, I started to have a look but haven't had a chance to reply sorry! I didn't have any changes from my other branches, but I did notice that the wrong cancellation token is used in a couple of places after a merge from main (should be using the combined one.) |
Oops. I get that fixed up. Thanks @neoGeneva! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @neoGeneva!
Async Worker Queue Changes
Included in this pull requests is some changes to how the worker queue works for the CSharpDiagnosticWorkerWithAnalyzers. The motivation for this work is a couple of performance related issues with the current queue, that requires a new approach.
The main issues I aim to resolve are:
My fixes for these issues are:
Additional Notes: