From 10c0149ad1dcec200ae38c90a27801d9669bd757 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sun, 20 Aug 2023 11:28:03 +0300 Subject: [PATCH 1/2] [Performance] Simplify UseSpanBasedStringConcat for better performance --- .../Runtime/CSharpUseSpanBasedStringConcat.cs | 17 +----- .../Runtime/UseSpanBasedStringConcat.cs | 58 +++++-------------- .../Runtime/BasicUseSpanBasedStringConcat.vb | 15 +---- 3 files changed, 19 insertions(+), 71 deletions(-) diff --git a/src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpUseSpanBasedStringConcat.cs b/src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpUseSpanBasedStringConcat.cs index 98910ac31d..d4a5b18407 100644 --- a/src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpUseSpanBasedStringConcat.cs +++ b/src/NetAnalyzers/CSharp/Microsoft.NetCore.Analyzers/Runtime/CSharpUseSpanBasedStringConcat.cs @@ -1,6 +1,5 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. -using System.Diagnostics.CodeAnalysis; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; @@ -11,20 +10,10 @@ namespace Microsoft.NetCore.CSharp.Analyzers.Runtime [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class CSharpUseSpanBasedStringConcat : UseSpanBasedStringConcat { - private protected override bool TryGetTopMostConcatOperation(IBinaryOperation binaryOperation, [NotNullWhen(true)] out IBinaryOperation? rootBinaryOperation) + private protected override bool IsTopMostConcatOperation(IBinaryOperation binaryOperation) { - if (!IsConcatOperation(binaryOperation)) - { - rootBinaryOperation = default; - return false; - } - - var current = binaryOperation; - while (current.Parent is IBinaryOperation parentBinaryOperation && IsConcatOperation(parentBinaryOperation)) - current = parentBinaryOperation; - - rootBinaryOperation = current; - return true; + return IsConcatOperation(binaryOperation) && + (binaryOperation.Parent is not IBinaryOperation parentBinary || !IsConcatOperation(parentBinary)); static bool IsConcatOperation(IBinaryOperation operation) { diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs index abcf9f6f16..c41dbf942c 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs @@ -32,11 +32,9 @@ public abstract class UseSpanBasedStringConcat : DiagnosticAnalyzer isDataflowRule: false); /// - /// If the specified binary operation is a string concatenation operation, we try to walk up to the top-most - /// string-concatenation operation that it is part of. If it is not a string-concatenation operation, we simply - /// return false. + /// Returns true if the specified binary operation is a top-most string concatenation operation /// - private protected abstract bool TryGetTopMostConcatOperation(IBinaryOperation binaryOperation, [NotNullWhen(true)] out IBinaryOperation? rootBinaryOperation); + private protected abstract bool IsTopMostConcatOperation(IBinaryOperation binaryOperation); /// /// Remove the built in implicit conversion on operands to concat. @@ -59,49 +57,19 @@ private void OnCompilationStart(CompilationStartAnalysisContext context) if (!RequiredSymbols.TryGetSymbols(context.Compilation, out RequiredSymbols symbols)) return; - context.RegisterOperationBlockStartAction(OnOperationBlockStart); - return; - - // Local functions - void OnOperationBlockStart(OperationBlockStartAnalysisContext context) + context.RegisterOperationAction(context => { - // Maintain set of all top-most concat operations so we don't report sub-expressions of an - // already-reported violation. + // Report diagnostic only if the operation is the top-most concat operation so we don't report sub-expressions of an + // already-reported violation. // We also don't report any diagnostic if the concat operation has too many operands for the span-based // Concat overloads to handle. - var topMostConcatOperations = TemporarySet.Empty; - - context.RegisterOperationAction(PopulateTopMostConcatOperations, OperationKind.Binary); - context.RegisterOperationBlockEndAction(ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls); - - void PopulateTopMostConcatOperations(OperationAnalysisContext context) - { - // If the current operation is a string-concatenation operation, walk up to the top-most concat - // operation and add it to the set. - var binary = (IBinaryOperation)context.Operation; - if (!TryGetTopMostConcatOperation(binary, out var topMostConcatOperation)) - return; - - topMostConcatOperations.Add(topMostConcatOperation, context.CancellationToken); - } - - void ReportDiagnosticsOnRootConcatOperationsWithSubstringCalls(OperationBlockAnalysisContext context) - { - // We report diagnostics for all top-most concat operations that contain - // direct or conditional substring invocations when there is an applicable span-based overload of - // the string.Concat method. - // We don't report when the concatenation contains anything other than strings or character literals. - foreach (var operation in topMostConcatOperations.NonConcurrentEnumerable) - { - if (ShouldBeReported(operation)) - { - context.ReportDiagnostic(operation.CreateDiagnostic(Rule)); - } - } + var binary = (IBinaryOperation)context.Operation; + if (IsTopMostConcatOperation(binary) && ShouldBeReported(binary)) + context.ReportDiagnostic(binary.CreateDiagnostic(Rule)); + }, OperationKind.Binary); + return; - topMostConcatOperations.Free(context.CancellationToken); - } - } + // Local functions bool ShouldBeReported(IBinaryOperation topMostConcatOperation) { @@ -133,8 +101,8 @@ bool ShouldBeReported(IBinaryOperation topMostConcatOperation) bool IsAnyDirectOrConditionalSubstringInvocation(IOperation operation) { - if (operation is IConditionalAccessOperation conditionallAccessOperation) - operation = conditionallAccessOperation.WhenNotNull; + if (operation is IConditionalAccessOperation conditionalAccessOperation) + operation = conditionalAccessOperation.WhenNotNull; return operation is IInvocationOperation invocation && symbols.IsAnySubstringMethod(invocation.TargetMethod); } diff --git a/src/NetAnalyzers/VisualBasic/Microsoft.NetCore.Analyzers/Runtime/BasicUseSpanBasedStringConcat.vb b/src/NetAnalyzers/VisualBasic/Microsoft.NetCore.Analyzers/Runtime/BasicUseSpanBasedStringConcat.vb index b281587e3d..21cdc49e61 100644 --- a/src/NetAnalyzers/VisualBasic/Microsoft.NetCore.Analyzers/Runtime/BasicUseSpanBasedStringConcat.vb +++ b/src/NetAnalyzers/VisualBasic/Microsoft.NetCore.Analyzers/Runtime/BasicUseSpanBasedStringConcat.vb @@ -10,22 +10,13 @@ Namespace Microsoft.NetCore.VisualBasic.Analyzers.Runtime Public NotInheritable Class BasicUseSpanBasedStringConcat : Inherits UseSpanBasedStringConcat - Private Protected Overrides Function TryGetTopMostConcatOperation(binaryOperation As IBinaryOperation, ByRef rootBinaryOperation As IBinaryOperation) As Boolean - + Private Protected Overrides Function IsTopMostConcatOperation(binaryOperation As IBinaryOperation) As Boolean If Not IsStringConcatOperation(binaryOperation) Then - rootBinaryOperation = Nothing Return False End If - Dim parentBinaryOperation = binaryOperation - Dim current As IBinaryOperation - Do - current = parentBinaryOperation - parentBinaryOperation = TryCast(WalkUpImplicitConversionToObject(current.Parent), IBinaryOperation) - Loop While parentBinaryOperation IsNot Nothing AndAlso IsStringConcatOperation(parentBinaryOperation) - - rootBinaryOperation = current - Return True + Dim parentBinaryOperation = TryCast(WalkUpImplicitConversionToObject(binaryOperation.Parent), IBinaryOperation) + Return parentBinaryOperation Is Nothing OrElse Not IsStringConcatOperation(parentBinaryOperation) End Function Private Protected Overrides Function WalkDownBuiltInImplicitConversionOnConcatOperand(operand As IOperation) As IOperation From c572b7263b9ac170480eec4dbc8e64611316e9a3 Mon Sep 17 00:00:00 2001 From: Youssef Victor Date: Sun, 20 Aug 2023 11:39:26 +0300 Subject: [PATCH 2/2] Remove unused using --- .../Runtime/UseSpanBasedStringConcat.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs index c41dbf942c..7954d1720f 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseSpanBasedStringConcat.cs @@ -6,7 +6,6 @@ using System.Linq; using Analyzer.Utilities; using Analyzer.Utilities.Extensions; -using Analyzer.Utilities.PooledObjects; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations;