Git: Spawning git.exe takes over the event loop

[jrieken]

Telemetry tells us that git is one of the extensions causes the extension host to freeze. I have capture the following, unsuspicious, profile. It only got reported to the log because the freeze was only for ~1sec. Anyways, we should see if there is something wrong with git or the cpu profile logic.

git-slow.cpuprofile.txt

[jrieken]

I'd put my money on spawn. In the profile it's called ~10 times, often costing 30ms. On windows spawning is much slower, esp on some setups. @joaomoreno did we do this different before moving into the extension host? Didn't we have a dedicated process that manages git processes?

[jrieken]

From 24047 stalls caused by vscode.git, 18517 happen on windows, 1468 on mac, and 4062 on linux. I blame it on git and its usage of spawn, the reporting seems to work

[joaomoreno]

We did! In Visual Studio Online "Monaco". 😆

@jrieken Bringing it back means we'll have yet another process in our tree structure, serving the single purpose of spawning git.exe processes.

[jrieken]

We did! In Visual Studio Online "Monaco".

No - before git became an extension, running in the UX process were these things aren't "accepted"

serving the single purpose of spawning git.exe processes.

If you rather prefer to block any other extension from working than that's your decision. Those 24047 stalls, are stalls over 5 secs exclusively caused by git. That's almost one work day wasted plus all the time below the threshold that we don't report.

[joaomoreno]

No - before git became an extension, running in the UX process were these things aren't "accepted"

Oh yeah, I guess we kept that going from Monaco.

If you rather prefer to block any other extension from working than that's your decision. Those 24047 stalls, are stalls over 5 secs exclusively caused by git. That's almost one work day wasted plus all the time below the threshold that we don't report.

@jrieken I don't dispute the severity, I 100% agree with you on that. I just find it hard to accept the tradeoff of having 50MB allocated for spawning processes.

Instead of forking, we could actually write a lightweight process spawner (C++/Rust) and just use that instead. 🤓

[joaomoreno]

Reference issue: nodejs/node#14917

[jrieken]

Instead of forking, we could actually write a lightweight process spawner (C++/Rust) and just use that instead. 🤓

I don't really care how you do it but it's kinda lame that we aggressively chase extensions that block the extension host (#70492) and on the other side ship a built-in extension that's almost the worst (git is second)

[jrieken]

and you aren't alone, emmet is 3rd (#71381) moving up the ranks fast

[joaomoreno]

Ahah check out this reply from that thread from @nathansobo back in 2017:

I don't understand the details of process spawning as well as I'd like, but I thought it might be helpful to report that the synchronous nature of child_process.spawn has been a problem for us in Atom. We were making pretty heavy use of spawn to run Git commands in the background, and eventually tracked it down as the source of intermittent pauses that were harming the responsiveness of the application.

As a solution, we moved all Git spawning to a separate Electron process. Since that eats way more memory than we'd like, I'm tinkering with a native spawn server that will serve a similar role to the server in node-spawn-async, but with a lighter memory footprint.

[jrieken]

Well, I see that as a clear sign of moving things into a separate process. Sure, there is a memory drawback but it won't freeze the process anymore. One really bad thing and one less ideal thing

[joaomoreno]

OK thanks for the help.

[lszomoru]

This does not seem to be an issue any more based on our recent telemetry data.