Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion src/EditorFeatures/Core/Remote/SolutionChecksumUpdater.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,13 @@ public SolutionChecksumUpdater(

_shutdownToken = shutdownToken;

// A time span of Short is chosen here to ensure that the batching favors fewer but larger batches.
// During solution load a large number of WorkspaceChange events might be raised over a few seconds,
// and in performance tests a 50ms delay was found to be causing a lot of extra memory churn synchronizing
// things OOP. Short didn't cause a similar issue; it's possible this will need to be fine tuned for something in
// the middle.
_synchronizeWorkspaceQueue = new AsyncBatchingWorkQueue(
DelayTimeSpan.NearImmediate,
DelayTimeSpan.Short,
Copy link
Member

Choose a reason for hiding this comment

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

250 'feels' good to me. It's less than a user could ever notice, while allowing a lot to batch up in firehose scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I imagine this will be noticeable to those users who can perfectly time opening a document right after solution load completes and can do their first OOP dependent operation within 200ms.

SynchronizePrimaryWorkspaceAsync,
listener,
shutdownToken);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,8 @@ private sealed class DocumentActiveContextChangedEventSource(ITextBuffer subject
{
private WorkspaceEventRegistration? _documentActiveContextChangedDisposer;

// Require main thread on the callback as RaiseChanged implementors may have main thread dependencies.
protected override void ConnectToWorkspace(Workspace workspace)
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
=> _documentActiveContextChangedDisposer = workspace.RegisterDocumentActiveContextChangedHandler(OnDocumentActiveContextChanged);

protected override void DisconnectFromWorkspace(Workspace workspace)
=> _documentActiveContextChangedDisposer?.Dispose();
Expand Down
2 changes: 1 addition & 1 deletion src/EditorFeatures/Core/Tagging/ITaggerEventSource.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ internal interface ITaggerEventSource

/// <summary>
/// An event has happened on the thing the tagger is attached to. The tagger should
/// recompute tags.
/// recompute tags. May be raised on any thread.
/// </summary>
event EventHandler<TaggerEventArgs> Changed;
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,7 @@ public CodeAnalysisDiagnosticAnalyzerService(
_workspace = workspace;
_diagnosticAnalyzerService = _workspace.Services.GetRequiredService<IDiagnosticAnalyzerService>();

// Main thread as OnWorkspaceChanged's call to IDiagnosticAnalyzerService.RequestDiagnosticRefresh isn't clear on
// threading requirements
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
_ = workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChanged);
}

private void OnWorkspaceChanged(WorkspaceChangeEventArgs e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ internal interface IDiagnosticAnalyzerService : IWorkspaceService
/// <summary>
/// Re-analyze all projects and documents. This will cause an LSP diagnostic refresh request to be sent.
/// </summary>
/// <remarks>
/// This implementation must be safe to call on any thread.
/// </remarks>
void RequestDiagnosticRefresh();

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ public virtual void Register(Workspace? workspace)
m["WorkspacePartialSemanticsEnabled"] = workspace.PartialSemanticsEnabled;
}, workspace));

// Forward workspace change events for all registered LSP workspaces. Requires main thread as it
// fires LspSolutionChanged which hasn't been guaranteed to be thread safe.
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);
var workspaceChangedDisposer = workspace.RegisterWorkspaceChangedHandler(OnLspWorkspaceChanged);

lock (_gate)
{
Expand Down Expand Up @@ -94,9 +92,7 @@ public void Dispose()
}

/// <summary>
/// Indicates whether the LSP solution has changed in a non-tracked document context.
///
/// <b>IMPORTANT:</b> Implementations of this event handler should do as little synchronous work as possible since this will block.
/// Indicates whether the LSP solution has changed in a non-tracked document context. May be raised on any thread.
/// </summary>
public EventHandler<WorkspaceChangeEventArgs>? LspSolutionChanged;
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ public StackTraceExplorerViewModel(IThreadingContext threadingContext, Workspace
_threadingContext = threadingContext;
_workspace = workspace;

// Main thread dependency as Workspace_WorkspaceChanged modifies an ObservableCollection
_ = workspace.RegisterWorkspaceChangedHandler(Workspace_WorkspaceChanged, WorkspaceEventOptions.RequiresMainThreadOptions);

_classificationTypeMap = classificationTypeMap;
_formatMap = formatMap;

Expand Down Expand Up @@ -103,15 +100,6 @@ private void CallstackLines_CollectionChanged(object sender, System.Collections.
NotifyPropertyChanged(nameof(IsInstructionTextVisible));
}

private void Workspace_WorkspaceChanged(WorkspaceChangeEventArgs e)
{
if (e.Kind == WorkspaceChangeKind.SolutionChanged)
{
Selection = null;
Frames.Clear();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

why was this safe/correct to remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah comment was in the previous PR of this: I suspect the intended behavior of this code was to clear out the list of frames on Solution Close; i.e. you closed the solution so you're done with it. But the code had a typo of "SolutionChanged", which doesn't make any sense at all. It means you'll clear the frame list on something like a project being loaded (which will do a SolutionChanged if it impacts project-to-project references)....but not anything else? Clearly we haven't had problems with it not being cleared on SolutionClose, and I didn't understand why that'd be good anyways (maybe you've pasted in a stack and you need to switch the solution to navigate to it?) so I just deleted this.


private FrameViewModel GetViewModel(ParsedFrame frame)
=> frame switch
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ internal sealed partial class CpsDiagnosticItemSource : BaseDiagnosticAndGenerat
{
private readonly IVsHierarchyItem _item;
private readonly string _projectDirectoryPath;
private readonly string? _analyzerFilePath;

private WorkspaceEventRegistration? _workspaceChangedDisposer;

Expand All @@ -37,6 +38,8 @@ public CpsDiagnosticItemSource(
_item = item;
_projectDirectoryPath = Path.GetDirectoryName(projectPath);

_analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, _item.CanonicalName);

this.AnalyzerReference = TryGetAnalyzerReference(Workspace.CurrentSolution);
if (this.AnalyzerReference == null)
{
Expand All @@ -47,9 +50,7 @@ public CpsDiagnosticItemSource(
// then connect to it.
if (workspace.CurrentSolution.ContainsProject(projectId))
{
// Main thread dependency as OnWorkspaceChangedLookForAnalyzer accesses the IVsHierarchy
// and fires the PropertyChanged event
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer, WorkspaceEventOptions.RequiresMainThreadOptions);
_workspaceChangedDisposer = Workspace.RegisterWorkspaceChangedHandler(OnWorkspaceChangedLookForAnalyzer);
item.PropertyChanged += IVsHierarchyItem_PropertyChanged;

// Now that we've subscribed, check once more in case we missed the event
Expand Down Expand Up @@ -118,14 +119,11 @@ private void OnWorkspaceChangedLookForAnalyzer(WorkspaceChangeEventArgs e)
return null;
}

var canonicalName = _item.CanonicalName;
var analyzerFilePath = CpsUtilities.ExtractAnalyzerFilePath(_projectDirectoryPath, canonicalName);

if (string.IsNullOrEmpty(analyzerFilePath))
if (string.IsNullOrEmpty(_analyzerFilePath))
{
return null;
}

return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, analyzerFilePath, StringComparison.OrdinalIgnoreCase));
return project.AnalyzerReferences.FirstOrDefault(r => string.Equals(r.FullPath, _analyzerFilePath, StringComparison.OrdinalIgnoreCase));
}
}
Loading