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

Avoid sync fs-calls when opening windows #16381

Closed
jrieken opened this issue Dec 2, 2016 · 5 comments
Closed

Avoid sync fs-calls when opening windows #16381

jrieken opened this issue Dec 2, 2016 · 5 comments
Assignees
Labels
debt Code quality issues perf perf-startup

Comments

@jrieken
Copy link
Member

jrieken commented Dec 2, 2016

Like this (already in an async context) and this

Update (bpasero):

@jrieken
Copy link
Member Author

jrieken commented Dec 2, 2016

related to #15455

@bpasero bpasero added this to the November 2016 milestone Dec 2, 2016
bpasero added a commit that referenced this issue Dec 2, 2016
@bpasero bpasero modified the milestones: Backlog, November 2016 Dec 2, 2016
@bpasero bpasero changed the title Avoid sync fs-call at startup Avoid sync fs-call at startup when opening windows (windows.ts) Dec 2, 2016
@bpasero bpasero changed the title Avoid sync fs-call at startup when opening windows (windows.ts) Opening windows: Avoid sync fs-calls Dec 8, 2016
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug debt Code quality issues and removed bug Issue identified by VS Code Team member as probable bug labels Nov 6, 2017
@bpasero bpasero removed this from the Backlog milestone Nov 15, 2017
@bpasero bpasero changed the title Opening windows: Avoid sync fs-calls Avoid sync fs-calls when opening windows Jun 7, 2021
@bpasero bpasero added this to the Backlog milestone Oct 5, 2021
@bpasero
Copy link
Member

bpasero commented Sep 7, 2022

Backup service is no longer writing to its metadata on startup in a way that it blocks the startup. Previously we had:

  • an async write very early (async but still blocking)
  • a sync write for each window that opens

This showed up as largest block in profiles for me when looking for main process and seems to result in a significant reduction from todays exploration build:

image

Tracing the remaining sync fs operations, we still do, they seem not to be significant (profile attached below):

  • configureCommandlineSwitchesSync - we read argv.json on startup to support runtime flags very early and we must set the flags before app.on("ready") so imho this is fine
  • getUntitledWorkspacesSync - we check for untitled workspaces with a readDirSync call since we always restore untitled workspaces
  • statSync - we still check paths to open with a stat call to know if the file exists and what type it is (folder vs file)

These remaining ones are not in an async context and I wonder how far we want to push it. I think changing the remaining ones would require us to change to async window opening which would probably be a larger undertaking.

Attached profile, I was searching for sync in the profile to find every location (77 in total, mainly from node.js require calls):
prof-ZKVq5BB7.main.cpuprofile.zip

@bpasero
Copy link
Member

bpasero commented Sep 10, 2022

@bpasero
Copy link
Member

bpasero commented Sep 12, 2022

Ended up making opening windows async, and thus was able to remove the sync fs calls. We still need to read argv.json on startup to configure Electron switches, but the rest is all async now, esp. the initially mentioned statSync calls to validate paths to open.

@bpasero bpasero closed this as completed Sep 12, 2022
@jrieken
Copy link
Member Author

jrieken commented Sep 13, 2022

Thanks @bpasero - This issue almost turned 6 🍰 and yielded good speed up

@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues perf perf-startup
Projects
None yet
Development

No branches or pull requests

2 participants