Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Better progress reporting #96

Merged
merged 7 commits into from
Sep 17, 2018
Merged

Better progress reporting #96

merged 7 commits into from
Sep 17, 2018

Conversation

MikhailArkhipov
Copy link

@MikhailArkhipov MikhailArkhipov commented Sep 14, 2018

Fixes #94

  • Make separate class for progress reporting
  • Do not use 'estimate work' and rather track specific events
  • Make reporting thread safe so we don't end up with multiple competing reporting tasks

@@ -19,5 +19,6 @@
namespace Microsoft.Python.LanguageServer {
public interface ILogger {
void TraceMessage(IFormattable message);
void TraceMessage(string message);
Copy link
Contributor

@AlexanderSher AlexanderSher Sep 14, 2018

Choose a reason for hiding this comment

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

This has an issue of creating strings (lots of strings) even when logging is switched off or doesn't meet the required severity. It creates noticeable performance impact. If we need to get formatted strings from resources and can't use IFormattable, we can add TraceMessageFormat(string format, params object[] parameters) and do formatting only when logging is enabled.

Copy link
Author

Choose a reason for hiding this comment

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

OK

@@ -117,6 +117,12 @@
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="AnalysisProgress_MultipleItemsRemaining" xml:space="preserve">
Copy link
Contributor

@AlexanderSher AlexanderSher Sep 14, 2018

Choose a reason for hiding this comment

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

If those strings go directly to the client, how do we choose the culture?

Copy link
Author

Choose a reason for hiding this comment

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

It is to be determined.... Unfortunately, LSP does not specify locale, so we'll have to add some custom notification. Which reminds me to file #98

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we have those strings on VSC side and provide only numbers?

Copy link
Author

Choose a reason for hiding this comment

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

VSC is not the only client. LSP is public and supported by Eclipse, Vim, Emacs and more. So there is actually a plan to try LS with one of those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but isn't this part of the LS protocol is python specific and we will have to add something to the client-side anyway?

Copy link
Author

Choose a reason for hiding this comment

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

Something like python/setLocale. If client doesn't send it, we'll just use English. Also, clients typically expect string in exceptions which they display in UI. We throw with descriptive message when we cannot rename, for example.

private void UpdateProgressMessage() {
if(_activeAnalysis.Count > 0) {
_progress = _progress ?? _progressService.BeginProgress();
_progress.Report(_activeAnalysis.Count == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is more like a status message rather than a progress, cause _activeAnalysis.Count may actually go up, correct?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. This is how progresss is reported in VSC - via spinner and message in the status bar. I am not 100% about count but since analysis is recursive and event may fire from threads, I better be safe

lock (_lock) {
if (_activeAnalysis.TryGetValue(e.uri, out var count)) {
if (count > 1) {
_activeAnalysis[e.uri]--;
Copy link
Contributor

@AlexanderSher AlexanderSher Sep 14, 2018

Choose a reason for hiding this comment

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

PythonAnalyzer.AnalyzeQueuedEntries may exit before raising OnAnalysisComplete. While this one may happen only when server is disposed, I'm not sure these events (OnAnalysisQueued and OnAnalysisComplete) are actually guaranteed to be raised in pairs.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. This may explain stale messages in the status bar. I will check it out.

Copy link
Author

Choose a reason for hiding this comment

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

OK then, back to monitoring queue. The important part is to make sure we do NOT display progress when nothing is being analyzed.

@sneridagh
Copy link

Can we have a release? The problem that this fix is very annoying...

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Sep 18, 2018

@sneridagh - try insider builds starting with 9/19 build, see https://github.com/Microsoft/vscode-python/blob/master/CONTRIBUTING.md#development-build

@fgrcon
Copy link

fgrcon commented Sep 24, 2018

@MikhailArkhipov - i installed the insider beta right now, but it does not help - still get stuck with "Analyzing workspace, 3467 items left" number increasing .....

@sneridagh
Copy link

using 20/9 build:
Same behavior, the issue persists.
One CPU locked, fans spinning, Memory usage 2GB.

@sneridagh
Copy link

@MikhailArkhipov any insight?

@MikhailArkhipov
Copy link
Author

@sneridagh - Unfortunately, new builds are not available yet, we are changing build process in order to better support dynamic updates of the LS in the VS Code extension. You can try and build it yourself or wait couple of days until we settle on URLs and versioning.

@fgrcon
Copy link

fgrcon commented Sep 25, 2018

@MikhailArkhipov where can I dowload an older (last stable) extension?

@yzhang-gh
Copy link

Hope it can be solved soon 🥺

@fgrcon Maybe try uninstalling the current one, restarting vscode, and then installing from the marketplace.

@fgrcon
Copy link

fgrcon commented Sep 25, 2018

Hope it can be solved soon 🥺

@fgrcon Maybe try uninstalling the current one, restarting vscode, and then installing from the marketplace.

@neilsustc : that'is what I tried, but i have no clue where to find previous releases ...

@sneridagh
Copy link

sneridagh commented Sep 26, 2018

@MikhailArkhipov Thanks for your patience and support... but somehow I'm unable to build it on MACOS. I even tried to follow the steps by hand but multiple points of failure... The build process just doesn't build on it. I cannot spend more time on this, I will wait until October... it's a pity that the release process is that tight and cannot be bent when it's required. Something really uncommon in OSS.

What it really concerns me is that we haven't been able to test the patch properly... what if it doesn't work? We will have to wait until November and hope that all is fine?

Also, I don't understand yet the source of the problem. There's a setting that states clearly:

  // Only show errors and warnings for open files rather than for the entire workspace.
  "python.analysis.openFilesOnly": true,

I don't want my entire workspace to be analyzed, and it seems that it's what is happening. Is this setting working as expected?

By the way, I workaround it by (literally)

$ rm -rf ~/.vscode/extensions/ms-python.python-2018.8.0/languageServer

and I have at the end the same outcome but without my CPU spinning like crazy during all day.

@DonJayamanne
Copy link

@sneridagh
If you would like to test the Python extension with the latest version of the language server then please:

@sneridagh
Copy link

sneridagh commented Sep 27, 2018

@DonJayamanne I already tried this build (2018.8.1-beta (20 September 2018) and as confirmed by @MikhailArkhipov, it doesn't contain the fix or either since it doesn't have changelog entry, it doesn't work.

@yzhang-gh
Copy link

yzhang-gh commented Sep 27, 2018

@DonJayamanne Sorry for digression. Could you please also publish (or package) a new version of your vscodeJupyter extension? Some PRs were merge many monthes ago but we still cannot benefit from them now 😥.
(I tried to package it by myself but failed for knowing little about webpack and some package version conflicts(?))
I finally figured it out

@MikhailArkhipov
Copy link
Author

20th was still an old build. We published update on 28th.

@fgrcon
Copy link

fgrcon commented Sep 28, 2018

@MikhailArkhipov interesting: you are able to publish in the future ? ;-)
but https://pvsc.blob.core.windows.net/extension-builds/ms-python-insiders.vsix is still the same from 9/20!

@MikhailArkhipov
Copy link
Author

@fgrcon - Right but it downloads proper LS. VSIX itself was not an issue, content of the storage blob was. I guess it was 27th. :-)

@sneridagh
Copy link

@MikhailArkhipov The I would say that problem is still there. Probably you can here my fans blowing from where you are.

Ok, my workspace is huge, but it's not what the vast majority of people will have? In large projects where hundreds of eggs are used, I mean...

It should be something that has changed recently, even Jedi gets stuck and make my CPUs spin going loco. Before summer this didn't happen. Please, I'm considering exiting VSCode in favor of other IDE for Python because of it.

I would really want to help pinpoint what the cause is, if I'm given directions. I cannot see anymore the enable traceback log setting. Did you remove it?

@sneridagh
Copy link

@MikhailArkhipov FYI I was giving it another try, then the process blew and rebooted my mac in my face.

@AlexanderSher
Copy link
Contributor

Is it a publicly available source code?

@MikhailArkhipov
Copy link
Author

MikhailArkhipov commented Sep 28, 2018

@sneridagh - With huge workspace it is a different problem.

The bug was that analysis reporting was stuck, i.e. reporting items outstanding while actually there were none. With huge projects analysis may take a long time. That does not mean though that product is not usable. You don't have to wait for everything to complete.

If Jedi is stuck it means even Python engine takes a long time to build and walk ASTs. While C# is more performant in general, it is not a panacea. It won't be orders of magnitude faster.

@sneridagh
Copy link

@MikhailArkhipov it gets stuck as well, in fact it's a combination of both factors. The analysis, being huge, never ends, the process eats all the memory available. The message in the status bar "X items remaining" doesn't go away.

@TomFaulkner
Copy link

TomFaulkner commented Oct 1, 2018

While it isn't unusable, the whole computer slows to a crawl while the Python language server is analyzing. A 2016 MacBook Pro has the Microsoft Python Language Server taking nearly two of the four cores to itself, and half of the 16gb of RAM the system has.

Installing this https://pvsc.blob.core.windows.net/extension-builds/ms-python-insiders.vsix seems to have resolved the issue.

@sneridagh
Copy link

@MikhailArkhipov @DonJayamanne Please provide a setting for disable completely both Jedi and languageServer analyzers. I prefer not to have Intellisense at all that having my CPU fans blowing continuously.

@TomFaulkner it works for a while... until then it starts again (even after some hours).

@MikhailArkhipov now it does it even partitioning my workspace into smaller parts (not loading the big ones, eggs sources). It stays calmed, until it begins again. Just killed it manually (the languageServer process) and it stays calm again for a while. Don't know why it causes to start to get crazy again.

@TomFaulkner
Copy link

@sneridagh, you can remove the Python extension, or disable it. That is what I did prior to searching out the issue and using the insider build above. (Which is so far working.)

@sneridagh
Copy link

@TomFaulkner then I'll loose linting, etc... I only want to get rid of intellisense.

@TomFaulkner
Copy link

Sadly, yes, that is the impact.

@fgrcon
Copy link

fgrcon commented Nov 5, 2018

sadly giving up!
the latest release still eats up the whole memory within minutes and everything gets stuck.

how to get back to working release (i think early summer this year ) - I need debugging, linting & outline but can live without greedy intellisense, if there is no other way.

@TomFaulkner
Copy link

sadly giving up!
the latest release still eats up the whole memory within minutes and everything gets stuck.

how to get back to working release (i think early summer this year ) - I need debugging, linting & outline but can live without greedy intellisense, if there is no other way.

@fgrcon, I was using the beta release to get around this issue. Then after an update it got replaced and I was back to this problem. However, I noticed there was an update available for the extension, and that update resolved things. I hope this helps you, I was ready to switch as well.

@sneridagh
Copy link

@TomFaulkner @fgrcon About to give up as well... Returning either to Atom or Sublime. @fgrcon where did you get the update? I'm on 2018.10.1 (09 Nov 2018) (it seems).

/cc @MikhailArkhipov Again, please provide a way to disable intellisense (either via Jedi or PLS) in the plugin, for those that we don't care about it, but we do about the other things the plugin provides. Thanks.

jakebailey pushed a commit to jakebailey/python-language-server that referenced this pull request Nov 1, 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.

None yet

7 participants