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

Do not wait until config watcher is ready in order to launch the Atom window #19246

Merged
merged 5 commits into from May 2, 2019

Conversation

Projects
None yet
4 participants
@rafeca
Copy link
Contributor

commented May 1, 2019

Based on some profiling done locally, this await takes ~300-500ms on Windows machines (I couldn't find the root cause of it but may be related to #19242).

@rafeca rafeca requested review from smashwilson and nathansobo May 1, 2019

@smashwilson
Copy link
Member

left a comment

👍 Good catch.

@50Wliu

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Do you think a comment should be added saying why it's not awaited? Because I could potentially see someone changing it back in the future if they weren't aware.

@nathansobo
Copy link
Contributor

left a comment

I'm concerned about this introducing a race condition where we don't catch config file changes that occur just after the window starts up. Does it make sense to skip the await only on Windows so that we only introduce the race condition when it is the lesser of two evils? The downside of that is that we're introducing inconsistent behavior across platforms.

How can we remember to revisit this change once we upgrade Electron and see if it's still an issue?

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I'm concerned about this introducing a race condition where we don't catch config file changes that occur just after the window starts up. Does it make sense to skip the await only on Windows so that we only introduce the race condition when it is the lesser of two evils? The downside of that is that we're introducing inconsistent behavior across platforms.

I can do that! I'm not super happy about this solution tbh... it's one of these things where we trade off correctness to overcome a performance issue... hopefully we get rid of this on Electron v3 (I can also abandon this change and just claim the perf improvement when we do the upgrade, but it seemed like a small change to get some perf improvements without having to wait for the big upgrade).

How can we remember to revisit this change once we upgrade Electron and see if it's still an issue?

What do you think about what I suggested in #19242 (comment) ?

@rafeca rafeca merged commit 5b21fda into master May 2, 2019

2 checks passed

Atom Pull Requests #20190502.8 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the delay-waiting-for-watcher branch May 2, 2019

if (process.platform === 'win32') {
this.configFilePromise.then(disposable => this.disposable.add(disposable))
} else {
this.disposable.add(await this.configFilePromise)

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 17, 2019

Contributor

The this.config.onDidChange calls below this await really do need to happen after the config file is loaded. That is what is causing #19323. I have a fix in the works and will get it out later today.

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 17, 2019

Contributor

Fixed in #19354.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.