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.

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

NuGetAbstraction.GetSdkResult could take out two thread pool threads in one call #12739

Open
lifengl opened this issue Jul 11, 2023 · 4 comments
Labels
Functionality:Restore Priority:2 Issues for the current backlog. Product:MSBuildSDKResolver The NuGet powered SDK resolver. Owned by MSBuild team Product:VS.Client Tenet:Performance Performance issues Type:Bug

Comments

@lifengl
Copy link

lifengl commented Jul 11, 2023

NuGet Product Used

Other/NA

Product Version

Visual Studio 17.6

Worked before?

No response

Impact

Other

Repro Steps & Context

The function is written in a way that it could take out two thread pool threads in one single call. Here is the problematic piece:

image

This call to use Task.Result is generally forbidden inside VS, as it can easily cause deadlocks on UI thread. (The generally pattern is calling JTF.Run and await the task inside it.

When the function is called on the thread pool. This pattern can lead poor performance as:
1, Task.Run would queue the work in the current thread pool queue
2, when the code runs into Task.Result, it blocks the current thread, so it will stop processing any other pending work.
3, generally, the other thread pool thread takes work in the priority order of its own queue, then the global queue, and when both empty, it looks for each other's private queue.

So, the code pattern would lead the thread to be blocked, until the condition of another thread pool thread happens to have no other work to do to start the Restore task. This can lead a much longer delay, when many other works are queued in the thread pool.

as the comment in the code, the Task.Run is a workaround to address the problem when this function is called on the UI thread, so we should remove this workaround when the function is not called on the UI thread, especially on the thread pool.

Verbose Logs

No response

@lifengl
Copy link
Author

lifengl commented Jul 11, 2023

@nkolev92 nkolev92 added the Product:MSBuildSDKResolver The NuGet powered SDK resolver. Owned by MSBuild team label Jul 13, 2023
@nkolev92
Copy link
Member

This is code that runs in .NET SDK as well, where we have no access to a Joinable Task Factory.
If we want to do that, we would need to be creative.

@nkolev92 nkolev92 added the Priority:2 Issues for the current backlog. label Jul 13, 2023
@lifengl
Copy link
Author

lifengl commented Jul 14, 2023

@nkolev92 : yes, i understand that. This is a part of the problem, however, the part I most care about is to remove this Task.Run/ task.Result, when this code is on the thread pool. This would not require any JTF dependencies. Basically, it need check Thread.IsThreadPoolThread and skip all this workaround. Can we fix that part first?

@nkolev92
Copy link
Member

Yeah, that should be simple enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:Restore Priority:2 Issues for the current backlog. Product:MSBuildSDKResolver The NuGet powered SDK resolver. Owned by MSBuild team Product:VS.Client Tenet:Performance Performance issues Type:Bug
Projects
None yet
Development

No branches or pull requests

3 participants