Avoid decoding all SuppressMessageAttributes up front#83087
Avoid decoding all SuppressMessageAttributes up front#83087333fred merged 9 commits intodotnet:mainfrom
Conversation
Currently, when requesting whether any attribute suppresses a specific diagnostic, we will eagerly decode all `SuppressMessageAttribute`s in the global context. This is potentially a lot of work unrelated to the diagnostic being queried, especially if the compilation then doesn't produce any of the diagnostic that then is resolved. To avoid this, we now only resolve symbols for suppressions of a specific diagnostic ID when that diagnostic ID is requested, and never otherwise. This improves perf of a 1st-party partner team's CI by ~5x.
|
@dotnet/roslyn-compiler for review |
| @@ -71,12 +59,13 @@ public void AddCompilationWideSuppression(SuppressMessageInfo info) | |||
| public void AddGlobalSymbolSuppression(SuppressMessageInfo info, TargetScope scope) | |||
| { | |||
| Debug.Assert(!_lazyResolvedSuppressionsById.ContainsKey(info.Id)); | |||
There was a problem hiding this comment.
nit: perhaps we could strengthen the assert and require the resolved suppression dictionary is empty while we construct the unresolved dictionary. #Resolved
|
@dotnet/roslyn-compiler for a second review |
|
Consider adding a test or referencing a test to illustrate the issue. That would make the issue being fixed clearer. Also, such a test could be used as benchmark, to show the impact of the PR. |
| private readonly Dictionary<ISymbol, Dictionary<string, SuppressMessageInfo>> _globalSymbolSuppressions = new Dictionary<ISymbol, Dictionary<string, SuppressMessageInfo>>(); | ||
| // Keep targeted suppressions grouped by diagnostic ID so we only resolve target symbols | ||
| // for IDs that are actually queried. | ||
| private readonly Dictionary<string, List<(SuppressMessageInfo Info, TargetScope Scope)>> _unresolvedSuppressionsById = new Dictionary<string, List<(SuppressMessageInfo Info, TargetScope Scope)>>(StringComparer.Ordinal); |
There was a problem hiding this comment.
It feels like this could be simplified. The SuppressMessageInfo.Scope string is redundant with this TargetScope Scope. I'd be tempted to remove Scope here or store a TargetScope in SuppressMessageInfo (instead of string Scope), unless there's an observable effect on benchmark.
#Closed
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (commit 2)
…ture * upstream/main: (77 commits) Fix ArgumentNullException in VB Edit and Continue (dotnet#83250) Fix property pattern completion filtering out member being edited (dotnet#83230) Add branch merge skill (dotnet#83229) [main] Source code updates from dotnet/dotnet (dotnet#83215) Support MatchPriority comparison in LSP completion (dotnet#83164) Have CompleteStatement handle EOF statements (dotnet#83205) Minor cleanups related to attributes in VB (dotnet#83206) Simplify Address additional PR feedback Port remaining unit test projects to Linux (dotnet#83153) Unsafe evolution: allow unsafe property accessors (dotnet#83115) Address PR review feedback Allow cohost rename in Razor source-generated docs Refine code review skill (dotnet#82666) Review feedback Allow creation of DocumentUri instances even if System.Uri cannot parse it Improve TextLine and line table performance by packing existing data into unused bits (dotnet#83000) Skip TestFindReferencesAsync_UsingAlias on non-Windows platforms (dotnet#83188) [main] Source code updates from dotnet/dotnet (dotnet#83174) fix comment ...
|
@jcouv for another look please. |
| var resolvedSuppressions = new Dictionary<ISymbol, SuppressMessageInfo>(); | ||
| foreach (var info in suppressions) | ||
| { | ||
| foreach (var target in ResolveTargetSymbols(_compilation, info.Target, info.Scope)) |
| } | ||
|
|
||
| if (TryGetTargetScope(info, out TargetScope scope)) | ||
| if (info.Scope != TargetScope.Invalid) |
There was a problem hiding this comment.
nit: consider inverting if to decrease nesting (if (invalid) { continue; } if (info is { Scope: ... } ) { continue; }
|
Gentle ping on this comment In reply to: 4276450884 |
jcouv
left a comment
There was a problem hiding this comment.
LGTM Thanks (commit 5) with minor comments to consider. I still think it would be good to include a targeted/stress test
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Currently, when requesting whether any attribute suppresses a specific diagnostic, we will eagerly decode all
SuppressMessageAttributes in the global context. This is potentially a lot of work unrelated to the diagnostic being queried, especially if the compilation then doesn't produce any of the diagnostic that then is resolved. To avoid this, we now only resolve symbols for suppressions of a specific diagnostic ID when that diagnostic ID is requested, and never otherwise. This improves perf of a 1st-party partner team's CI by ~5x.Microsoft Reviewers: Open in CodeFlow