Start moving workspace events to fire on background threads #78134

ToddGrun · 2025-04-13T00:48:12Z

This addresses a major portion of #78091

There are still some WorkspaceChanged events that haven't been switched over. This is big enough without doing all those and I'll have a followup PR finish the rest of the roslyn workspace events.

This addresses a major portion of #78091 There are still some WorkspaceChanged events that haven't been switched over. This is big enough without doing all those and I'd like to get a ci run on this to see how broken tests are. Additionally, this didn't tackle adding the WorkspaceChangedEventOptions, that can be added later if we determine it would provide value.

...anguageServer/Protocol/Features/Diagnostics/DiagnosticAnalyzerService_IncrementalAnalyzer.cs

src/VisualStudio/Core/Def/LanguageService/AbstractCreateServicesOnTextViewConnection.cs

src/VisualStudio/Xaml/Impl/Implementation/XamlProjectService.cs

CyrusNajmabadi · 2025-04-13T01:10:18Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

@@ -596,6 +603,13 @@ protected internal Task ScheduleTask(Action action, string? taskName = "Workspac
        return _workQueue.WaitUntilCurrentBatchCompletesAsync();
    }

+    [SuppressMessage("Style", "VSTHRD200:Use \"Async\" suffix for async methods", Justification = "This is a Task wrapper, not an asynchronous method.")]
+    internal Task ScheduleBackgroundTask(Func<Task> action)


interesting. slightly surprised this returns a Task.

src/Workspaces/Core/Portable/Workspace/Workspace.cs

CyrusNajmabadi · 2025-04-13T01:11:33Z

src/Workspaces/Core/Portable/Workspace/WorkspaceEventRegistration.cs

+
+namespace Microsoft.CodeAnalysis;
+
+internal sealed class WorkspaceEventRegistration(Action unregisterAction) : IDisposable


possible fro this to be a struct?

Anythings possible! But, I don't know if it's desirable. I would expect most users to interact with this as a stored IDisposable, which means boxing if this is a struct. Also, this is only allocated during event registration, so not the highest of traffic paths.

That was my thinking:

These are allocated rarely (i.e. maybe a few each time you open a document.)

If we made it a struct I'd assume we would have to block copying of it, otherwise semantics might get a bit strange there.

I also proposed a type (versus directly returning IDisposable) because one thing I noticed while looking at existing uses of this is some users of WorkspaceChanged would benefit from a "wait until my handler has gotten all notifications up to this point", which probably means this gets a bit more state.

CyrusNajmabadi · 2025-04-13T01:13:32Z

src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs

        }

+        return Task.WhenAll(eventHandlerTasks);


interesting. i was not expecting this. i woudl not have thought that raising the event would have us wait on the receiver completing their work.

I'd be moer of a fan of the event just firing on the BG. and potentially telling receivers: you need to be synchronous. if you need async, add an item to your own async BG queue.

If this is the one I'm thinking of, it's a strange method in that it returns a Task representing the completion of the work...which all of the callers then ignore. I'd absolutely say we should change this signature if it wasn't the fact that's a breaking change...

src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs

src/Workspaces/Core/Portable/Workspace/Workspace_EventsAsync.cs

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/AsyncEventMap.cs

CyrusNajmabadi · 2025-04-13T01:19:14Z

Overall, this is giving me a little oogyness. I like teh high level idea. But i want to be on the same page on some of the details. Esp. as this would likely become a new public API, and we'll want to feel very confident about it.

2) Remove Obsolete attribute as that will require making the new registration public 3) Remove delegate passed to WorkspaceRegistration construction. 4) Switch new code to use SemaphoreSlim instead of NonReentrantLock 5) Rename methods on AsyncEventHandlerSet

CyrusNajmabadi · 2025-04-13T18:07:27Z

Do we need the async callback version at all. I don't mind these being events. Just issued in the bg. If we do need the async version, I'd like to know the case.

It's possible we might go with a public fire-synchronously model, and have a could internal async registration apis for the very special case.

ToddGrun · 2025-04-13T18:21:03Z

sued in the bg. If we do need the async version, I'd like to know the case.

It's possible we might go with a public fire-synchronously model, and have a could internal async registration apis for the very s

I think it was mostly to support handlers that needed to run on the main thread to do the switch itself without resorting to JTF usage. However, maybe the registration could just take in a flag indicating the threading context it prefers to be called on, and then we don't need the whole task part of the api. @jasonmalinowski -- thoughts?

CyrusNajmabadi · 2025-04-13T19:24:14Z

I think it was mostly to support handlers that needed to run on the main thread to do the switch itself without resorting to JTF usage.

