Skip to content

Commit

Permalink
[Performance] Simplify UseSpanBasedStringConcat for better performance (
Browse files Browse the repository at this point in the history
#6870)

* [Performance] Simplify UseSpanBasedStringConcat for better performance

* Remove unused using
  • Loading branch information
Youssef1313 authored Sep 12, 2023
1 parent 8011355 commit fc117a2
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 72 deletions.
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -32,11 +31,9 @@ public abstract class UseSpanBasedStringConcat : DiagnosticAnalyzer
isDataflowRule: false);

/// <summary>
/// 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
/// </summary>
private protected abstract bool TryGetTopMostConcatOperation(IBinaryOperation binaryOperation, [NotNullWhen(true)] out IBinaryOperation? rootBinaryOperation);
private protected abstract bool IsTopMostConcatOperation(IBinaryOperation binaryOperation);

/// <summary>
/// Remove the built in implicit conversion on operands to concat.
Expand All @@ -59,49 +56,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<IBinaryOperation>.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)
{
Expand Down Expand Up @@ -133,8 +100,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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,13 @@ Namespace Microsoft.NetCore.VisualBasic.Analyzers.Runtime
<DiagnosticAnalyzer(LanguageNames.VisualBasic)>
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
Expand Down

0 comments on commit fc117a2

Please sign in to comment.