buchizo / ClaudiaIDE

This extension can change easily the background image of editor window in Visual Studio.
https://marketplace.visualstudio.com/items?itemName=kbuchi.ClaudiaIDE
638 stars 112 forks source link

ClaudiaIdePackage.InvokeChangeImage and ClaudiaIDE.InvokeChangeImage causing Visual Studio responsiveness issues #128

Closed davkean closed 1 year ago

davkean commented 1 year ago

Hey I'm from the Visual Studio performance team, both ClaudiaIdePackage.InvokeChangeImage and ClaudiaIDE.InvokeChangeImage are both causing unresponsiveness in Visual Studio.

In the wild, these methods are consuming large amount of thread pool threads, causing other features inside Visual Studio to have responsiveness issues. At the worst case of this (see below image), the time it took for work to be queued to the thread pool to the time before it started running was nearly 12 minutes. This Visual Studio session that ran into this would have been completely unusable.

image

Failure Score Session % Cab Count Threads 90th Threads 50th Avg Threads
Stk_ThdPl_ClaudiaIDE!ClaudiaIdePackage.InvokeChangeImage/ClaudiaIDE.InvokeChangeImage 573 0.03% 4 953 42 266

The reason this is occurring is because both methods are performing a synchronous thread blocking JTF.Run call from a background thread. In times where the UI thread is busy; such as downloading a symbol while debugging or loading a solution, these threads back up faster than the UI thread can respond causing the above issue. These JTF.Run calls should be replaced instead with a single ongoing task that is queued only once, and not blocked on.

Something similar to (I've not run or tested this):


object _lock = new object();
Task _task;

[...]
NewImageAvailable += () =>
{
   if (_task== null)
   {
      lock (_lock)
      {
         if (_task == null)
         {
            _task= ChangeImageAsync();
         }
      }
   }
}

private async Task ChangeImageAsync()
{
    try
    {
        await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
        ChangeImage();
    }
    catch
    {
    }
}

Here's our internal guidance on this:

DO NOT call JoinableTaskFactory.Run from a background thread

JoinableTaskFactory.Run is for the UI thread. If called on a thread pool or background thread, it will block that thread (preventing it from taking on new work) until the async work has completed. This can cause thread pool starvation, reducing UI and feature responsiveness.

Avoid

Here JoinableTaskFactory.Run is being used to bridge synchronous property with an async method:

public bool IsOpen // Bad
{   
    get 
    {
        JoinableTaskFactory jtf = …

        // Blocks a thread
        return jtf.Run(async () => 
        {
            var state = await GetOpenStateAsync();

            return state == FrameState.Open;
        }); 
    } 
}

private Task<FrameState> GetOpenStateAsync()
{
    …
}

Instead Instead, async should be plumbed all the way up the call chain:

public async Task IsOpenAsync()   // Good 
{
    var state = await GetOpenStateAsync();

    return state == FrameState.Open;
}

private Task<FrameState> GetOpenStateAsync()
{
    …
}
buchizo commented 1 year ago

@davkean Thank you for your report! I fixed this issue in ver 3.1.3, maybe.