I'd love if we either:

didn't have those.
if we had those, it was only an internal capability.
if we think we need those, go look at the feature that needs it, still have it hear about BG events, and have it switch to teh UI thread itself in the future, instead of on the event firing thread.

basically, don't design the work here about what is (hopefully) a single case, or potentially a case we can remove :)

jasonmalinowski · 2025-04-14T20:46:27Z

I think it was mostly to support handlers that needed to run on the main thread to do the switch itself without resorting to JTF usage. However, maybe the registration could just take in a flag indicating the threading context it prefers to be called on, and then we don't need the whole task part of the api. @jasonmalinowski -- thoughts?

That was one reason I had in mind; I also wasn't sure if you'd see cases in external code where in a WorkspaceChanged handler they're trying to update some shared state and need to acquire a lock. And generally, from an API perspective, it's a bit easier to allow for asynchrony (and maybe folks are returning Task.CompletedTask) rather than not having it, and then a use case discovering you needed it. There's a bit of a bias from our codebase since we have AsyncBatchingWorkQueue, we tend to use it in most of the places using WorkspaceChanged, but other consumers might not be as lucky -- we probably should try to find some external uses if we can to see what they're all doing.

We could make the threading context explicit, but I think in the case of Inline Rename it could still start off the UI thread where it does filtering, and then move to the UI thread once it knows something needs to happen. But it could also do that "fire and forget". If the concern about adding more overloads, then of course having an overload take the configuration isn't helping that....

I don't have a problem though with having an overload that just takes Action and calls the other method as a simplification. Or maybe for round one (since we're right now focusing on making an API for internal use) we just make it synchronous and find a different approach for the handlers that might need async. My 'just make it async' was more the general habit to avoid discovering later we needed it. But we could keep it that way...and maybe use ValueTask instead.

(I do have a temptation to file a C# LDM proposal for a 'sync' modifier on a method that makes it clear that yes, I want the compiler to transform it to task-returning, but yes, I know it's synchronous without the ugliness of the pragma.)

…el that takes in an eventoptions that allows picking the callback thread

…eed it

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

CyrusNajmabadi · 2025-04-15T21:15:41Z

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

+    private void AddEventHandler<TEventArgs>(EventHandler<TEventArgs> eventHandler, WorkspaceEventType eventType)
+        where TEventArgs : EventArgs
+    {
+        // Require main thread on the callback as this is used from publicly exposed eventss


Suggested change

// Require main thread on the callback as this is used from publicly exposed eventss

// Require main thread on the callback as this is used from all the existing publicly exposed events

// which historically ran on the UI thread, and which extenders almost certainly have taken a

// dependency on.

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

2) Add assert 3) Formatting changes

…lls in LspWorkspaceRegistrationServices weren't doing matched calls to workspace changed registration

ToddGrun · 2025-04-20T12:53:37Z

test insertion doesn't show any regressions: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/629923

...rFeatures/Core/Classification/Syntactic/SyntacticClassificationTaggerProvider.TagComputer.cs

jasonmalinowski

Not entirely done, but leaving comments so far.

src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs

jasonmalinowski · 2025-04-24T00:35:10Z

...re/Shared/Tagging/EventSources/TaggerEventSources.DocumentActiveContextChangedEventSource.cs

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


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.

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.

jasonmalinowski · 2025-04-24T00:38:15Z

src/Features/Core/Portable/Diagnostics/Service/DiagnosticAnalyzerService_IncrementalAnalyzer.cs

@@ -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());


....is this ever unsubscribed???

nope. It wasn't before, and I considered addressing that outside the scope of this PR. :)

jasonmalinowski · 2025-04-24T00:43:38Z

src/VisualStudio/Core/Def/LanguageService/AbstractCreateServicesOnTextViewConnection.cs

@@ -56,9 +57,12 @@ public AbstractCreateServicesOnTextViewConnection(
                listenerProvider.GetListener(FeatureAttribute.CompletionSet),
                threadingContext.DisposalToken);

-        Workspace.DocumentOpened += QueueWorkOnDocumentOpened;
+        _workspaceDocumentOpenedDisposer = Workspace.RegisterDocumentOpenedHandler(QueueWorkOnDocumentOpened);


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?

Maybe. If you have the chance, can you log a bug and I'll take a look as a separate issue?

Filed #78355

...IntegrationTest/New.IntegrationTests/InProcess/ITextViewWindowVerifierInProcessExtensions.cs

jasonmalinowski · 2025-04-24T01:04:24Z

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

