perf: skip teardown analysis when no disposable members need cleanup - #6780
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe analyzer now exits ChangesDisposable member analysis
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The optimization preserves analyzer behavior while avoiding unnecessary teardown scans for classes without disposable members. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checked the fields at dawn Comment |
|
Benchmark reproduction and validation evidence This compares the actual
The workload has 10,000 methods across 100 classes. Each operation creates a fresh analyzer driver over the same prebuilt compilation, runs only Reproduction:
Regression validation: 47 tests passed, zero failures/skips, with: dotnet build src/TUnit.Core/TUnit.Core.csproj -c Release -f netstandard2.0
dotnet build src/TUnit.Assertions/TUnit.Assertions.csproj -c Release -f netstandard2.0
dotnet build tests/TUnit.TestProject.Library/TUnit.TestProject.Library.csproj -c Release -f netstandard2.0
dotnet run --project tests/TUnit.Analyzers.Tests -c Release -f net10.0 -- --treenode-filter '/*/*/DisposableFieldPropertyAnalyzerTests/*'The reference-assembly builds are required by the analyzer test project. The analyzer itself builds successfully; the existing Full BenchmarkDotNet report:
Project file: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net10.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="BenchmarkDotNet" Version="0.15.8" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.14.0" />
<Reference Include="TUnit.Core"><HintPath>TUnit.Core.dll</HintPath></Reference>
<None Update="before/*.dll;after/*.dll" CopyToOutputDirectory="PreserveNewest" />
</ItemGroup>
</Project>
Program.cs: using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Diagnostics;
using System.Collections.Immutable;
using System.Runtime.Loader;
using System.Text;
BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
[MemoryDiagnoser]
public class DisposableAnalyzerBenchmarks
{
[Params("Bare", "BodyHeavy", "UndisposedFixture")]
public string Scenario { get; set; } = "Bare";
private CSharpCompilation _compilation = null!;
private ImmutableArray<DiagnosticAnalyzer> _before;
private ImmutableArray<DiagnosticAnalyzer> _after;
private readonly CompilationWithAnalyzersOptions _options = new(
new AnalyzerOptions(ImmutableArray<AdditionalText>.Empty), null,
concurrentAnalysis: false, logAnalyzerExecutionTime: false, reportSuppressedDiagnostics: false);
[GlobalSetup]
public async Task Setup()
{
var trees = new List<SyntaxTree>();
for (var c = 0; c < 100; c++)
{
var source = new StringBuilder("using System; using TUnit.Core; public class Tests" + c + " {");
if (Scenario == "UndisposedFixture")
source.Append("private System.IO.MemoryStream stream = new System.IO.MemoryStream();");
for (var m = 0; m < 100; m++)
{
source.Append("[Test] public void Test").Append(m).Append("() { int x = 42; if (x * x + 1 != 1765) throw new Exception();");
if (Scenario != "Bare")
for (var i = 0; i < 20; i++) source.Append("x = Math.Abs(x) + 1;");
source.Append('}');
}
source.Append('}');
trees.Add(CSharpSyntaxTree.ParseText(source.ToString()));
}
var references = ((string)AppContext.GetData("TRUSTED_PLATFORM_ASSEMBLIES")!).Split(Path.PathSeparator)
.Select(p => MetadataReference.CreateFromFile(p)).ToList();
references.Add(MetadataReference.CreateFromFile(typeof(TUnit.Core.TestAttribute).Assembly.Location));
_compilation = CSharpCompilation.Create("SyntheticSuite", trees, references,
new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary));
var errors = _compilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error).ToArray();
if (errors.Length != 0) throw new InvalidOperationException(string.Join("\n", errors.Select(e => e.ToString())));
_before = Load("before");
_after = Load("after");
var before = await Before();
var after = await After();
var expectedCount = Scenario == "UndisposedFixture" ? 100 : 0;
if (before.Length != expectedCount || after.Length != expectedCount ||
before.Concat(after).Any(d => d.Id != "TUnit0023") ||
!before.Select(d => d.ToString()).Order().SequenceEqual(after.Select(d => d.ToString()).Order()))
throw new InvalidOperationException("Unexpected diagnostics: " + string.Join("\n", before.Concat(after)));
}
private static ImmutableArray<DiagnosticAnalyzer> Load(string name)
{
var path = Path.Combine(AppContext.BaseDirectory, name, "TUnit.Analyzers.dll");
var assembly = new AssemblyLoadContext(name).LoadFromAssemblyPath(path);
Console.WriteLine($"{name} analyzer MVID: {assembly.ManifestModule.ModuleVersionId}");
return ImmutableArray.Create((DiagnosticAnalyzer)Activator.CreateInstance(
assembly.GetType("TUnit.Analyzers.DisposableFieldPropertyAnalyzer")!)!);
}
[Benchmark(Baseline = true)]
public Task<ImmutableArray<Diagnostic>> Before() => _compilation.WithAnalyzers(_before, _options).GetAnalyzerDiagnosticsAsync();
[Benchmark]
public Task<ImmutableArray<Diagnostic>> After() => _compilation.WithAnalyzers(_after, _options).GetAnalyzerDiagnosticsAsync();
} |
Greptile SummaryThis PR improves analyzer performance by skipping teardown invocation analysis when field, property, constructor, and setup analysis found no disposable members.
Confidence Score: 5/5The PR appears safe to merge because the skipped teardown and reporting paths cannot affect diagnostics when the tracked-member collection is empty. Teardown processing can only remove existing tracked members, while diagnostics are emitted only for members left in that collection; the new empty-collection return therefore preserves behavior.
|
| Filename | Overview |
|---|---|
| src/TUnit.Analyzers/DisposableFieldPropertyAnalyzer.cs | Adds a behavior-preserving early return that avoids unnecessary teardown analysis when there are no tracked disposable members. |
Reviews (1): Last reviewed commit: "perf: skip disposal teardown scans witho..." | Re-trigger Greptile
Review: perf: skip teardown analysis when no disposable members need cleanup (#6780)Verdict: Looks good. What the change doesAdds an early return in CorrectnessI traced the data flow to confirm this is safe:
Scope and validation
SuggestionsNone — this is a clean, well-justified micro-optimization with a clear invariant (teardown analysis is subtractive-only) backing its safety. No architectural or design concerns; nothing to flag on maintainability or scalability grounds for a change this small and self-contained. |
1 participant
DisposableFieldPropertyAnalyzercurrently scans every method for teardown calls even when field, property, constructor, and setup analysis found no disposable members. This can resolve hundreds of thousands of invocation operations that cannot affect any diagnostic.Return after setup analysis when the tracked-member collection is empty. Classes with disposable members continue through the existing teardown and reporting paths; instance and static analysis remain separate.
Measured with BenchmarkDotNet 0.15.8 over 10,000 tests in 100 classes, using the actual baseline (
b43fecac6d) and PR analyzer assemblies:The larger-body workload allocated approximately 31% less. Timing is indicative: the unchanged fixture path also moved substantially between processes, and an earlier incomplete run did not show a small-body speedup. The full report below includes confidence intervals. No whole-build or test-execution speedup is claimed.
The benchmark includes Roslyn driver costs, excludes parsing and initial compilation validation, and runs one complete suite per iteration with analyzer concurrency disabled. Environment: Windows 11, i7-12700K, .NET 10.0.12, SDK 11.0.100-preview.7.26381.103; 10 measured iterations, 5 warmups, 1 launch.
Validation: all 47
DisposableFieldPropertyAnalyzerTestspass. Benchmark setup verifies identical diagnostics, including the same 100TUnit0023warnings for the fixture control. Analyzer builds succeed; the existingRS2007release-header warning appears in both revisions. Reproduction source and full results are posted in the PR comment.Summary by CodeRabbit