-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix race condition in analyzer driver where multiple threads attempti… #25496
Changes from 1 commit
0b6152b
4755064
c77f475
a7bb57c
9685356
f7a7565
8779fa9
c4fda78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,14 +21,14 @@ internal class CompilationData | |
private readonly Dictionary<SyntaxTree, SemanticModel> _semanticModelsMap; | ||
|
||
private readonly Dictionary<SyntaxReference, DeclarationAnalysisData> _declarationAnalysisDataMap; | ||
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 _declarationAnalysisDataMap pure cache or state? looks like a cache 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. Its just cache, it is fine to recompute the data if it does not exist but computation was showing up in FSA traces, so we added this cache. This was done before OOP, so in future we may want to experiment removing the cache completely and see if it is indeed an issue anymore. |
||
private readonly ObjectPool<DeclarationAnalysisData> _declarationAnalysisDataPool; | ||
private readonly ObjectPool<DeclarationAnalysisDataBuilder> _declarationAnalysisDataBuilderPool; | ||
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. not sure whether this pool is needed... 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. This was added before OOP for perf tuning FSA scenarios, when FSA was on by default. We can either retain this pool for now and remove in a separate PR OR do it in this PR itself. I thought it is better to separate perf work in separate PR. |
||
|
||
public CompilationData(Compilation comp) | ||
{ | ||
_semanticModelsMap = new Dictionary<SyntaxTree, SemanticModel>(); | ||
this.SuppressMessageAttributeState = new SuppressMessageAttributeState(comp); | ||
_declarationAnalysisDataMap = new Dictionary<SyntaxReference, DeclarationAnalysisData>(); | ||
_declarationAnalysisDataPool = new ObjectPool<DeclarationAnalysisData>(() => new DeclarationAnalysisData()); | ||
_declarationAnalysisDataBuilderPool = new ObjectPool<DeclarationAnalysisDataBuilder>(() => new DeclarationAnalysisDataBuilder()); | ||
} | ||
|
||
public SuppressMessageAttributeState SuppressMessageAttributeState { get; } | ||
|
@@ -64,31 +64,48 @@ public bool RemoveCachedSemanticModel(SyntaxTree tree) | |
|
||
internal DeclarationAnalysisData GetOrComputeDeclarationAnalysisData( | ||
SyntaxReference declaration, | ||
Func<Func<DeclarationAnalysisData>, DeclarationAnalysisData> computeDeclarationAnalysisData, | ||
Func<Func<DeclarationAnalysisDataBuilder>, DeclarationAnalysisDataBuilder> computeDeclarationAnalysisData, | ||
bool cacheAnalysisData) | ||
{ | ||
if (!cacheAnalysisData) | ||
DeclarationAnalysisDataBuilder dataBuilder = null; | ||
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. this is a super strange pattern for pooling and retrieving objects. it seems very error prone. |
||
try | ||
{ | ||
return computeDeclarationAnalysisData(_declarationAnalysisDataPool.Allocate); | ||
} | ||
if (!cacheAnalysisData) | ||
{ | ||
dataBuilder = computeDeclarationAnalysisData(_declarationAnalysisDataBuilderPool.Allocate); | ||
return DeclarationAnalysisData.CreateFrom(dataBuilder); | ||
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. 💡 If using the builder pattern, consider using an approach like immutable collections:
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. This code is from previous iteration. |
||
} | ||
|
||
DeclarationAnalysisData data; | ||
lock (_declarationAnalysisDataMap) | ||
{ | ||
if (_declarationAnalysisDataMap.TryGetValue(declaration, out data)) | ||
DeclarationAnalysisData data; | ||
lock (_declarationAnalysisDataMap) | ||
{ | ||
return data; | ||
if (_declarationAnalysisDataMap.TryGetValue(declaration, out data)) | ||
{ | ||
return data; | ||
} | ||
} | ||
} | ||
|
||
data = computeDeclarationAnalysisData(_declarationAnalysisDataPool.Allocate); | ||
dataBuilder = computeDeclarationAnalysisData(_declarationAnalysisDataBuilderPool.Allocate); | ||
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. so, originally pool was there to reduce DeclarationAnalysisData type (and Lists in it). but now we always create those (including immutable arrays) this now introduces new type builder which is pooled (so its builders as well) but all these are new types. so, pooling this doesnt include new allocations but also doesnt reduce any. so, at the end, it is same as not having pool at all since we always create DeclarationAnalysisData (and its array). if we are going this approach, I think we should just remove the pool. by the way, the pool is not static. and I think we know doesnt share same analyzer driver between compilation. so do we actually need this clear declaration analysis data anymore? 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 would rather make sure AnalyzerDriver state is not shared between compilation with analyzer in host and deprecate ClearCacheXXX methods. 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. Sure, let me get rid of this pool as well and make it cleaner. |
||
|
||
lock (_declarationAnalysisDataMap) | ||
lock (_declarationAnalysisDataMap) | ||
{ | ||
if (!_declarationAnalysisDataMap.TryGetValue(declaration, out data)) | ||
{ | ||
data = DeclarationAnalysisData.CreateFrom(dataBuilder); | ||
_declarationAnalysisDataMap.Add(declaration, data); | ||
} | ||
} | ||
|
||
return data; | ||
} | ||
finally | ||
{ | ||
_declarationAnalysisDataMap[declaration] = data; | ||
if (dataBuilder != null) | ||
{ | ||
dataBuilder.Free(); | ||
_declarationAnalysisDataBuilderPool.Free(dataBuilder); | ||
} | ||
} | ||
|
||
return data; | ||
} | ||
|
||
internal void ClearDeclarationAnalysisData(SyntaxReference declaration) | ||
|
@@ -102,54 +119,8 @@ internal void ClearDeclarationAnalysisData(SyntaxReference declaration) | |
} | ||
|
||
_declarationAnalysisDataMap.Remove(declaration); | ||
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 think now you can just do without TryGetValue since you don't need data anymore 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. Yes, sure. |
||
|
||
declarationData.Free(); | ||
_declarationAnalysisDataPool.Free(declarationData); | ||
} | ||
} | ||
} | ||
|
||
internal class DeclarationAnalysisData | ||
{ | ||
/// <summary> | ||
/// GetSyntax() for the given SyntaxReference. | ||
/// </summary> | ||
public SyntaxNode DeclaringReferenceSyntax { get; set; } | ||
|
||
/// <summary> | ||
/// Topmost declaration node for analysis. | ||
/// </summary> | ||
public SyntaxNode TopmostNodeForAnalysis { get; set; } | ||
|
||
/// <summary> | ||
/// All member declarations within the declaration. | ||
/// </summary> | ||
public List<DeclarationInfo> DeclarationsInNode { get; } | ||
|
||
/// <summary> | ||
/// All descendant nodes for syntax node actions. | ||
/// </summary> | ||
public List<SyntaxNode> DescendantNodesToAnalyze { get; } | ||
|
||
/// <summary> | ||
/// Flag indicating if this is a partial analysis. | ||
/// </summary> | ||
public bool IsPartialAnalysis { get; set; } | ||
|
||
public DeclarationAnalysisData() | ||
{ | ||
this.DeclarationsInNode = new List<DeclarationInfo>(); | ||
this.DescendantNodesToAnalyze = new List<SyntaxNode>(); | ||
} | ||
|
||
public void Free() | ||
{ | ||
DeclaringReferenceSyntax = null; | ||
TopmostNodeForAnalysis = null; | ||
DeclarationsInNode.Clear(); | ||
DescendantNodesToAnalyze.Clear(); | ||
IsPartialAnalysis = false; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Immutable; | ||
|
||
namespace Microsoft.CodeAnalysis.Diagnostics | ||
{ | ||
internal abstract partial class AnalyzerDriver : IDisposable | ||
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. 💡 Avoid repeating the interface list on |
||
{ | ||
internal sealed class DeclarationAnalysisData | ||
{ | ||
public static DeclarationAnalysisData CreateFrom(DeclarationAnalysisDataBuilder builder) => new DeclarationAnalysisData(builder); | ||
private DeclarationAnalysisData(DeclarationAnalysisDataBuilder builder) | ||
{ | ||
DeclaringReferenceSyntax = builder.DeclaringReferenceSyntax; | ||
TopmostNodeForAnalysis = builder.TopmostNodeForAnalysis; | ||
DeclarationsInNode = builder.DeclarationsInNode.ToImmutable(); | ||
DescendantNodesToAnalyze = builder.DescendantNodesToAnalyze.ToImmutable(); | ||
IsPartialAnalysis = builder.IsPartialAnalysis; | ||
} | ||
|
||
/// <summary> | ||
/// GetSyntax() for the given SyntaxReference. | ||
/// </summary> | ||
public SyntaxNode DeclaringReferenceSyntax { get; } | ||
|
||
/// <summary> | ||
/// Topmost declaration node for analysis. | ||
/// </summary> | ||
public SyntaxNode TopmostNodeForAnalysis { get; } | ||
|
||
/// <summary> | ||
/// All member declarations within the declaration. | ||
/// </summary> | ||
public ImmutableArray<DeclarationInfo> DeclarationsInNode { get; } | ||
|
||
/// <summary> | ||
/// All descendant nodes for syntax node actions. | ||
/// </summary> | ||
public ImmutableArray<SyntaxNode> DescendantNodesToAnalyze { get; } | ||
|
||
/// <summary> | ||
/// Flag indicating if this is a partial analysis. | ||
/// </summary> | ||
public bool IsPartialAnalysis { get; } | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Collections.Immutable; | ||
|
||
namespace Microsoft.CodeAnalysis.Diagnostics | ||
{ | ||
internal abstract partial class AnalyzerDriver : IDisposable | ||
{ | ||
internal sealed class DeclarationAnalysisDataBuilder | ||
{ | ||
public DeclarationAnalysisDataBuilder() | ||
{ | ||
this.DeclarationsInNode = ImmutableArray.CreateBuilder<DeclarationInfo>(); | ||
this.DescendantNodesToAnalyze = ImmutableArray.CreateBuilder<SyntaxNode>(); | ||
} | ||
|
||
/// <summary> | ||
/// GetSyntax() for the given SyntaxReference. | ||
/// </summary> | ||
public SyntaxNode DeclaringReferenceSyntax { get; set; } | ||
|
||
/// <summary> | ||
/// Topmost declaration node for analysis. | ||
/// </summary> | ||
public SyntaxNode TopmostNodeForAnalysis { get; set; } | ||
|
||
/// <summary> | ||
/// All member declarations within the declaration. | ||
/// </summary> | ||
public ImmutableArray<DeclarationInfo>.Builder DeclarationsInNode { get; } | ||
|
||
/// <summary> | ||
/// All descendant nodes for syntax node actions. | ||
/// </summary> | ||
public ImmutableArray<SyntaxNode>.Builder DescendantNodesToAnalyze { get; } | ||
|
||
/// <summary> | ||
/// Flag indicating if this is a partial analysis. | ||
/// </summary> | ||
public bool IsPartialAnalysis { get; set; } | ||
|
||
public void Free() | ||
{ | ||
DeclaringReferenceSyntax = null; | ||
TopmostNodeForAnalysis = null; | ||
DeclarationsInNode.Clear(); | ||
DescendantNodesToAnalyze.Clear(); | ||
IsPartialAnalysis = false; | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,7 +684,7 @@ private ImmutableHashSet<ISymbol> ComputeGeneratedCodeSymbolsInTree(SyntaxTree t | |
var model = compilation.GetSemanticModel(tree); | ||
var root = tree.GetRoot(cancellationToken); | ||
var span = root.FullSpan; | ||
var builder = new List<DeclarationInfo>(); | ||
var builder = ImmutableArray.CreateBuilder<DeclarationInfo>(); | ||
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. why not ArrayBuilder? |
||
model.ComputeDeclarationsInSpan(span, getSymbol: true, builder: builder, cancellationToken: cancellationToken); | ||
|
||
ImmutableHashSet<ISymbol>.Builder generatedSymbolsBuilderOpt = null; | ||
|
@@ -1391,8 +1391,6 @@ internal class AnalyzerDriver<TLanguageKindEnum> : AnalyzerDriver where TLanguag | |
private ImmutableDictionary<DiagnosticAnalyzer, ImmutableArray<OperationBlockAnalyzerAction>> _lazyOperationBlockActionsByAnalyzer; | ||
private ImmutableDictionary<DiagnosticAnalyzer, ImmutableArray<OperationBlockAnalyzerAction>> _lazyOperationBlockEndActionsByAnalyzer; | ||
|
||
private static readonly ObjectPool<DeclarationAnalysisData> s_declarationAnalysisDataPool = new ObjectPool<DeclarationAnalysisData>(() => new DeclarationAnalysisData()); | ||
|
||
/// <summary> | ||
/// Create an analyzer driver. | ||
/// </summary> | ||
|
@@ -1670,17 +1668,17 @@ private void ClearCachedAnalysisDataIfAnalyzed(SyntaxReference declaration, ISym | |
compilationData.ClearDeclarationAnalysisData(declaration); | ||
} | ||
|
||
private DeclarationAnalysisData ComputeDeclarationAnalysisData( | ||
private DeclarationAnalysisDataBuilder ComputeDeclarationAnalysisData( | ||
ISymbol symbol, | ||
SyntaxReference declaration, | ||
SemanticModel semanticModel, | ||
AnalysisScope analysisScope, | ||
Func<DeclarationAnalysisData> allocateData, | ||
Func<DeclarationAnalysisDataBuilder> allocateData, | ||
CancellationToken cancellationToken) | ||
{ | ||
cancellationToken.ThrowIfCancellationRequested(); | ||
|
||
var declarationAnalysisData = allocateData(); | ||
DeclarationAnalysisDataBuilder declarationAnalysisData = allocateData(); | ||
var builder = declarationAnalysisData.DeclarationsInNode; | ||
|
||
var declaringReferenceSyntax = declaration.GetSyntax(cancellationToken); | ||
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.
Please spell out the type. #Closed |
||
|
@@ -1698,7 +1696,7 @@ private DeclarationAnalysisData ComputeDeclarationAnalysisData( | |
return declarationAnalysisData; | ||
} | ||
|
||
private static void ComputeDeclarationsInNode(SemanticModel semanticModel, ISymbol declaredSymbol, SyntaxNode declaringReferenceSyntax, SyntaxNode topmostNodeForAnalysis, List<DeclarationInfo> builder, CancellationToken cancellationToken) | ||
private static void ComputeDeclarationsInNode(SemanticModel semanticModel, ISymbol declaredSymbol, SyntaxNode declaringReferenceSyntax, SyntaxNode topmostNodeForAnalysis, ImmutableArray<DeclarationInfo>.Builder builder, CancellationToken cancellationToken) | ||
{ | ||
// We only care about the top level symbol declaration and its immediate member declarations. | ||
int? levelsToCompute = 2; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please wrap long lines. On github this was not at all clear that this was a Builder and not an ImmutableArray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use split view. it will show long line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would prefer our code be usable on github in either view :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well. not sure when there is a way to see it on GitHub without too much trouble. that feels like something github should fix not code to change to fit into github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a lot of trouble. because now you have to use split view instead of the unified view.
it also affects anyone who wants to ever look at teh code on github outside of diff-view as it will scroll off the right.
I disagree. We're an open source project that uses github extensively as the medium for everyone to collaborate. We should be cognizant of that and should try to be good citizens here. If the tooling ever changes, we can recalibrate. But we should not expect the rest of the ecosystem to bend to us. We should be considerate and be good to the rest of hte ecosystem. That's just good project stewardship.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the size of the monitor. It's the resolution. And even recent telemetry data (like 6-9 months old) showed the vast majority of devs were 1080p or less**. I'm currently running 1080p, and i cannot see these lines.
--
Other hardware surveys (like steam's) also show the vast majority of users are around this as well. But MS has this info already and knows that above 1080p is actually the large minority here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dotnet/roslyn-ide @dotnet/roslyn-analysis any opinion? if all think same, I can create simple analyzer that will issue warning if any line is longer than 130 characters. so that people can find that one as they use code.
fixer probably not easy since how to wrap is different per case by case. but we can identify all those cases and make the change once for all. and don't think about it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi point taken , @heejaechang can you pls file a separate issue for this and we can priorities it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have a passionate objection to this. I even blocked attempts to add it as a configurable rule to StyleCop Analyzers. I have the Editor Guidelines extension installed with a guideline at 120 characters, but I only use it for wrapping comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharwell not even as a suggestion? I agree warning is too aggressive but having a subtle suggestion at the character after the column limit doesn't seem like a bad idea. what is your objection to that?