Skip to content

Filter identifier name simplification according to aliases and predefined types#41079

Merged
sharwell merged 2 commits intodotnet:masterfrom
sharwell:filter-aliases
Jan 22, 2020
Merged

Filter identifier name simplification according to aliases and predefined types#41079
sharwell merged 2 commits intodotnet:masterfrom
sharwell:filter-aliases

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jan 19, 2020 •
edited
Loading

Extracted from #40746 and generalized to work with Visual Basic.

Baseline numbers (49a468c + #41072):

Found 24193 diagnostics in 97737ms (148431953600 bytes allocated)
Execution times (ms):
CSharpSimplifyTypeNamesDiagnosticAnalyzer:      1552142
  SimpleMemberAccessExpression: 917073ms to try simplifying 1383559 instances
  IdentifierName: 321246ms to try simplifying 2951512 instances
  GenericName: 127254ms to try simplifying 92323 instances
  QualifiedName: 60160ms to try simplifying 158084 instances
  QualifiedCref: 238ms to try simplifying 1245 instances
  AliasQualifiedName: 10ms to try simplifying 30 instances
VisualBasicSimplifyTypeNamesDiagnosticAnalyzer: 1528633
  SimpleMemberAccessExpression: 1098896ms to try simplifying 632418 instances
  IdentifierName: 272517ms to try simplifying 1197747 instances
  GenericName: 51408ms to try simplifying 32776 instances
  QualifiedName: 48384ms to try simplifying 62234 instances

Updated numbers (49a468c + this pull request):

Found 24202 diagnostics in 93467ms (141284021216 bytes allocated)
Execution times (ms):
CSharpSimplifyTypeNamesDiagnosticAnalyzer:      1601508
  SimpleMemberAccessExpression: 1237560ms to try simplifying 1383578 instances
  GenericName: 169917ms to try simplifying 92326 instances
  QualifiedName: 73563ms to try simplifying 158087 instances
  IdentifierName: 1724ms to try simplifying 10343 instances
  QualifiedCref: 340ms to try simplifying 1245 instances
  AliasQualifiedName: 10ms to try simplifying 30 instances
VisualBasicSimplifyTypeNamesDiagnosticAnalyzer: 1487597
  SimpleMemberAccessExpression: 1300676ms to try simplifying 632444 instances
  QualifiedName: 57909ms to try simplifying 62236 instances
  GenericName: 57496ms to try simplifying 32779 instances
  IdentifierName: 1739ms to try simplifying 4668 instances

📝 The increase in diagnostics is due to a bug in Simplify Type Names for the Visual Basic code NameOf([Object]). The apparent increase in analyzer execution time is noise caused by contested locks used for measuring the callbacks. The allocation numbers and end-to-end time improvements were consistent.

📝 In combination with #41072, 99.75% of calls to TrySimplify were eliminated as unnecessary for IdentifierNameSyntax.

@sharwell sharwell changed the base branch from master to master-vs-deps January 21, 2020 17:26
@sharwell sharwell changed the base branch from master-vs-deps to master January 21, 2020 17:27
@sharwell sharwell marked this pull request as ready for review January 21, 2020 17:27
@sharwell sharwell requested a review from a team as a code owner January 21, 2020 17:27
CyrusNajmabadi
CyrusNajmabadi reviewed Jan 21, 2020
View reviewed changes
src/Features/CSharp/Portable/Diagnostics/Analyzers/TypeSyntaxSimplifierWalker.cs
_cancellationToken = cancellationToken;

var root = semanticModel.SyntaxTree.GetRoot(cancellationToken);
_aliasedNames = GetAliasedNames(root as CompilationUnitSyntax);
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jan 21, 2020

Choose a reason for hiding this comment

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

this should be a normal cast.

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jan 21, 2020

Choose a reason for hiding this comment

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

This doesn't seem problematic, and there are known cases where syntax trees do not have CompilationUnitSyntax as the root.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jan 21, 2020

Choose a reason for hiding this comment

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

and there are known cases where syntax trees do not have CompilationUnitSyntax as the root.

Those would all be bugs in the case of analyzers though. No one should ever register a semantic-model analyzer and not get a CompilationUnit. That would be a big invariance bug.

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jan 22, 2020

Choose a reason for hiding this comment

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

The rewrite of GetAliasedNames specifically targets scenarios where the entry point is not a semantic model analyzer. Semantic model analyzers always start at the root so the normal recursive descent walker can be used for this.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jan 22, 2020

Choose a reason for hiding this comment

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

i'm not certain what you mean. When is the entrypoint not a semantic-model analyzer?

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jan 22, 2020

Choose a reason for hiding this comment

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

In the current code, it's not. But we're considering a hybrid code block + syntax node analyzer if we can't get the performance up through SemanticModel sharing in the compiler or driver.

CyrusNajmabadi
CyrusNajmabadi reviewed Jan 21, 2020
View reviewed changes
src/Features/CSharp/Portable/Diagnostics/Analyzers/TypeSyntaxSimplifierWalker.cs
// The only possible simplifications to an unqualified identifier are replacement with an alias or
// replacement with a predefined type.
canTrySimplify = CanReplaceIdentifierWithAlias(node.Identifier.ValueText!)
|| CanReplaceIdentifierWithPredefinedType(node.Identifier.ValueText!);
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jan 21, 2020

Choose a reason for hiding this comment

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

extract out node.Identifier.ValueText to a local.

Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi Jan 21, 2020

Choose a reason for hiding this comment

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

extract all of this to a local function canTrySimplify()

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

CyrusNajmabadi commented Jan 21, 2020

The apparent increase in analyzer execution time is noise caused by contested locks used for measuring the callbacks.

Yikes. Can we file issue for that as well? Is this just callbacks in AnalyzerRunner? Or in the analyzer infrastructure itself?

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jan 21, 2020

Yikes. Can we file issue for that as well? Is this just callbacks in AnalyzerRunner? Or in the analyzer infrastructure itself?

This issue doesn't exist in the main AnalyzerRunner implementation. It's the test code I added to get the number of TrySimplify attempts were made for each type of syntax.

@sharwell sharwell closed this Jan 22, 2020
@sharwell sharwell reopened this Jan 22, 2020
@sharwell sharwell merged commit f411a32 into dotnet:master Jan 22, 2020
@sharwell sharwell deleted the filter-aliases branch January 22, 2020 20:05
@sharwell sharwell added this to the 16.5.P3 milestone Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.5.P3

Development

Successfully merging this pull request may close these issues.

3 participants

@sharwell @CyrusNajmabadi @mavasani