feat(EC22): Implementing Avoid Method Usage for Only Basic Operations

[moabidi91]

Description :

Using methods for basic operations consumes additional system resources.
The interpreter must in effect and solve the objects and then the methods, just to carry out these simple operations of the language.

Implementation

1. What's a basic operation in Java?
   A "basic operation" involve the use of arithmetic, comparison, and logical operators.
   
   These operators are used either in Binary expression or Unary expression and also in Conditional exprssion using ternary operator.
   
   So technically, using Java Sonar's API, an expression which is instance of BinaryExpressionTree or UnaryExpressionTree or is Tree.Kind.CONDITIONAL_EXPRESSION => represents a basic operation !

2. What kind of methods should be targeted ?
   Each method having a return statement of a basic operation

3. What this Rule do not cover ?
   The below example of code contains an assignment ecpression then a return statement :

public int add(int a, int b) {
  int c = a+b;
  return c;
}

As there is an unnecessarily assignement , it should be reported by another Rule.

So respecting Single Resp. Principle of SOLID and regarding the scope of EC22 this practice wont be reported despite the presence of a basic operation.

Same case could be detected with mutation.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[wokier]

Hi

I think this rule does not apply to Java, because the JVM does the optimisation at runtime.
Having a meaningful method for a basic operation is a recommendation for a good design for a rich domain.

[moabidi91]

@wokier Thanks for your review.

• Could you please explain more how "JVM does the optimisation at runtime" for this use case ?

• I understand your point of view. Indeed, when it comes to simple operations that are not repeated frequently, it might not be worth creating a separate utility method for them, as it could decrease performance. So we need to find the right balance between duplicated code and appropriate use of utility methods (or readability). And so it's a sonar smell not a bug.

[MP-Aubay]

Issue #131

[wokier]

• Could you please explain more how "JVM does the optimisation at runtime" for this use case ?

I know that the JVM is able to rearrange the program structure to simplify it.

In this case, it is called inlining. I have found this documentation that explains it https://blog.jdriven.com/2019/11/hotspot-jvm-optimizations/

I agree that this rule does apply to other language, especially interpreted ones, where the method will be an extra step.

• I understand your point of view. Indeed, when it comes to simple operations that are not repeated frequently, it might not be worth creating a separate utility method for them, as it could decrease performance. So we need to find the right balance between duplicated code and appropriate use of utility methods (or readability). And so it's a sonar smell not a bug.

My conclusion is that this proposed rule would reduce the readability (in term of domain richness) and provide near to zero optimization.

[github-actions]

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

[dedece35]

Hi @moabidi91,
please resolve conflict and check DoD list https://github.com/green-code-initiative/ecoCode-common/blob/main/doc/starter-pack.md#definition-of-done-of-a-pr

[github-actions]

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

[dedece35]

Hi @moabidi91,
regarding previous discussions, I think we have to check if this rule is applicable for Java with tests based on decompilation to check real code built by compiler.

[dedece35]

EDIT : wrong conclusion with compilation demonstration, please see next comments to complete discussion

I found another recent benchmark (september 2023) by baeldung ( a Java reference on web) : https://www.baeldung.com/jvm-method-inlining
It show us that optimization is done respecting some checks.

My local tests show us that by default there is no optimization.

Here is Java source :

Here is decompiled Class file with IntelliJ with JDK 11 :

Here is decompiled Class file with another decompilation tool (JD-GUI app) :

In conclusion, I don't think optimizations are systematic with JIT compiler and this optimization is maybe usefull.

[wokier]

Hello
Thanks for the researches
The baeldung page explains that the optimizations are done at runtime.
It can depends on the arguments given to the java command
You did'nt shown the same output as in the article.
Compile and decompile proves nothing.

[dedece35]

Hello Thanks for the researches The baeldung page explains that the optimizations are done at runtime. It can depends on the arguments given to the java command You did'nt shown the same output as in the article. Compile and decompile proves nothing.

Hi @wokier,
sorry, my bad, you're rigth, it's at runtime, I read too quickly the article, thus it's wrong too check it just after compilation.
After reading correctly the article, here are some conclusion sentences I think :

Generally, if we find a hot method with a very complex conditional statement, we should try to separate the content of the if-statement and increase the granularity so that the JIT can optimize the code. The same goes for the switch and for-loop statements.
We can conclude that a manual method inlining is something that we don’t need to do in order to optimize our code. The JVM does it more efficiently, and we would possibly make the code long and hard to follow.

We must emphasize the recommendation of not changing the default value of the -XX:FreqInlineSize flag unless absolutely necessary

Thus, I don't think this rule is very relevant to make our code greener and we should let JIT optimze our code (with "inline" system). Maybe, what we can do in our code to help JIT optimizations is to split our algorithms like said in th quot above

@cyChop @jycr @MP-Aubay @jhertout @glalloue, your point of view ?

[cyChop]

Many interesting facts and opinions. This is one of those times when finding the right balance between eco-friendliness and maintainability must be found.

I fully agree with both those statements:

Having a meaningful method for a basic operation is a recommendation for a good design for a rich domain.

So we need to find the right balance between duplicated code and appropriate use of utility methods (or readability). And so it's a sonar smell not a bug.

I'll also say this:

• When I look at developers using Sonar, I see three profiles:

  • Those who don't fix anything.
  • Those who have enough maturity to separate what's applicable from what's not.
  • Those who just do their best to fix everything, even smells and infos. I've been one of those and I've learned much in that time, but we need to be relevant especially for developers who will use this as a learning tool. We must help them be critical. That may be as simple as a rule explanation that invite to caution.

• Ultimately, having a method with a meaningful name stating the intention makes the code clearer. I'm not vouching for a sum method, but I'm sure we can find examples of code with a real functional meaning to the method that is a basic operation.
  For such cases, the method is important and must be preserved. If the intention/functional meaning is not obvious, the code will be less clear, so less maintainable, so it will become less green over time, as it evolves. I'm convinced a good code design is an important part of keeping the code green over time.

This is only food for thought, not a definite answer of course.

About testing which code is best:

Compile and decompile proves nothing.

True, but a benchmark could be useful, if we can measure the CPU, memory and bandwidth used. It could help for all our rules, and if we can do something generic with Docker (for instance), we probably could apply it to all languages. It could also help us arbiter all rules, decide if they're relevant or not, provide figures to explain our rule choices and so on.

I've thought (months ago) of setting up something, probably with Docker and Scaphandre, to compare two snippets of code. ETSDiff might be a nice solution for that, too. Maybe something we could build on GreenFrame. Unfortunately, I've lacked the time until now, and things are not getting better on that front.

This might actually be a third challenge at the ecoCode: we need a way to evaluate our rules, probably measure the energy gain of compliant vs non-compliant code. Teams compete to create a tool, and we can hope to build something streamlined from the participations. Just a way to kickstart it.

One final consideration we might have is: since which version this optimization has been implemented, and do we want to add a rule that applies only to the affected versions of Java? (I've wondered about this for loops and StringBuilders: rule has been deprecated because of optimizations in recent Java, but it still applies to some older JDKs and I know clients who still are hosting Java 7 code.)

[dedece35]

decision inside core-team : we have to measure and find real use cases to prove this rule
for now thie rule EC22 for Java is refused.
once measure is done, we could reopen PR and the issue and continue implementation