Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
nalka0 opened this issue Sep 30, 2024 · 1 comment
Open

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

nalka0 opened this issue Sep 30, 2024 · 1 comment
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be. Type: Performance It takes too long.

Comments

@nalka0
Copy link
Contributor

nalka0 commented Sep 30, 2024

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

  • Microsoft Visual Studio Community 2022 - Version 17.11.4
  • .NET 5 to 9
  • SonarScanner for .NET version 9.0.0
  • Windows 11
@CristianAmbrosini
Copy link
Contributor

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,

@CristianAmbrosini CristianAmbrosini added Area: C# C# rules related issues. Type: Performance It takes too long. Type: False Positive Rule IS triggered when it shouldn't be. labels Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Type: False Positive Rule IS triggered when it shouldn't be. Type: Performance It takes too long.
Projects
None yet
Development

No branches or pull requests

2 participants