Bad performance: launching processes in parallel on Win32 / Windows

[bernd5]

I would really like to launch multiple worker-processes in parallel with minimal blocking.
My code is heavily using async / await. Unfortunately the overall perfomance is not as good as I would expect.

For me the perfomance killer seems to be line 479 lock (s_createProcessLock) which disallows effectively parallel process launching.

Why is the lock static?

[danmoseley]

I do not know why that lock is static. It is so in the internal code back to at least 2010 after which I do not currently know how to find history.

It's only used for Windows.

@stephentoub do you know?

[bernd5]

any news?

[stephentoub]

do you know?

I know there was a reason for it, as this question has come up before and there was discussion about it, but I just spent a while searching and can't find remnants of those discussions.

[bernd5]

I can't see any reason, too.

I have checked the final assembly code (including all other parts). The variable s_createProcessLock is only used in StartWithCreateProcess. But there seems to be no static data to protect.
The most simple solution would be just to remove static from that lock object.

Via blame I can see it was moved 5 years ago to winodows only in a very huge commit, which refactored the Process library to have a PAL. Before this it was in the "initial commit" - so in this repository there is no further history...

For my project I could access win api directly via PInvoke, but that is something I would really like to avoid because it breaks portability...

[danmoseley]

@jkotas do you recall any context?

[jkotas]

This lock is trying fix the problem with the inheritable stdin/stdout/stderr handles. We need to make sure that only one process inherits them. If the redirected handle gets inherited by multiple processes starting in parallel by accident, bad things happen.

For the record, here is the .NET Framework change that introduced the lock:

change 2624613 edit on 2007/06/21 11:19:03

	DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Title: DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Bugs fixed:
	111431
	Issue description:
	DevDiv Bugs #111431: Port for DTS - SRX070508601750 - Process.WaitForExit
	Method is broken
	Change description:
	Take a lock during process creation to ensure only one process is
	created while handles which can be duplicated into spawned processes are
	open

[jkotas]

The fix to remove this lock is to use UpdateProcThreadAttribute and PROC_THREAD_ATTRIBUTE_HANDLE_LIST to explicitly specify the list of handles to be inherited by the process.

[jkotas]

While you are on it, you can also replace DuplicateHandle here:

runtime/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

Line 862 in 4f9ae42

if (!Interop.Kernel32.DuplicateHandle(currentProcHandle,

with just SetHandleInformation. Patching the existing handle should have slightly better performance than creating new ones.

[stephentoub]

he fix to remove this lock is to use UpdateProcThreadAttribute and PROC_THREAD_ATTRIBUTE_HANDLE_LIST to explicitly specify the list of handles to be inherited by the process.

Won't that exclude from being inherited other handles in the process that should have been, e.g. any created by AnonymousPipeServerStream?

[jkotas]

Ok, I have not realized that other components may be depending on the CreateProcess always being called with bInheritHandles: TRUE. If it is the case, this fix requires to either create a new API to opt into CreateProcess that scales well (but limits handles that get inherited) and/or accept some breaking changes.

[jkotas]

Related: https://github.com/dotnet/corefx/issues/306

[bernd5]

In my case I have explicitly redirected all handles via RedirectStandardOutput / RedirectStandardError / RedirectStandardInput. So the API should create some Pipes.

Is it still required to do some locking?

[stephentoub]

Is it still required to do some locking?

You don't need to do any, but the implementation still does, yes. Let's say you launch two processes concurrently. If one of them redirects standard input/output/error, that means it's opening new handles to be inherited into the child process to use for communication between the parent and child. But those handles could also end up getting inherited by the other concurrently started child process.

[bernd5]

Up to now I dont got the point.
I understand that some synchronization would be required without redirection. Otherwise you would get strange results (mixed output / or buffer corruption...).
But if all inputs/outputs are redirected by inheriting these private redirect pipe handles than it should just work in parallel (or why not??). They can't be accessed by a second CSharp-Process.

=> The lock should be aquired only if not all standard inputs / outputs are redirected.

If it is not possible, the locked region should be smaller (e.g. getting the working directory ...).

[stephentoub]

But if all inputs/outputs are redirected by inheriting these private redirect pipe handles than it should just work in parallel (or why not??). They can't be accessed by a second CSharp-Process.

When it goes to start a child process, it creates an anonymous pipe with an inheritable handle for the child: the child will inherit that handle, which is how it communicates with the parent. If any other process is started at the same time, it would also inherit that same handle that wasn't intended for it, such that both child processes inherited the same handle. That's the problem the lock is avoiding. Jan's suggestion was to explicitly tell the child process which handle to inherit (i.e. "inherit these and only these"), which would address this, because then an unrelated child wouldn't inherit the other's handle, but that's problematic for other reasons, because other parts of the system were designed with the expectation that their handles would be inherited by children.

If it is not possible, the locked region should be smaller (e.g. getting the working directory ...).

It's not about the working directly. It's about making the whole region around handle creation / process starting / handle destruction atomic.