+        if (disposer == null)
+        {
+            disposer = RegisterHandler(eventType, (Action<EventArgs>)Handler, WorkspaceEventOptions.RequiresMainThreadOptions);
+
+            lock (_gate)
+            {
+                _disposableEventHandlers[(eventHandler, eventType)] = (AdviseCount: 1, disposer);
+            }
+        }


Why did we jump out of the lock here? Since there's a theoretical race if you had two parallel subscriptions where the TryGetValue misses both times, and then hits this path and the write happens twice. In that case AdviseCount should be 2, not 1.

(I can't imagine this ever happening in practice.)

I had concerns about calling RegisterHandler inside the lock, but looking a bit closer, maybe that concern was unnecessary. I'll change this to not transition into the lock multiple times and we can chat more about this tomorrow.

@jasonmalinowski -- How does this look? Can we chat tomorrow on any remaining concerns you may have?

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

2) Rename some methods/members in Workspace_EventsLegacy.cs file 3) Fix potential multi-threaded issue in AddLegacyEventHandler by only taking the lock once 4) Be a bit more paranoid in WorkspaceEventRegistration.Dispose

src/Workspaces/Core/Portable/Workspace/Workspace_EventsLegacy.cs

jasonmalinowski · 2025-04-25T22:10:10Z

src/EditorFeatures/CSharpTest/Workspaces/WorkspaceTests_EditorFeatures.cs

I have no idea why this file is here rather than the Workspaces layer. Or minimally, why it's in the C# folder...

src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs

jasonmalinowski

Really great piece of work here -- there's a lot of really subtle behavior and edge cases that we could break here, and I you've done a really good job trying to keep those working while also keeping everything quite clean. Well done; this will be good to see how it does once it's in.

jasonmalinowski · 2025-04-25T22:18:20Z

src/Workspaces/Core/Portable/Workspace/Workspace_Events.cs

+    /// <summary>
+    /// Registers a handler that is fired whenever the current solution is changed.
+    /// </summary>
+    internal WorkspaceEventRegistration RegisterWorkspaceChangedImmediateHandler(Action<WorkspaceChangeEventArgs> handler, WorkspaceEventOptions? options = null)


I imagine the options passed here don't matter right, since this one gets called synchronously and bypasses the queue? Should this not take options here?

I think it's better to take in the options, for both consistency and for possible future considerations. However, I will update the comment to better reflect that the passed in threading preferences won't be adhered to.

src/Workspaces/Core/Portable/Workspace/Workspace_Registration.cs

jasonmalinowski · 2025-04-25T22:27:52Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+    }
+
+    [SuppressMessage("Style", "VSTHRD200:Use \"Async\" suffix for async methods", Justification = "This is a Task wrapper, not an asynchronous method.")]
+    internal Task ScheduleTask(EventArgs args, EventHandlerSet handlerSet)


Maybe call this AddToEventHandlerWorkQueue or something, so it's very clear this isn't doing the thread scheduling that the existing method is doing.

I think I'm going to avoid this one in this PR. All the "ScheduleTask" methods go through this method, so they are all kind of misnamed now. Add to that the whole Task returning behaviors for these methods, and I think this needs to be thought through a bit more.

jasonmalinowski · 2025-04-25T22:39:33Z

src/Workspaces/Core/Portable/Workspace/Workspace.cs

+        // Verify there are handlers which require the main thread before performing the thread switch.
+        if (list.Any(static x => x.HandlerSet.HasMatchingOptions(isMatch: static options => options.RequiresMainThread)))


Did you consider having us kick off the work to the main thread prior to doing the background thread ones, and then awaiting the task once we're done? At least that way there's a bit more happening in parallel and the change is fairly trivial.

Concurrent handler invocation is definitely something we should consider and worthy of it's own effort as there are a lot of possibilities there with a lot of potential snags.

jasonmalinowski · 2025-04-25T22:46:51Z

src/Workspaces/Core/Portable/Workspace/WorkspaceEventOptions.cs

+internal record struct WorkspaceEventOptions(bool RequiresMainThread)
+{
+    public static readonly WorkspaceEventOptions DefaultOptions = new(RequiresMainThread: false);
+    public static readonly WorkspaceEventOptions RequiresMainThreadOptions = new(RequiresMainThread: true);


On one hand, call it the "main thread" is bugging me here since from the workspace perspective it doesn't really have a concept of the main thread like JTF, it's just "the scheduler we schedule things to". On the other hand, we all know what this means and I can't come up with a better name.

Will keep it for now as I can't come up with something better (foreground, ui, main all have this same association).

ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 13, 2025

dotnet-policy-service bot added the VSCode label Apr 13, 2025