Skip to content

Rework ProcessScheduler #228

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

Merged
merged 20 commits into from
Apr 29, 2020
Merged

Rework ProcessScheduler #228

merged 20 commits into from
Apr 29, 2020

Conversation

david-driscoll
Copy link
Member

This is to rework the scheduler to achieve a few things:

  1. The code has always been a pain point, it works but it's hard to work on.
  2. We've never been able to put a hard cap on total number of parallel operations that can happen at a time (number of concurrent requests)

If you don't like observables cover your eyes.
If you like observables and know a slightly better way to fan-in fan-out let me know! 😄

@TylerLeonhardt what are your thoughts on making completion resolve handler parallel? Originally I had it serial because it can contain actions and edits but that would get invalidated by another document change anyway.

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #228 into master will increase coverage by 0.03%.
The diff coverage is 82.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
+ Coverage   43.65%   43.69%   +0.03%     
==========================================
  Files         360      361       +1     
  Lines        8045     8040       -5     
  Branches      992      985       -7     
==========================================
+ Hits         3512     3513       +1     
+ Misses       4237     4233       -4     
+ Partials      296      294       -2     
Impacted Files Coverage Δ
src/JsonRpc/Connection.cs 0.00% <0.00%> (ø)
src/Protocol/Document/Server/ICompletionHandler.cs 0.00% <ø> (ø)
src/Server/LanguageServer.cs 0.00% <0.00%> (ø)
src/Server/LanguageServerOptions.cs 0.00% <ø> (ø)
src/Server/LanguageServerOptionsExtensions.cs 0.00% <0.00%> (ø)
src/JsonRpc/ProcessScheduler.cs 98.61% <98.27%> (+11.11%) ⬆️
src/JsonRpc/IScheduler.cs 100.00% <100.00%> (ø)
src/JsonRpc/InputHandler.cs 86.77% <100.00%> (+0.07%) ⬆️
src/JsonRpc/RequestRouterBase.cs 72.06% <100.00%> (ø)
... and 1 more

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 a004474...457ce48. Read the comment docs.

@TylerLeonhardt
Copy link
Collaborator

well, if I hold down arrow, and go through my list of completions, that's a lot of stuff that would be happening serially.

In our case, it would be better parallel.. but you may want to ask @NTaylorMullen or others if that's ok.

@NTaylorMullen
Copy link

Does this only impact scenarios where you return isInComplete: true?

/cc @ToddGrun @jimmylewis @alexgav

@david-driscoll
Copy link
Member Author

@NTaylorMullen so whenever the editor highlights an completion item, resolve is called to get additional details. Partial lists are a different thing.

@NTaylorMullen
Copy link

Ah what would the potential drawbacks be of it being parallel?

@TylerLeonhardt
Copy link
Collaborator

Ah what would the potential drawbacks be of it being parallel?

I think @david-driscoll's issue was that it has the ability to change state I think...:

/**
	 * An optional command that is executed *after* inserting this completion. *Note* that
	 * additional modifications to the current document should be described with the
	 * additionalTextEdits-property.
	 */
	command?: Command;

This is found on CompletionItem. However, the Command is executed after it's accepted... I don't really see any issues making it parallel.

@NTaylorMullen
Copy link

I think @david-driscoll's issue was that it has the ability to change state I think...:

I see. Ya I wouldn't think that making completion resolve parallel would cause any issues. That being said, in the middle of a completion session I wouldn't expect many other events to be going through the language server either though so i'm not sure it being parallel would buy a whole lot either

@david-driscoll david-driscoll merged commit 954496d into master Apr 29, 2020
@david-driscoll david-driscoll deleted the fix/cancellation2 branch April 29, 2020 22:15
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.

3 participants