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

Make workspace context creation/disposal free threaded #51016

Closed
1 of 2 tasks
drewnoakes opened this issue Feb 5, 2021 · 3 comments · Fixed by #51138
Closed
1 of 2 tasks

Make workspace context creation/disposal free threaded #51016

drewnoakes opened this issue Feb 5, 2021 · 3 comments · Fixed by #51138

Comments

@drewnoakes
Copy link
Member

drewnoakes commented Feb 5, 2021

The .NET Project System currently switches to the UI thread for invocations of:

https://github.com/dotnet/project-system/issues/353 is tracking updating our code to avoid those switches once these two APIs are free threaded. There did not appear to be a Roslyn issue tracking this work, hence this new issue.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 5, 2021
@CyrusNajmabadi
Copy link
Member

I poked around and tehse APIs already seem to be mostly free threaded (though i haven't guaranteed 100%). Parts that are currently suspect are:

            if (languageName == LanguageNames.FSharp)
            {
                var shell = (IVsShell)ServiceProvider.GlobalProvider.GetService(typeof(SVsShell));

                // Force the F# package to load; this is necessary because the F# package listens to WorkspaceChanged to 
                // set up some items, and the F# project system doesn't guarantee that the F# package has been loaded itself
                // so we're caught in the middle doing this.
                shell.LoadPackage(Guids.FSharpPackageId, out _);
            }

and

**            // HACK: since we're on the UI thread, ensure we initialize our options provider which depends on a UI-affinitized experimentation service
            _visualStudioWorkspaceImpl.EnsureDocumentOptionProvidersInitialized();**

Note: instead of being synchronous-free-threaded, could we potentially just make these both async? Would that fit into your side of things @drewnoakes ? That woudl allow us to trivially switch over, handlign UI concerns on our end. We coudl then move off of those pieces when we're able to, while unblocking you to not need to be on the ui thread.

Thoughts?

@drewnoakes
Copy link
Member Author

drewnoakes commented Feb 5, 2021

@CyrusNajmabadi that seems like a good approach to me. Both our call sites are already async, so the change would be straightforward from the Project System side.

@jinujoseph jinujoseph added Area-Performance Concept-Continuous Improvement and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 10, 2021
@jinujoseph jinujoseph added this to the 16.10 milestone Feb 10, 2021
@jasonmalinowski
Copy link
Member

@drewnoakes: Dispose() is already free-threaded, you don't need to transition for that one.

@CyrusNajmabadi: indeed, the create is not free threaded, but it's all silly crap that doesn't the UI thread. Switching it to JTF-friendly async we could do; some of that also might just be fixable directly to keep it off the UI thread entirely. The options stuff I'm looking at specifically because we have a few other bugs happening right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants