Skip to content

Commit aac8366

Browse files
github-actions[bot]JamesNKmartincostello
authored
[release/8.0] Fix route analyzer performance with highly concatenated strings (#54763)
* Add test for route analyzer performance issue * Clean up * Clean up * Fix build * Save last string node as an optimization to exit out of deeply nested string concatenations * Update src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternAnalyzerTests.cs Co-authored-by: Martin Costello <[email protected]> * Improve perf, remove class file * Fix stress test * Improve analysis performance * Test mixing lang at levels * Simplify * Old code * Comment * Comment * Match new Roslyn behavior * Clean up --------- Co-authored-by: James Newton-King <[email protected]> Co-authored-by: Martin Costello <[email protected]>
1 parent 22bb1c3 commit aac8366

File tree

4 files changed

+152
-1
lines changed

4 files changed

+152
-1
lines changed

src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/RouteStringSyntaxDetector.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,19 @@ private static bool HasLanguageComment(
149149
return true;
150150
}
151151

152+
// Check for the common case of a string literal in a large binary expression. For example `"..." + "..." +
153+
// "..."` We never want to consider these as regex/json tokens as processing them would require knowing the
154+
// contents of every string literal, and having our lexers/parsers somehow stitch them all together. This is
155+
// beyond what those systems support (and would only work for constant strings anyways). This prevents both
156+
// incorrect results *and* avoids heavy perf hits walking up large binary expressions (often while a caller is
157+
// themselves walking down such a large expression).
158+
if (token.Parent.IsLiteralExpression() &&
159+
token.Parent.Parent.IsBinaryExpression() &&
160+
token.Parent.Parent.RawKind == (int)SyntaxKind.AddExpression)
161+
{
162+
return false;
163+
}
164+
152165
for (var node = token.Parent; node != null; node = node.Parent)
153166
{
154167
if (HasLanguageComment(node.GetLeadingTrivia(), out identifier, out options))

src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/SyntaxNodeExtensions.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,12 @@ public static SyntaxNode GetRequiredParent(this SyntaxNode node)
3333
return parent;
3434
}
3535

36+
public static bool IsLiteralExpression([NotNullWhen(true)] this SyntaxNode? node)
37+
=> node is LiteralExpressionSyntax;
38+
39+
public static bool IsBinaryExpression([NotNullWhen(true)] this SyntaxNode? node)
40+
=> node is BinaryExpressionSyntax;
41+
3642
[return: NotNullIfNotNull("node")]
3743
public static SyntaxNode? WalkUpParentheses(this SyntaxNode? node)
3844
{

src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
<Project Sdk="Microsoft.NET.Sdk">
1+
<Project Sdk="Microsoft.NET.Sdk">
22

33
<PropertyGroup>
44
<TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>

src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternAnalyzerTests.cs

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,28 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Diagnostics;
45
using System.Globalization;
6+
using System.Text;
57
using Microsoft.AspNetCore.Analyzer.Testing;
68
using Microsoft.AspNetCore.Analyzers.RenderTreeBuilder;
79
using Microsoft.AspNetCore.Analyzers.RouteEmbeddedLanguage.Infrastructure;
10+
using Microsoft.CodeAnalysis;
11+
using Xunit.Abstractions;
812

913
namespace Microsoft.AspNetCore.Analyzers.RouteEmbeddedLanguage;
1014

1115
public partial class RoutePatternAnalyzerTests
1216
{
17+
private readonly ITestOutputHelper _testOutputHelper;
18+
1319
private TestDiagnosticAnalyzerRunner Runner { get; } = new(new RoutePatternAnalyzer());
1420

21+
public RoutePatternAnalyzerTests(ITestOutputHelper testOutputHelper)
22+
{
23+
_testOutputHelper = testOutputHelper;
24+
}
25+
1526
[Fact]
1627
public async Task CommentOnString_ReportResults()
1728
{
@@ -512,4 +523,125 @@ public object TestAction(int id)
512523
// Assert
513524
Assert.Empty(diagnostics);
514525
}
526+
527+
[Fact]
528+
public async Task ConcatString_PerformanceTest()
529+
{
530+
// Arrange
531+
var builder = new StringBuilder();
532+
builder.AppendLine("""
533+
class Program
534+
{
535+
static void Main() { }
536+
static readonly string _s =
537+
""");
538+
for (var i = 0; i < 2000; i++)
539+
{
540+
builder.AppendLine(" \"a{}bc\" +");
541+
}
542+
builder.AppendLine("""
543+
"";
544+
}
545+
""");
546+
var source = TestSource.Read(builder.ToString());
547+
548+
// Act 1
549+
// Warm up.
550+
var diagnostics1 = await Runner.GetDiagnosticsAsync(source.Source);
551+
552+
// Assert 1
553+
Assert.Empty(diagnostics1);
554+
555+
// Act 2
556+
// Measure analysis.
557+
var stopwatch = Stopwatch.StartNew();
558+
559+
var diagnostics2 = await Runner.GetDiagnosticsAsync(source.Source);
560+
_testOutputHelper.WriteLine($"Elapsed time: {stopwatch.Elapsed}");
561+
562+
// Assert 2
563+
Assert.Empty(diagnostics2);
564+
}
565+
566+
[Fact]
567+
public async Task ConcatString_DetectLanguage_NoWarningsBecauseConcatString()
568+
{
569+
// Arrange
570+
var builder = new StringBuilder();
571+
builder.AppendLine("""
572+
class Program
573+
{
574+
static void Main() { }
575+
// lang=Route
576+
static readonly string _s =
577+
""");
578+
for (var i = 0; i < 2000; i++)
579+
{
580+
builder.AppendLine(" \"a{}bc\" +");
581+
}
582+
builder.AppendLine("""
583+
"";
584+
}
585+
""");
586+
var source = TestSource.Read(builder.ToString());
587+
588+
// Act
589+
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);
590+
591+
// Assert
592+
Assert.Empty(diagnostics);
593+
}
594+
595+
[Fact]
596+
public async Task NestedLangComment_NoWarningsBecauseConcatString()
597+
{
598+
// Arrange
599+
var builder = new StringBuilder();
600+
builder.AppendLine("""
601+
class Program
602+
{
603+
static void Main() { }
604+
static readonly string _s =
605+
"{/*MM0*/te*st0}" +
606+
// lang=Route
607+
"{/*MM1*/te*st1}" +
608+
"{/*MM2*/te*st2}" +
609+
"{test3}";
610+
}
611+
""");
612+
var source = TestSource.Read(builder.ToString());
613+
614+
// Act
615+
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);
616+
617+
// Assert
618+
Assert.Empty(diagnostics);
619+
}
620+
621+
[Fact]
622+
public async Task TopLangComment_NoWarningsBecauseConcatString()
623+
{
624+
// Arrange
625+
var builder = new StringBuilder();
626+
builder.AppendLine("""
627+
class Program
628+
{
629+
static void Main() { }
630+
static readonly string _s =
631+
// lang=Route
632+
"{/*MM0*/te*st0}" +
633+
"{/*MM1*/te*st1}" +
634+
"{/*MM2*/te*st2}" +
635+
// lang=regex
636+
"{test3}";
637+
}
638+
""");
639+
var source = TestSource.Read(builder.ToString());
640+
641+
// Act
642+
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);
643+
644+
// Assert
645+
Assert.Empty(diagnostics);
646+
}
515647
}

0 commit comments

Comments
 (0)