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

improve queues, (one effect: improve code completion) #4310

Merged
merged 8 commits into from
Jan 4, 2021
Merged

improve queues, (one effect: improve code completion) #4310

merged 8 commits into from
Jan 4, 2021

Conversation

an-dr-eas-k
Copy link
Contributor

@an-dr-eas-k an-dr-eas-k commented Dec 21, 2020

After updating to the recent 1.23.x versions I had severe performance issues on code completion.

I investigated that and made the following changes:

  • Completion, CompletionResolve, and AutoComplete should be equally prioritized (add to normal queue)
  • remove request from _waiting queue when cancelRequest is called and call onError on the request to reject() any awaited promises.
  • add logging (debugMode=true in main.ts) for canceled requests and add logging details to RequestQueue internals.

The second bullet point addresses a deferred queue that stays always full when requests are cancelled. A symptom for that are many log entries of the kind:

Problem invoking 'GetCodeActions' on OmniSharp server: Error: Pending request cancelled: /v2/getcodeactions

@codecov
Copy link

codecov bot commented Dec 21, 2020

Codecov Report

Merging #4310 (aa6fe5e) into master (687fe0e) will decrease coverage by 0.00%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4310      +/-   ##
==========================================
- Coverage   86.01%   86.01%   -0.01%     
==========================================
  Files          60       60              
  Lines        1859     1866       +7     
  Branches      215      216       +1     
==========================================
+ Hits         1599     1605       +6     
  Misses        200      200              
- Partials       60       61       +1     
Flag Coverage Δ
integration 100.00% <ø> (ø)
unit 86.01% <93.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/omnisharp/protocol.ts 79.28% <ø> (-0.15%) ⬇️
src/observers/OmnisharpDebugModeLoggerObserver.ts 86.27% <85.71%> (-0.96%) ⬇️
src/omnisharp/EventType.ts 100.00% <100.00%> (ø)
src/omnisharp/loggingEvents.ts 98.04% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 687fe0e...aa6fe5e. Read the comment docs.

@an-dr-eas-k
Copy link
Contributor Author

For me, especially the other change, calling dequeue when a request is canceled is most important. Please note the log entries that I entered to debug the behaviour in 954ce95. I found out, the deferred queue is considered full just when two requests are canceled but remain in the _waiting queue forever. Then the queue does no longer accept further requests. This especially had an impact on code completion in the last versions. But also, Unit-test classes loose the "RunTest" links, Code Structure gets lost in the outline panel and so on.

The code I am working on has lots of automapper statements which made trouble when calling codeactions couple of times that were subsequently canceled, some already _waiting. And suddenly all requests that are prioritized to deferred queue didn't reach out for the omnisharp server.

I am not sure, if adding the id to the requests is proper, but at least solves a quick handling in the dequeue method.

I think this fix addresses at least some of:
#4292
#4289
#4311
#4080
#4047
#4072
#3421
#4065

src/omnisharp/server.ts Outdated Show resolved Hide resolved
333fred
333fred previously approved these changes Dec 24, 2020
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Overall looks good. We can delete the autocomplete entry and the definition in protocol.ts as well.

@333fred 333fred dismissed their stale review December 30, 2020 20:33

Going to need to sit down and go through the new cancellation code changes to understand the flow.

@an-dr-eas-k
Copy link
Contributor Author

an-dr-eas-k commented Jan 1, 2021

Sure. I needed to do these adjustments because I realized the OmnisharpWorkspaceSymbolProvider (ctrl-t) also needs request.onError to cancel the awaited task when it is in the waiting queue.

Essentially what changed in the last 4 commits are two things:

  1. I added the OmnisharpServerRequestCancelled BaseEvent and did some small changes to other BaseEvents to get more logging for canceled requests in the output panel when debugMode=true in main.ts.
  2. I moved the request.onError call one level higher to be called independent of the queueStatus the request is in. Originally, onError was only called for requests in the _pending queue.

Cheers.

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Thanks. This is mostly looking good, just a couple of small pieces of feedback.

src/omnisharp/EventType.ts Outdated Show resolved Hide resolved
src/omnisharp/prioritization.ts Outdated Show resolved Hide resolved
src/omnisharp/loggingEvents.ts Outdated Show resolved Hide resolved
@an-dr-eas-k an-dr-eas-k requested a review from 333fred January 4, 2021 16:32
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

LGTM. @JoeRobich for a second review.

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

4 participants