Update remaining performance rules (the ones with the tag) to include benchmarks

[marco-carvalho]

Hello SonarQube Team.

I'm writing to request clarification about rule S3247 which has been tagged as "Performance". While I've been using SonarQube for static code analysis, I've come across this rule but I couldn't find explicit information about how this particular rule contributes to the improvement of code performance.

According to my understanding and the description of the rule itself, it's primary purpose is to enforce proper use of the 'is' keyword for type checking in C#. However, the correlation between this practice and performance enhancement isn't directly clear in the provided documentation.

Could you please provide more details about how following this rule might help improve performance? More specifically, what is the performance implication when the 'is' keyword is not used appropriately, and how would proper usage aid in enhancing performance?

It would be really helpful to have these details for me and others who are using your tool for code quality analysis to better understand and justify the enforcement of this rule in our code.

I appreciate your time and effort in addressing this issue.

[martin-strecker-sonarsource]

Hello @marco-carvalho,

thank you for asking. The rule description states that is Fruit and the following cast (Fruit)x causes two operations to be performed. There are other alternatives that achieve the same with a single operation. So there is nothing wrong with using is. It is the combination of is and the following cast that is problematic. I think the documentation is quite clear about this.

The potential savings here are marginal. I would recommend using if (x is Fruit fruit) not for being more performant but for being much more concise and clearer.

If you are interested in the details, you can have a look here sharplab.io.

For the first method M1, you can find an isinst [System.Runtime]System.Exception intermediate language-opcode followed by a castclass [System.Runtime]System.Exception while in M2, there is only one isinst and castclass is missing.

Do you still think the documentation should be updated? If so, how can we improve it?

[marco-carvalho]

Hi @martin-strecker-sonarsource, thanks for your answer.

I've observed that there are rules, such as this one, which feature a Benchmark section. This section includes benchmark code, benchmark results, and details about the hardware configuration used for benchmarking.

I've found these details to be immensely valuable, providing direct, quantitative insights into the performance implications of the rule in question. This got me thinking – I believe that all rules tagged with "performance" could greatly benefit from a similar structure.

By incorporating benchmarks directly into the rule description, developers will not only understand the nature of the rule but also have a tangible sense of its impact on performance. This can lead to a more informed decision-making process when it comes to code optimisation.

Could the sonar-dotnet team please consider adding a benchmark section to other performance-tagged rules? This would greatly enhance our understanding of the rules and their practical implications.

[costin-zaharia-sonarsource]

Hi @marco-carvalho, thanks a lot for your feedback. I'm very happy that you find the benchmark details and the results we measured useful. The rules that have the benchmark data are part of a recent effort (all of them are new) and we were curious to see how they are received by the users.

I've added a discussion topic for our team to take a decision regarding the old performance rules update.