Skip to content

Do not call JTF.Run from a bg thread in options initialization #77788

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

Merged
merged 1 commit into from
Mar 25, 2025
Merged

Do not call JTF.Run from a bg thread in options initialization #77788

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/Workspaces/Core/Portable/Options/GlobalOptionService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,9 @@ static ImmutableArray<IOptionPersister> GetOptionPersistersSlow(
ImmutableArray<Lazy<IOptionPersisterProvider>> persisterProviders,
CancellationToken cancellationToken)
{
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.
Copy link
Member

@jasonmalinowski jasonmalinowski Mar 25, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 25, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@AArnott AArnott Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 25, 2025

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@ToddGrun ToddGrun Mar 25, 2025

Choose a reason for hiding this comment

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

I've gone ahead and pushed out a PR with one potential way to address this by making GetOption have async variants:

#77808

return workspaceThreadingService.Run(() => GetOptionPersistersAsync(persisterProviders, cancellationToken));
}
else
Expand Down
Loading