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

Remove synchronous process launches #15725

Closed
DonJayamanne opened this issue Mar 19, 2021 · 4 comments
Closed

Remove synchronous process launches #15725

DonJayamanne opened this issue Mar 19, 2021 · 4 comments
Assignees
Labels
area-editor-* User-facing catch-all bug Issue identified by VS Code Team member as probable bug

Comments

@DonJayamanne
Copy link

DonJayamanne commented Mar 19, 2021

In configSettings.ts we have some code that launches python processes synchronously.
// const output = child_process.execFileSync(pythonPath, args, { encoding: 'utf8' });
This slows down other extensions when loading Python extension.

If user is working purely on python extension, then I guess this is ok to have.
However consider the following scenario:

  • User loads a Jupyter Notebook
  • Jupyter notebook activates python extension
  • Now Python extension runs some code synchronously, thereby blocking other extensions such as Jupyter from running - i.e. Jupyter looks slow in opening Jupyter Notebooks.

Removing this code improves the load times of Jupyter notebooks significantly.

Related upstream issue causing serious perf issues in Jupyter Notebooks microsoft/vscode-jupyter#5211
Basically Jupyter Notebooks takes 20-30 seconds to load and we're trying to address that.

@DonJayamanne DonJayamanne added bug Issue identified by VS Code Team member as probable bug triage-needed Needs assignment to the proper sub-team labels Mar 19, 2021
@karthiknadig
Copy link
Member

@DonJayamanne Do we need to run python there to check if it is valid? I feel like an file exists check should be enough for the variety of cases it handles there.

@brettcannon
Copy link
Member

A file check should be fairly cheap compared to executing Python itself.

@DonJayamanne
Copy link
Author

I feel like an file exists check should be enough for the variety of cases it handles there.

Sure thats much better, is there a way we can do this even without a synchronous file check (sync = still blocking node process, though its a significant improvement)

@karthiknadig
Copy link
Member

karthiknadig commented Mar 19, 2021

I have looked into that class before. Turning functions in the class to async is something we need to do in the long term. Since most of the APIs in that class are all sync, changing any one of them to async especially something so fundamental like pythonPath cause significant async creep across the extension.

I would say, give it a try to make that async, but if you see that it is creeping into too many places, then we can just do the sync version.

@karthiknadig karthiknadig added area-editor-* User-facing catch-all needs PR labels Mar 20, 2021
@ghost ghost removed the triage-needed Needs assignment to the proper sub-team label Mar 20, 2021
@ghost ghost removed the needs PR label Mar 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-editor-* User-facing catch-all bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

3 participants