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

searchApp.js startup should be delayed #38235

Closed
alexandrudima opened this Issue Nov 13, 2017 · 9 comments

Comments

Projects
None yet
5 participants
@alexandrudima
Member

alexandrudima commented Nov 13, 2017

The Search process should be delayed until after the workbench is completely initialized (explorer restored, editors restored, ext host up and running):

The dark red shows that the searchApp.js competes for OS resources with the ext host process (purple). I can share the tool that generates this when I polish it a bit:
image

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 13, 2017

Member

Adding @bpasero to provide guidance what would be a good workbench event to listen to and begin warming up/starting up the search service.

Member

alexandrudima commented Nov 13, 2017

Adding @bpasero to provide guidance what would be a good workbench event to listen to and begin warming up/starting up the search service.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Nov 13, 2017

Member

The search process only starts on request, so when it starts that early, it's probably due to extensions with workspaceContains patterns. I agree that we should delay it.

Member

roblourens commented Nov 13, 2017

The search process only starts on request, so when it starts that early, it's probably due to extensions with workspaceContains patterns. I agree that we should delay it.

@alexandrudima

This comment has been minimized.

Show comment
Hide comment
@alexandrudima

alexandrudima Nov 13, 2017

Member

@roblourens This is not what profiling via ProcMon or logging shows. It shows that the search service is always spawned eagerly on startup:

Here is a change I do. I add console.log(`forking ${this.modulePath}, ${JSON.stringify(args)}`); after the line at

this.child = fork(this.modulePath, args, forkOpts);

I see the following console output:
image

Can you please double check that the search process only starts on request ?

Member

alexandrudima commented Nov 13, 2017

@roblourens This is not what profiling via ProcMon or logging shows. It shows that the search service is always spawned eagerly on startup:

Here is a change I do. I add console.log(`forking ${this.modulePath}, ${JSON.stringify(args)}`); after the line at

this.child = fork(this.modulePath, args, forkOpts);

I see the following console output:
image

Can you please double check that the search process only starts on request ?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Nov 13, 2017

Member

Actually since this change c354fca the search process is forced to started on launch. If I remove the forwardTelemetry() call in the SearchService constructor, then it doesn't start the search service until the user does a search.

@chrmarti Can that call be delayed until a user's search request?

Member

roblourens commented Nov 13, 2017

Actually since this change c354fca the search process is forced to started on launch. If I remove the forwardTelemetry() call in the SearchService constructor, then it doesn't start the search service until the user does a search.

@chrmarti Can that call be delayed until a user's search request?

@chrmarti

This comment has been minimized.

Show comment
Hide comment
@chrmarti

chrmarti Nov 13, 2017

Contributor

Yes, my issue.

Contributor

chrmarti commented Nov 13, 2017

Yes, my issue.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 13, 2017

Member

@roblourens @chrmarti you can see here how I delay starting of the file watcher until we are running.

The idea is that the lifecycle service has phases that you can join on:

  • LifecyclePhase.Starting: right on startup (earliest)
  • LifecyclePhase.Restoring: right before we start to restore views and editors
  • LifecyclePhase.Running: after we have restored views and editors
  • LifecyclePhase.RunningForABit: 3 seconds after we have restored views and editors

@alexandrudima did you see the file watcher spawning also show up?

Member

bpasero commented Nov 13, 2017

@roblourens @chrmarti you can see here how I delay starting of the file watcher until we are running.

The idea is that the lifecycle service has phases that you can join on:

  • LifecyclePhase.Starting: right on startup (earliest)
  • LifecyclePhase.Restoring: right before we start to restore views and editors
  • LifecyclePhase.Running: after we have restored views and editors
  • LifecyclePhase.RunningForABit: 3 seconds after we have restored views and editors

@alexandrudima did you see the file watcher spawning also show up?

@chrmarti

This comment has been minimized.

Show comment
Hide comment
@chrmarti

chrmarti Nov 13, 2017

Contributor

For the search app we can postpone the launch to the first text search / QuickOpen. It used to be like that until I inadvertently changed it.

Contributor

chrmarti commented Nov 13, 2017

For the search app we can postpone the launch to the first text search / QuickOpen. It used to be like that until I inadvertently changed it.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Nov 13, 2017

Member

I think that it will still be activated on launch by extensions with workspaceContains patterns, so we can also either force it to wait in that case too, or allow it in that case.

Member

roblourens commented Nov 13, 2017

I think that it will still be activated on launch by extensions with workspaceContains patterns, so we can also either force it to wait in that case too, or allow it in that case.

@bpasero bpasero removed their assignment Nov 14, 2017

@kieferrm kieferrm referenced this issue Nov 14, 2017

Closed

Iteration Plan for November 2017 #38268

23 of 25 tasks complete

@roblourens roblourens removed their assignment Nov 27, 2017

@roblourens roblourens added the bug label Nov 27, 2017

@chrmarti chrmarti closed this in d51ea03 Nov 27, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Dec 7, 2017

Member

Verified that no search service is started when code starts.

Member

dbaeumer commented Dec 7, 2017

Verified that no search service is started when code starts.

@dbaeumer dbaeumer added the verified label Dec 7, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 12, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.