Electron: Enable renderer process reuse

[rzhao271]

We currently have process reuse disabled in main.js, but considering how the native modules we use are now context-aware, we can start looking more into enabling process reuse.

There are other processes to look into, but for now, I would like to concentrate just on the extensionHost and watcherService. For those, I'm wondering

1. Are the processes killed + restarted during a reload?
2. Are the processes killed + restarted when a user opens a new folder or workspace? I'm currently trying to see if such actions are equivalent to a reload right now, but haven't yet found the code.
3. If yes to 1 and/or 2, can they be set up in a way using IPC such that the main process can send a reset signal during reloads? This way, we can continue using the same child process, and it's just the state of the child process that gets reset.

[rzhao271]

CC @deepak1556

[bpasero]

@rzhao271 anytime a window either gets reloaded, closed or workspace changed within the window, the same code gets triggered to handle this because anyone in the window can participate on this event to either veto it from happening or doing some task (even long running). The events are ILifecycleService.onBeforeShutdown and ILifecycleService.onWillShutdown. The latter is triggered from here:

vscode/src/vs/workbench/services/lifecycle/electron-sandbox/lifecycleService.ts

Line 86 in 94c9ea4

ipcRenderer.on('vscode:onWillUnload', async (event: unknown, reply: { replyChannel: string, reason: ShutdownReason }) => {

I think we can easily use these events to kill processes when the onWillShutdown is fired.

The watcher is actually a IPC Client and it looks like the dispose method will kill the process.

I think the extension host already has code to participate in the shutdown so that extensions can properly get deactivated:

vscode/src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts

Line 130 in 94c9ea4

this._toDispose.add(this._lifecycleService.onShutdown(reason => this.terminate()));

A couple of things we have to make sure:

• are there other child processes that the renderer spawns (e.g. search is also a process!)
• are we sure all native modules are either context aware or no longer being used from the renderer (e.g.,only recently I found that the shared process -which is also a renderer process - was still not ready, see spdlog: Loading non-context-aware native module in renderer #120159)
• are there any other implications for process renderer reuse that we need to be thinking about

Would you be interested in providing a PR for this work to help out on this task?

[bpasero]

It looks like windows-process-tree is a dependency that we still drag into the shared process (here) which has not been adopted yet to be context aware iirc. A requirement would be to first make that module context aware unless we can control the process reuse per renderer.

[rzhao271]

are there other child processes that the renderer spawns (e.g. search is also a process!)

Deepak mentioned that I can look for places that call bootstrap-fork, and so I have the following list:

• watcherService
• customEndpointTelemetryService
• ptyHostService
• localProcessExtensionHost
• searchService

The following places use loadURL to open a new window:

• issueMainService
• sharedProcess
• window in windows/electron-main

are we sure all native modules are either context aware or no longer being used from the renderer (e.g.,only recently I found that the shared process -which is also a renderer process - was still not ready, see #120159)

Before doing any optimizations (this issue), we should double-check that all the native modules we use are context-aware. I'll create a separate issue for windows-process-tree.

are there any other implications for process renderer reuse that we need to be thinking about

Deepak provided two scenarios to consider, which are reloading a window, and switching the current workspace. Are there any other cases that could involve a process being reused?

If we implement a reset signal so that instead of killing + relaunching the window, the window state is simply reset, a part we would have to be careful about would be ensuring the old state doesn't leak into the new one.

[deepak1556]

@bpasero in what situations does the shared process get reloaded ? Ignoring the case where we want to restart the process when it crashed.

[bpasero]

Deepak mentioned that I can look for places that call bootstrap-fork

Yes, that is a good way to find out what child processes we spawn from a renderer.

The following places use loadURL to open a new window

Fyi that issueMainService has 2 different windows: the process explorer and the issue reporter

Before doing any optimizations (this issue), we should double-check that all the native modules we use are context-aware

Yes, I would suggest to do a pass over our dependencies of package.json including the optionalDependencies which list specific native modules for Windows only and find the ones which are still being used in renderers.

When I did that last time, I only found the windows-process-tree, while the others where only accessed from either a child process (like file watchers) or the main process.

Deepak provided two scenarios to consider, which are reloading a window, and switching the current workspace

At least for VSCode workbench windows, these 2 are the same. They both go through the same code and will trigger loadURL eventually.

If we implement a reset signal so that instead of killing + relaunching the window, the window state is simply reset, a part we would have to be careful about would be ensuring the old state doesn't leak into the new one.

I do not think that keeping the script context alive is an option currently, simply because we currently have no means of cleaning up all the state on shutdown. So far we get a free cleanup because the context changes everytime we call loadURL. My understanding is that we continue to call loadURL, even when process reuse is enabled, or not?

#35109 is related where we discussed the concept of doing a workspace change without a loadURL call, but there is lots of work involved currently.

@deepak1556

in what situations does the shared process get reloaded

Never. We have no support to reconnect to the shared process currently.

[deepak1556]

So far we get a free cleanup because the context changes everytime we call loadURL. My understanding is that we continue to call loadURL, even when process reuse is enabled, or not?

That is correct and there will be a context change on reload. Based on the lifecycle hooks you have mentioned in #120431 (comment) , they are sufficient to have child processes restart in the process reuse model as well, we don't have to treat the process reuse mode as a different state.

[rzhao271]

#35109 is related where we discussed the concept of doing a workspace change without a loadURL call, but there is lots of work involved currently.

@bpasero So this issue is essentially reviving #35109? I would also like to confirm what is meant by there being "lots of work involved currently", because I currently only see two blockers before we consider implementing some workspace reuse optimizations:

1. Verify the native modules are context-aware (shouldn't take too long)
2. Merging Electron 12 into Insiders and turning on process reuse (this one's riskier and we will likely have regressions to work on first)

[deepak1556]

We should keep the workspace reuse idea tracked separately, this issue I would like us to track only the work required to enable

app.allowRendererProcessReuse = true;

For that the blockers:

1. Verify the native modules loaded in workbench and shared process are context aware, ignore modules loaded in other processes as they are not affected

2. Turn on the flag in master and identify regressions. This need not depend on Electron 12 adoption

3. Ensure child processes spawned from workbench are restarted on reload.

[bpasero]

Agree on #120431 (comment). Some comments:

Turn on the flag in master and identify regressions. This need not depend on Electron 12 adoption

To clarify: are you suggesting to push this to insiders/stable even before Electron 12 and even before we moved away from webview?

Ensure child processes spawned from workbench are restarted on reload

I think they automatically get restarted because we always spawn them from the workbench when it starts. So the task at hand would be to ensure that the processes a renderer spawns properly terminate as part of the workbench shutdown (which is an event that happens anytime a window is closed, reloaded or when the workspace changes).

[deepak1556]

To clarify: are you suggesting to push this to insiders/stable even before Electron 12 and even before we moved away from webview?

yup that is correct, since electron 11 already supports process reuse. I think this should be experimented on current main branch.

[bpasero]

@deepak1556 oh sorry, nevermind. I confused myself here with the idea to enable contextIsolation :). Too many things...

[rzhao271]

Some notes based on offline discussions:

Verify the native modules loaded in workbench and shared process are context aware, ignore modules loaded in other processes as they are not affected

1. The shared process is not being killed and restarted during reloads. Therefore, we don't need native modules under the shared process to be context-aware right now. Even if we did, the only native modules I see are node-pty and vscode-windows-process-tree. I'm making the latter context-aware in Make module context-aware vscode-windows-process-tree#30 and I'm not sure about the former yet.
2. The workbench is being killed and restarted during reloads. The native modules that are used or might be used under the workbench are native-is-elevated, vscode-windows-ca-certs, native-watchdog, nsfw, fsevents, and spdlog. @deepak1556 has confirmed these modules are context aware (and I also took a look at some of them).

Turn on the flag in master and identify regressions. This need not depend on Electron 12 adoption
Ensure child processes spawned from workbench are restarted on reload.

Currently, the largest regression is the fact that child processes spawned from the workbench are not restarting on reloads. Right now with the process reuse flag enabled, we have the following:

1. The child processes of the workbench are killed.
2. workbench.html under electron-browser is loaded along with all the scripts.
3. The child processes of the workbench are not restarted.

[deepak1556]

The workbench is being killed and restarted during reloads

Are you saying there is a process swap on reloads ?

The native modules that are used or might be used under the workbench are native-is-elevated, vscode-windows-ca-certs, native-watchdog, nsfw, fsevents, and spdlog

native-watchdog is not context-aware, the module has global states that will conflict when used from multiple contexts.

spdlog should not be used in renderer after #120159, how did you confirm spdlog usage ?

[bpasero]

The shared process is not being killed and restarted during reloads

Isn't node-pty only used in child processes from the shared process? My understanding is that there is one child process per terminal tab and only in there we use node-pty?

The native modules that are used or might be used under the workbench are native-is-elevated, vscode-windows-ca-certs, native-watchdog, nsfw, fsevents, and spdlog

Can you clarify how you found these dependencies, from my memory:

• native-is-elevated is provided only from electron-main here
• vscode-windows-ca-certs is not a direct dependency anymore, but possibly a dependency of a module we depend on now (discussion in vscode-windows-ca-certs seems no longer being used #120546)
• native-watchdog seems to be used only in the extension host process which is a child process of the renderer
• nsfw, fsevents are used in file watcher processes that are child processes of the renderer
• spdlog should only be used from electron-main given recent changes

Currently, the largest regression is the fact that child processes spawned from the workbench are not restarting on reloads

That is something to follow up and is unexpected. After all, when the workbench starts we always spawn new processes for file watcher and extension host because we do not support to reconnect to existing processes.

[bpasero]

This seems to trigger electron/electron#30153 in selfhost where suddenly colorization and any node.js IPC communication stops to work.

[bpasero]

Closing this in favour of the general work item to update to Electron 16 where this will be the default.

[rzhao271]

I'm wondering where the testplan item for this issue is, and/or whether we need a new one? @bpasero

[bpasero]

I think @deepak1556 covered that as part of the E16 and E17 test plan items previously.

[deepak1556]

Yup its covered in previous milestone testing #141268. This feature has already gotten plenty of coverage, does not require additional testing.