SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
790 stars 227 forks source link

Fix S3260 FP: Performance seems no longer relevant for .Net 8 and 9 #9673

Closed nalka0 closed 3 weeks ago

nalka0 commented 1 month ago

Description

After re-running your benchmark for S3260 on .Net 8 and 9, it looks like this rule has become irrelevant :

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22631.4169/23H2/2023Update/SunValley3)
12th Gen Intel Core i7-1265U, 1 CPU, 12 logical and 10 physical cores
.NET SDK 9.0.100-rc.1.24452.12
  [Host]   : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2
  .NET 5.0 : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2
  .NET 6.0 : .NET 6.0.33 (6.0.3324.36610), X64 RyuJIT AVX2
  .NET 7.0 : .NET 7.0.20 (7.0.2024.26716), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2
  .NET 9.0 : .NET 9.0.0 (9.0.24.43107), X64 RyuJIT AVX2
Method Job Runtime Iterations Mean Error StdDev Ratio RatioSD
UnsealedType .NET 5.0 .NET 5.0 1000000 909.4 μs 6.29 μs 5.88 μs 1.00 0.01
SealedType .NET 5.0 .NET 5.0 1000000 235.9 μs 2.67 μs 3.28 μs 0.26 0.00
UnsealedType .NET 6.0 .NET 6.0 1000000 1,116.9 μs 10.36 μs 9.69 μs 1.00 0.01
SealedType .NET 6.0 .NET 6.0 1000000 226.5 μs 2.78 μs 2.47 μs 0.20 0.00
UnsealedType .NET 7.0 .NET 7.0 1000000 1,114.5 μs 13.93 μs 12.35 μs 1.00 0.02
SealedType .NET 7.0 .NET 7.0 1000000 232.9 μs 4.08 μs 3.82 μs 0.21 0.00
UnsealedType .NET 8.0 .NET 8.0 1000000 233.6 μs 4.57 μs 4.28 μs 1.00 0.02
SealedType .NET 8.0 .NET 8.0 1000000 234.5 μs 3.29 μs 2.92 μs 1.00 0.02
UnsealedType .NET 9.0 .NET 9.0 1000000 232.9 μs 3.17 μs 2.96 μs 1.00 0.02
SealedType .NET 9.0 .NET 9.0 1000000 232.9 μs 3.52 μs 3.29 μs 1.00 0.02

Repro steps

For reference, here's the benchmark's code (same benchmark than the one in the rule's documentation with extra [SimpleJob] for .Net 8 and 9)

[SimpleJob(RuntimeMoniker.Net50)]
[SimpleJob(RuntimeMoniker.Net60)]
[SimpleJob(RuntimeMoniker.Net70)]
[SimpleJob(RuntimeMoniker.Net80)]
[SimpleJob(RuntimeMoniker.Net90)]
[RPlotExporter]
public class Benchmarker
{
    [Params(1_000_000)]
    public int Iterations { get; set; }

    private readonly UnsealedClass unsealedType = new UnsealedClass();

    private readonly SealedClass sealedType = new SealedClass();

    [Benchmark(Baseline = true)]
    public void UnsealedType()
    {
        for (int i = 0; i < Iterations; i++)
        {
            unsealedType.DoNothing();
        }
    }

    [Benchmark]
    public void SealedType()
    {
        for (int i = 0; i < Iterations; i++)
        {
            sealedType.DoNothing();
        }
    }

    private class BaseType
    {
        public virtual void DoNothing() { }
    }

    private class UnsealedClass : BaseType
    {
        public override void DoNothing() { }
    }

    private sealed class SealedClass : BaseType
    {
        public override void DoNothing() { }
    }
}

Expected behavior

The rule is disabled for .Net 8 and 9

Actual behavior

This rule is currently always active

Related information

CristianAmbrosini commented 1 month ago

Hi @nalka0! Thanks a lot for providing the detailed benchmark. We already have an investigation sprint in our backlog to re-run all the benchmarks and compare them, particularly with the .NET 9 implementations. Based on the outcomes, we will decide whether to refactor the rules or deprecate them. We'll keep you updated on our progress once that sprint will start.

Thanks,

gregory-paidis-sonarsource commented 3 weeks ago

Hey there! Thanks for running the benchmarks. I also re-run them, and it seems like I found slightly different results than you. The performance seems to still be better, just not as much as before. This is because of Profile Guided Optimization (PGO), which allows the runtime to optimize more code in .NET 9.

The idea of the rule is sound, and sealed classes will always be at least as fast, and in complex cases faster than non-sealed ones. So we will opt to keep the rule as-is for now.

nalka0 commented 3 weeks ago

@gregory-paidis-sonarsource How come you found different results than me? What should I do next time to prevent having different results? Or is it just bound to the hardware I run the benchmark on?

gregory-paidis-sonarsource commented 3 weeks ago

I think it's a combination of hardware and running a more complex callback to public override void DoNothing() { }, as it is probably being optimized away

nalka0 commented 3 weeks ago

If it's about running a more complex callback to public override void DoNothing() { }, shouldn't the benchmark in the rule's documentation be updated accordingly? https://github.com/SonarSource/sonar-dotnet/blob/2ecf742558692066437bc5b7701d2c62c35d92cc/analyzers/rspec/cs/S3260.html#L129

gregory-paidis-sonarsource commented 1 week ago

Hey @nalka0 , sorry for the delay. We are discussing internally on how to proceed with this. The rule is a good idea in general, and we will update the RSPEC accordingly.