Skip to content

Implement a per interpreter language server cache #8815

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 28 commits into from
Dec 2, 2019

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Nov 27, 2019

For #8206

Change how language servers are started and shutdown. Instead of there being only one at a time, allow many based on the interpreter in use.

The crux of this change is in the activationService.ts, It is now also a cache (it kinda had one already) that can be queried to return a language server.

Each language server

  • Registers as VS code's language server (the same way this used to work)
  • Allows an internal caller to also talk directly to the server (the new ILanguageServer API)

The reason for this change is because Notebooks can select a different python to use (called a kernel) than what is currently selected in the python selection.

@codecov-io
Copy link

codecov-io commented Nov 27, 2019

Codecov Report

Merging #8815 into master will decrease coverage by 1.05%.
The diff coverage is 34.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8815      +/-   ##
==========================================
- Coverage   58.65%   57.59%   -1.06%     
==========================================
  Files         526      527       +1     
  Lines       27204    27124      -80     
  Branches     4059     4123      +64     
==========================================
- Hits        15956    15623     -333     
- Misses      10369    10617     +248     
- Partials      879      884       +5
Impacted Files Coverage Δ
src/client/providers/objectDefinitionProvider.ts 11.76% <ø> (-0.97%) ⬇️
src/client/datascience/types.ts 100% <ø> (ø) ⬆️
.../datascience/interactive-common/interactiveBase.ts 25.49% <ø> (+0.05%) ⬆️
src/client/ioc/types.ts 100% <ø> (ø) ⬆️
...science/jupyter/liveshare/guestJupyterExecution.ts 18.96% <0%> (ø) ⬆️
src/client/ioc/serviceManager.ts 35.48% <0%> (ø) ⬆️
src/client/providers/jediProxy.ts 23.37% <0%> (+0.17%) ⬆️
...active-common/intellisense/intellisenseDocument.ts 66.43% <0%> (-3.74%) ⬇️
src/client/datascience/serviceRegistry.ts 0% <0%> (ø) ⬆️
...t/activation/languageServer/languageServerProxy.ts 92.3% <100%> (ø)
... and 61 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 817199b...03c9869. Read the comment docs.

@rchiodo
Copy link
Author

rchiodo commented Nov 27, 2019

Actually I had another thought on my way home last night. I want to change the disconnect/reconnect pattern to activate/deactivate. Otherwise when we have a notebook switch kernels, we might end up activating a language server for VS code and have two activated at the same time (right now activate is called when the language server is created). I'm going to change activate to basically be reconnect, and have a 'start' on each language server which prepares it to be connected to VS code. Activate will then actually allow that language server to be the VS code language server. #Resolved

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Still got a lot to review...

import { noop } from '../common/utils/misc';
import { ILanguageServer, LanguageServerActivator } from './types';

export class RefCountedLanguageServer implements ILanguageServer {
Copy link

@DonJayamanne DonJayamanne Nov 27, 2019

Choose a reason for hiding this comment

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

You can use Proxy class instead. Avoids the need to implement each method. #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

Ah I thought there was some way to not have to implement each. Thanks will change.


In reply to: 351403997 [](ancestors = 351403997)

Copy link
Author

Choose a reason for hiding this comment

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

Proxy class is messing up tests. Don and I talked and this seemed like the least exposed way to implement the ref counting idea.


In reply to: 351423686 [](ancestors = 351423686,351403997)

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

:shipit:

@rchiodo rchiodo closed this Nov 27, 2019
@rchiodo rchiodo reopened this Nov 27, 2019
@rchiodo
Copy link
Author

rchiodo commented Dec 2, 2019

@DonJayamanne did you have any more changes?

@rchiodo rchiodo merged commit 751f5b2 into master Dec 2, 2019
@rchiodo rchiodo deleted the rchiodo/ls_per_kernel_2 branch December 2, 2019 20:20
@lock lock bot locked as resolved and limited conversation to collaborators Dec 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants