Skip to content
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.

Sign up for GitHub

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

Increase ThreadPool.MinThreads limit #3845

Merged
merged 2 commits into from
Jul 13, 2022
Merged

Increase ThreadPool.MinThreads limit #3845

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e4864d6
Increase ThreadPool.MinThreads limit
nohwnd Jul 7, 2022
649ca6a
Update src/vstest.console/CommandLine/Executor.cs
nohwnd Jul 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/vstest.console/CommandLine/Executor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Globalization;
using System.IO;
using System.Linq;
using System.Threading;

using Microsoft.VisualStudio.TestPlatform.CommandLine.Internal;
using Microsoft.VisualStudio.TestPlatform.CommandLine.Processors;
Expand Down Expand Up @@ -60,6 +61,23 @@ internal class Executor
/// </summary>
public Executor(IOutput output) : this(output, TestPlatformEventSource.Instance, new ProcessHelper(), new PlatformEnvironment())
{
// TODO: Get rid of this by making vstest.console code properly async.
// The current implementation of vstest.console is blocking many threads that just wait
// for completion in non-async way. Because threadpool is setting the limit based on processor count,
// we exhaust the threadpool threads quickly when we set maxCpuCount to use as many workers as we have threads.
//
// This setting allow the threadpool to start start more threads than it normally would without any delay.
// This won't pre-start the threads, it just pushes the limit of how many are allowed to start without waiting,
// and in effect makes callbacks processed earlier, because we don't have to wait that much to receive the callback.
// The correct fix would be to re-visit all code that offloads work to threadpool and avoid blocking any thread,
// and also use async await when we need to await a completion of an action. But that is a far away goal, so this
// is a "temporary" measure to remove the threadpool contention.
nohwnd marked this conversation as resolved.
Show resolved Hide resolved
//
// The increase to 5* (1* is the standard + 4*) the standard limit is arbitrary. I saw that making it 2* did not help
// and there are usually 2-3 threads blocked by waiting for other actions, so 5 seemed like a good limit.
var additionalThreadsCount = Environment.ProcessorCount * 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the concept of pushing the thread pool limit higher, but I'm not sure I get why we decided to raise the limit to exactly 4 times the processor count. Is "4" a constant determined empirically that yielded the best throughput overall or is there a more formal reason for choosing it ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used 2*, then 5* then 10*. I know that the path blocks 2-3 threads, and there are double the threads blocked when you have a datacollector. So it is part empirical, part guessing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I ended up with 1* (workerThreads) + 4*, to land on 5*

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write that down as comment in code for later.

Copy link
Contributor

@MarcoRossignoli MarcoRossignoli Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried to change only the worker one? I don't know the I/O one makes difference here, we're not async at all so I suppose we're not using it. And let's hope to not generate too much contention/memory consuption(if I recall well on .NET 1mb per stack is commited) on huge test servers 🤞 const heuristics is...meh...

Maybe if it's only for VS responsiveness we could recognize it(design mode) and increase only in that case, but not mandatory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try to change, but if the IO pool is not exhausted then this option has no impact, it simply won't get past the limit. In both cases we are just telling the threadpool thread to start more threads without delay when it needs to. We will eventually stop blocking them, and thread pool can re-balance itself, we are only paying a small price say 1MB per thread, but we know we will cap out on the amount of threads that is x times the processor count. So if there is a server that has 100 cores, it probably will also have 1GB of ram to spare. If it ever gets to the point where it allocates that many threads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the comment.

ThreadPool.GetMinThreads(out var workerThreads, out var completionPortThreads);
ThreadPool.SetMinThreads(workerThreads + additionalThreadsCount, completionPortThreads + additionalThreadsCount);
}

internal Executor(IOutput output, ITestPlatformEventSource testPlatformEventSource, IProcessHelper processHelper, IEnvironment environment)
Expand Down