Improve performance of parallel discovery

[shyamnamboodiripad]

Currently sequential discovery of tests with MaxCpuCount = 1 is faster than parallel discovery (MaxCpuCount = 0) for test assemblies that only contain a few (100) tests. This is because -

1. Sequential discovery reuses the same testhost process to discover all supplied test sources. Parallel discovery on the other hand creates a new testhost process for every supplied test source. The overhead of process startup, extension loading etc. is significantly greater than the time required to discover a few (100) tests in this case. Sharing / reusing each teshost processes to discover more than one test source over the lifetime of the discovery operation should improve performance significantly.

2. The testshost processes for parallel discovery are started on a staggered basis and the actual number of testhosts running at any point is always (significantly) smaller than the specified MaxCpuCount. Starting the max allowed number of testhost processes up front should help to better amortize the startup time overhead for individual processes.

[shyamnamboodiripad]

FYI @Evangelink @nohwnd Logging this so that I have something I can link to in my AzDO side changes.

[nohwnd]

1. This is only true for .NET Framework (we still can't repro what you said you saw 😁), and you are right, not sharing testhost for maxCpuCount=1 is about 2 times slower in this case. I will try to change how we start testhost to make it more ready to run next source. Because this would improve runtime for both .NET and .NET Framework.

2. I am fixing that in my multi TFM branch. We should discuss that maxCpuCount logic, logicalCoresCount - 4, or logicalCores / 2 seem to be much faster than setting it to the count of logical cores. Most likely because then a testhost does not compete with vstest.console for resources, which makes the execution slower for all other testhosts that need to wait for console to finish processing.

[Evangelink]

Is it worth to consider having vstest process having a higher priority compared to testhosts so that we ensure it's less of a bottleneck?

[shyamnamboodiripad]

This is only true for .NET Framework (we still can't repro what you said you saw 😁)

My bad @nohwnd. I went back and confirmed that the solution I was trying against (MSTest_100K) was indeed targeting .NET Framework 😢 I was probably confused because the projects in question were new SDK-style and incorrectly assumed they were also targeting .NET Core. Apologies for the confusion.

Its a pity .NET Framework and .NET Core differ in behavior here. Does .NET Framework also isolate each source being discovered in its own AppDomain?

Is it worth to consider having vstest process having a higher priority compared to testhosts so that we ensure it's less of a bottleneck?

@Evangelink as we discussed before I agree it would be great to use low priority by default for testhosts. MSBuild has a similar setting for build. All that does is to set the process priority to BelowNormal.

Process.GetCurrentProcess().PriorityClass = ProcessPriorityClass.BelowNormal;

We could have a similar runsetting for testhosts. That should give us good CPU utilization (without having to guess / have heuristics around optimum CPU count) while at the same time ensuring that testhosts don't steal CPU from other processes / foreground operations. Thoughts @nohwnd?

[nohwnd]

I forgot about the priority, I can try that out easily, but it won't be as "transparent" as running on Normal. Because then testhost is more likely to be pre-empted by not only vstest.console but also any other Normal priority process. We can also play with CPU affinity, but that is more guesswork.

[nohwnd]

Tried it, it does not help at all. But I also don't see using max cpu to slow down significantly, it probably depends on the workload, and I don't want to be guessing here. Also setting the priority is hard, because you either have to wait for the process to start and be initialized (race condition there), or set the priority of the parent process (vstest.console), start the process and then re-set the priority. Won't use it now, but pushed here:

https://github.com/nohwnd/vstest/tree/set-host-priority

[shyamnamboodiripad]

Tried it, it does not help at all. But I also don't see using max cpu to slow down significantly, it probably depends on the workload, and I don't want to be guessing here.

Ack that's a bummer. I wonder why... It would be great if we can experiment a bit to learn more.

have to wait for the process to start and be initialized (race condition there)

I think this race condition is acceptable so long as we can do it as soon as possible / as long as setting it to BelowNormal is the amongst the first pieces of code we run in the process. MSBuild also sets it after the fact as far as I can tell from the code I linked above.