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

fix #3697 Remove blocking await. #3699

Merged
merged 2 commits into from Dec 4, 2018
Merged

fix #3697 Remove blocking await. #3699

merged 2 commits into from Dec 4, 2018

Conversation

caseyflynn-google
Copy link
Contributor

PR removes blocking await when starting a file watcher. This is
particularly important when starting Theia with a large workspace root
as the file watch will block Theia loading.

Signed-off-by: Casey Flynn caseyflynn@google.com

PR removes blocking await when starting a file watcher. This is
particularly important when starting Theia with a large workspace root
as the file watch will block Theia loading.

Signed-off-by: Casey Flynn <caseyflynn@google.com>
@caseyflynn-google caseyflynn-google changed the title FIX: 3697 Remove blocking await. FIX #3697 Remove blocking await. Nov 30, 2018
@caseyflynn-google caseyflynn-google changed the title FIX #3697 Remove blocking await. fix #3697 Remove blocking await. Nov 30, 2018
@akosyakov
Copy link
Member

PR removes blocking await when starting a file watcher.

I wonder how await can be blocking. Blocking code usually is synchronous and does not allow node process to handle new requests. In this case, the code seems to be async and should allow handling of other requests.

@paul-marechal
Copy link
Member

@akosyakov you are right that an await allow for non blocking code, unless something in the Theia boot-up sequence someone specifically wait for this before starting the application. Somewhere inside a onStart hook?

root WARN EditorNavigationContribution.onStart is slow, took: 46547 ms

When I opened Theia on /.

@akosyakov
Copy link
Member

@marechal-p can you profile the frontend please and find out with the root cause why it takes 45s? We need to rewrite EditorNavigationContribution for sure, that it does not block the shell to load navigation history: #3413 But I suspect it is not related to this issue or a follow-up effect.

I am also not against these changes. I want to understand why it helps and how I can verify it + unwatch should be handled properly.

@paul-marechal
Copy link
Member

paul-marechal commented Dec 3, 2018

@akosyakov the long wait is because of something silly:

EditorNavigationContribution.onStart needs to call some WorkspaceStorageSerivce.getData which will wait for WorkspaceStorageService to be "initialiazed", that happens when the WorkspaceService has resolved its roots. But somewhere in the root resolution, there was an await on the watchers.

I made a diff to see if I could fix it where the wrong components were waiting for it: 7bb1e9b

With this, the EditorNavigationContribution.onStart goes from [10 to 50] seconds down to ~400ms, when opening my system root (/)

@caseyflynn-google
Copy link
Contributor Author

With this, the EditorNavigationContribution.onStart goes from [10 to 50] seconds down to ~400ms, when opening my system root (/)

This seems to resolve the issue for me too. Can we apply this fix to Master?

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov
Copy link
Member

@marechal-p thanks for the analysis, it is clear now

@caseyflynn-google as soon as tests are green

I've pushed one commit to address https://github.com/theia-ide/theia/pull/3699/files#r238060287 The await was there to make sure that the disposable object for a watcher is created. If we remove await we need to create this object earlier. I've pulled it up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants