Reduce Roslyn packages impact on UI thread #74501
Conversation
AsyncPackage does not need to be called as called out by the documentation.
GetServiceAsync doesn't need the UI thread, and neither does the cast to IComponentModel. I would have switched this to GetServiceAsync<TService, TInterface> which will do the right thing, but that is banned in this repository.
RegisterLanguageService and RegisterService do not require the UI thread.
SwitchToMainThreadAync will throw if its cancelled when it resumes.
All of these actions have no UI thread affinity based on inspection.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
dotnet-policy-service
bot
added
the
Community
CyrusNajmabadi
requested review from
jasonmalinowski,
sharwell,
CyrusNajmabadi and
ToddGrun
| @@ -27,10 +27,6 @@ internal IComponentModel ComponentModel | |||
|
|
|||
| protected override async Task InitializeAsync(CancellationToken cancellationToken, IProgress<ServiceProgressData> progress) | |||
| { | |||
| await base.InitializeAsync(cancellationToken, progress).ConfigureAwait(true); | |||
There was a problem hiding this comment.
is it ok/intentional to not cal the base here? i'd actually like a comment saying that thsi is intentional, so it doesn't come backk in the future.
There was a problem hiding this comment.
We could doc it if you want, but no package in VS calls InitializeAsync anymore. I've documented this.
There was a problem hiding this comment.
doc'ing would def be nice. just so we have smething to state that this is very intentional as opposed to accidental.
|
|
||
| var shell = (IVsShell7)await GetServiceAsync(typeof(SVsShell)).ConfigureAwait(true); | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| Assumes.Present(shell); |
There was a problem hiding this comment.
could this move below the foreach? is that beneficial? my thinking is smiply that we don't acutaly need this service until later, so why not delay till then?
There was a problem hiding this comment.
It can, there is no benefit to it being here. This is effectively a synchronous call anyway because SVsShell is always existing sync service, it never yields.
There was a problem hiding this comment.
Because Roslyn has their own version of these extensions, I would love to be able to replace all three lines with just the following but I can't until its "unbanned":.
var shell = this.GetServiceAsync<SVsShell, IVsShell>(cancellationToken)
|
@jasonmalinowski @sharwell @ToddGrun ptal. These PRs are very relevant for helping with VS startup time. Let's work fast to unblock dave. |
| await JoinableTaskFactory.SwitchToMainThreadAsync(ct); | ||
| return new TempPECompilerService(workspace.Services.GetService<IMetadataService>()); | ||
| return new TempPECompilerService(metadataService); |
There was a problem hiding this comment.
(Same question applies for the VisualBasicPackage.InitializeAsync)
There was a problem hiding this comment.
Tagging @davkean just to make sure this question has been seen
|
Sorry folks, I'll need to get back to this next week, I haven't tested this and this clearly has fallout so need to spend time understanding the ramifications. |
|
@davkean Is this still needing reviews or needing some more engineering work? |
|
This really needs someone familar with the code to pick up, and treat my PR as a proposal. We can close this for now, but this will become an ask this quarter as we start focusing on startup scenarios. |
|
@sharwell could you please help with this? |
Based on our UI and hang data, Roslyn's packages are one the most expensive packages during solution load. This is an attempt at moving as much as posssible of the Roslyn packages to a background thread, removing unnecessarily UI thread dependencies. This is entirely untested - and I expect to run this through a full insertion before merge.