-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Start moving workspace events to fire on background threads #78134
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
Changes from all commits
c553551
71b1d43
c2e874f
cdceae8
b0a49f8
b1f1b52
c48694a
be3fd62
2254c05
eaec791
7ad41df
7c21263
dbc299e
76229c7
92876f7
d0496b2
45c094f
34dd6ed
5c66609
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,20 +11,21 @@ internal partial class TaggerEventSources | |
| { | ||
| private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subjectBuffer) : AbstractWorkspaceTrackingTaggerEventSource(subjectBuffer) | ||
| { | ||
| private WorkspaceEventRegistration? _documentActiveContextChangedDisposer; | ||
|
|
||
| // Require main thread on the callback as RaiseChanged implementors may have main thread dependencies. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this a generic concern about risk, or do we actually have a known case where we still have a dependency? What's surprising here to me is we have a bunch of event sources, not just this one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't do the transitive look into whether advisers to AbstractTaggerEventSource.Changed have main thread dependencies. Generally, I wanted to be conservative in the callbacks moved off the main thread in this PR. I plan to have another PR after this one that will finish off the workspace event switch to the new API and at that point, I'll reprofile and see if any of the remaining main thread callbacks are showing up in the profile. |
||
| protected override void ConnectToWorkspace(Workspace workspace) | ||
| => workspace.DocumentActiveContextChanged += OnDocumentActiveContextChanged; | ||
| => _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions); | ||
|
|
||
| protected override void DisconnectFromWorkspace(Workspace workspace) | ||
| => workspace.DocumentActiveContextChanged -= OnDocumentActiveContextChanged; | ||
| => _documentActiveContextChangedDisposer?.Dispose(); | ||
|
|
||
| private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs e) | ||
| private void OnDocumentActiveContextChanged(DocumentActiveContextChangedEventArgs e) | ||
| { | ||
| var document = SubjectBuffer.AsTextContainer().GetOpenDocumentInCurrentContext(); | ||
|
|
||
| if (document != null && document.Id == e.NewActiveContextDocumentId) | ||
| { | ||
| this.RaiseChanged(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,11 +14,8 @@ private DiagnosticIncrementalAnalyzer CreateIncrementalAnalyzer(Workspace worksp | |
| private DiagnosticIncrementalAnalyzer CreateIncrementalAnalyzerCallback(Workspace workspace) | ||
| { | ||
| // subscribe to active context changed event for new workspace | ||
| workspace.DocumentActiveContextChanged += OnDocumentActiveContextChanged; | ||
| _ = workspace.RegisterDocumentActiveContextChangedHandler(args => RequestDiagnosticRefresh()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ....is this ever unsubscribed???
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope. It wasn't before, and I considered addressing that outside the scope of this PR. :) |
||
|
|
||
| return new DiagnosticIncrementalAnalyzer(this, AnalyzerInfoCache, this.GlobalOptions); | ||
| } | ||
|
|
||
| private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs e) | ||
| => RequestDiagnosticRefresh(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,10 +26,11 @@ namespace Microsoft.VisualStudio.LanguageServices.Implementation.LanguageService | |
| /// Creates services on the first connection of an applicable subject buffer to an IWpfTextView. | ||
| /// This ensures the services are available by the time an open document or the interactive window needs them. | ||
| /// </summary> | ||
| internal abstract class AbstractCreateServicesOnTextViewConnection : IWpfTextViewConnectionListener | ||
| internal abstract class AbstractCreateServicesOnTextViewConnection : IWpfTextViewConnectionListener, IDisposable | ||
| { | ||
| private readonly string _languageName; | ||
| private readonly AsyncBatchingWorkQueue<ProjectId?> _workQueue; | ||
| private readonly WorkspaceEventRegistration _workspaceDocumentOpenedDisposer; | ||
ToddGrun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| private bool _initialized = false; | ||
|
|
||
| protected VisualStudioWorkspace Workspace { get; } | ||
|
|
@@ -56,9 +57,12 @@ public AbstractCreateServicesOnTextViewConnection( | |
| listenerProvider.GetListener(FeatureAttribute.CompletionSet), | ||
| threadingContext.DisposalToken); | ||
|
|
||
| Workspace.DocumentOpened += QueueWorkOnDocumentOpened; | ||
| _workspaceDocumentOpenedDisposer = Workspace.RegisterDocumentOpenedHandler(QueueWorkOnDocumentOpened); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have correctly maintained this code's behavior, but as we're continuing to push on load performance, this is really not ideal that we're forcing the VisualStudioWorkspace to be created when a C# file is opened. If all you're doing is opening a loose file...that was extra work for no real benefit, maybe?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe. If you have the chance, can you log a bug and I'll take a look as a separate issue?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed #78355 |
||
| } | ||
|
|
||
| public void Dispose() | ||
| => _workspaceDocumentOpenedDisposer.Dispose(); | ||
ToddGrun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| void IWpfTextViewConnectionListener.SubjectBuffersConnected(IWpfTextView textView, ConnectionReason reason, Collection<ITextBuffer> subjectBuffers) | ||
| { | ||
| if (!_initialized) | ||
|
|
@@ -96,7 +100,7 @@ private async ValueTask BatchProcessProjectsWithOpenedDocumentAsync(ImmutableSeg | |
| } | ||
| } | ||
|
|
||
| private void QueueWorkOnDocumentOpened(object sender, DocumentEventArgs e) | ||
| private void QueueWorkOnDocumentOpened(DocumentEventArgs e) | ||
| { | ||
| if (e.Document.Project.Language == _languageName) | ||
| _workQueue.AddWork(e.Document.Project.Id); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.