Do not call JTF.Run from a bg thread in options initialization #77788
Conversation
Jan reported a speedometer regression in Threads_AdjustedBackgroundJTFRunCount_devenv when trying to merge the next version roslyn branch into VS. This codepath has changed significantly due to the package initialization changes I've been making. Specifically, RoslynPackage used to fire and forget a call to LoadOptionPersistersAsync in package initialization and now doesn't do this until after the package is loaded. Previously, after firing off this work, there would be a good amount of time before the first option was used, likely from ColorSchemaSettings.MigrateToColorSchemeSetting. Thus, the first call to GlobalOptions.GetOptionPersistersSlow would likely not have any async work remaining. My hypothesis is that even though a JTF.Run would have previously been invoked on a bg thread, it wouldn't have been reported as the task would have already been completed. LoadOptionPersistersAsync was intentionally moved to after the package load, and thus it's intentional that we don't frontload this work. It's now expected that when the first option is requested, that there is still async work remaining. The change in this PR makes it so if we are on a bg thread, we don't call JTF.Run in this context, instead just get the result directly. Blocking a bg thread while the options persisters finish loading isn't too concerning. Insertion PR reporting regression: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/621620
ghost
added
Area-IDE
untriaged
| if (workspaceThreadingService is not null) | ||
| if (workspaceThreadingService is not null && workspaceThreadingService.IsOnMainThread) | ||
| { | ||
| // speedometer tests report jtf.run calls from background threads, so we try to avoid those. |
There was a problem hiding this comment.
Is the reason that's flagged by Speedometer is because blocking one thread on another is bad, and we're still doing this here anyways? I get this will avoid noise, but do we need to delete this or something?
There was a problem hiding this comment.
I'm not 100% sure why they have that as a reported flag, but typically you don't need to do a JTF.Run while on a threadpool thread.
I did actually go down the path of making a GetOptionsAsync which completely removed the JTF.Run in that codepath and then modified the options users during package load to use that async method, but that ended up being quite a bit larger change, and this seemed like a lot easier approach. I can create a draft PR with the GetOptionsAsync change if you like, but it did seem like a lot of code overhead as only the very first call would really execute that jtf.run
There was a problem hiding this comment.
@davkean This looks like a case of threading rule violations created by folks motivated by the speedometer results, which may only track JTF.Run on a background thread but is fine with Task.Wait calls. We need to find a better way to communicate this, at least.
@ToddGrun This isn't a fix. It's just slipping under the radar. And your 'fix' is worse than the disease. If it's too expensive to refactor the code to be async, you should still use JTF.Run (as per the 2nd threading rule) and request an exception for speedometer.
JTF.Run reduces thread use when sync code calls async code, compared to Task.Wait() or Task.Result on an incomplete task, by setting up a SynchronizationContext that encourages await continuations to resume on the original (already blocking) thread. Task.Wait() simply blocks the thread completely, forcing continuations to run on yet another thread (now at least 2).
So JTF.Run is better to call on any thread than Task.Wait.
The reason the VS wiki says to avoid JTF.Run on background threads is not because Task.Wait is better, but because blocking threadpool threads contributes to threadpool starvation, which then causes UI delays. Blocking the UI thread also directly leads to UI delays, but I think @davkean's argument for the wiki is that code running on the background thread is newer than a lot of legacy code on the main thread, and therefore we have less excuse for not making the whole stack async when it's on the threadpool and avoiding the need to synchronously block at all.
There was a problem hiding this comment.
Will push out a draft PR today with the async changes I was trying out which remove the JTF.Run call (at least from the GetOption calls that occur during package load)
There was a problem hiding this comment.
I've gone ahead and pushed out a PR with one potential way to address this by making GetOption have async variants:
This is a potentially better way to address the speedometer regression "fixed" by dotnet#77788. That PR just replaced a JTF.Run on a bg thread with a .Result call (which doesn't get flagged by speedometer). Instead, go ahead and make there be an async path through this code that avoids the JTF.Run/Result code altogether. Note that only the first call to GetOption would hit this path, thus why I went down the expediant fix before. However, PR feedback has expressed a strong desire to not go that route, and thus the change here to add an async path to getting an option through IGlobalOptionService. There are *many* synchronous calls to GetOption, and this PR doesn't remove that codepath as that's outside the scope of the change that I'd like to make at this time. This PR only changes the GetOption calls that occur during package load as those were fairly easy to change and the most likely to cause the async work to occur.
4 participants
Jan reported a speedometer regression in Threads_AdjustedBackgroundJTFRunCount_devenv when trying to merge the next version roslyn branch into VS. This codepath has changed significantly due to the package initialization changes I've been making.
Specifically, RoslynPackage used to fire and forget a call to LoadOptionPersistersAsync in package initialization and now doesn't do this until after the package is loaded. Previously, after firing off this work, there would be a good amount of time before the first option was used, likely from ColorSchemaSettings.MigrateToColorSchemeSetting. Thus, the first call to GlobalOptions.GetOptionPersistersSlow would likely not have any async work remaining. My hypothesis is that even though a JTF.Run would have previously been invoked on a bg thread, it wouldn't have been reported as the task would have already been completed.
LoadOptionPersistersAsync was intentionally moved to after the package load, and thus it's intentional that we don't frontload this work. It's now expected that when the first option is requested, that there is still async work remaining.
The change in this PR makes it so if we are on a bg thread, we don't call JTF.Run in this context, instead just get the result directly. Blocking a bg thread while the options persisters finish loading isn't too concerning.
Insertion PR reporting regression: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/621620