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

[EC53][JAVA] Rule to challenge - deprecation #240

Closed
diyfr opened this issue Nov 9, 2023 · 8 comments · Fixed by #260
Closed

[EC53][JAVA] Rule to challenge - deprecation #240

diyfr opened this issue Nov 9, 2023 · 8 comments · Fixed by #260
Assignees
Labels
🏗️ refactoring refactoring for best practices 🗃️ rule rule improvment or rule development or bug java

Comments

@diyfr
Copy link

diyfr commented Nov 9, 2023

Rule definition :
Using List instead of Arrays with Foreach save CPU cycles calculations and RAM consumption.

I have doubts about the relevance of this rule in Java
An array is lower level than a List, there is a good chance that the data traversal will be more efficient
Besides, I invite you to look at the source code of an ArrayList, where the data is stored in an... Array

1- See this sample benchmark test
2- When the object returned by a dependency is in Array format, it is expensive to transform it into a List in order to be able to browse it.

image

@cyChop
Copy link
Contributor

cyChop commented Nov 9, 2023

EDIT: Maybe ignore this comment, I reversed List and Array in my reading of the list, so this comment is not really applicable.

I understand the rule, but I see so many cases where array is not applicable. This feels like "hey, there are high-level objects, but low-level objects are more efficient." Java is high-level; if we must always go low-level, better do C or Rust.

Here's some food for thought (and that's all it is because I can't make up my mind about this rule).

Collections have several high-level specificities over array:

  • Arrays can't grow, you need to know their final size before starting working with them (which is not the case with JS or PHP arrays; those behave more like lists in Java).
  • Arrays can't be used as queues or stacks without reimplementing a lot of logic, while specific collections (variants of lists) exist for this very purpose.
  • Arrays don't guarantee unicity like sets (not lists, but traversal efficiency issues could apply to all collections).
  • Collections are an established part of the Java API, and many libraries or native methods use them. Manually converting them to arrays every time we need to traverse them will probably cost more than traversing them directly.

Replacing lists or collections with arrays will imply rewriting the logic included in the collection objects. I'd rather trust the authors of the JVM to have written those efficiently rather than having developers on every project reinventing the wheel.

I won't go into the design rules that collections also allow, because we know that ecoCode will sometimes raise issues that will lead to having to compromise.

The language provide objects for all those cases, Lists are only one of them. I agree we should use arrays when we can (we know the size, it won't change, etc.), but saying that all lists should be replaced with arrays is going one step further.

To really be applicable, I think this rule should include some more parameters:

  • IF I create the list myself (not the return of a method, because I can't control that);
  • and IF I can set all its element immediately when creating it (not creating a list, passing it to n methods that will append to it);
  • THEN the rule applies.

The most obvious case I can see for this rule is everytime we do a List.of(...), make it an array instead. But then maybe we need to use features of the collections that won't be available on arrays?

It's a really delicate rule, and I'm not sure of how it is applied now, how it should be applied or if it should be applied.

Once again, if it throws warnings every time but can be fixed only 0.1% of the time, it will just be deactivated because it throws too many false positives.

@diyfr
Copy link
Author

diyfr commented Nov 9, 2023

@cyChop
What the rule requires is to transform an Array into a List to perform an iteration.
My intention was not to do the opposite, but only to challenge this rule.
What ecocode highlights is a saving of resources, and I am not sure that this is the case. (see benchmark)

Example

private void myFunction(Class<?> cls) {
    for(Field f: cls.getDeclaredFields()){// no compliant with EC53
           //....
    }
}
private void myFunction(Class<?> cls) {
    var myList = Arrays.asList(cls.getDeclaredFields()); // Duplicate Data in List = increase memory consumption
    for(Field f: myList ){  // Compliant with EC53
           //.....
    }
}

@cyChop
Copy link
Contributor

cyChop commented Nov 9, 2023

Oh sorry, I misread the rule and exchanged Lists and arrays.

@skerdudou
Copy link

The issue haven't followed the repo migration, but in fact, this rule statement IS false.
cnumr/ecoCode#176
Benchmarks clearly proves that iterating on an array is way faster.

@dedece35
Copy link
Member

dedece35 commented Nov 28, 2023

Hi @skerdudou, @diyfr, @cyChop,

I agree with all of you that we can depreciate this rule because of following reasons :

I will create a new PR for to depreciate it and ask for a core team member review.

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

@dedece35 dedece35 added 🗃️ rule rule improvment or rule development or bug java 🏗️ refactoring refactoring for best practices 🔥 in progress 🔥 labels Nov 28, 2023
@jhertout
Copy link
Contributor

Hello @dedece35,

Seeing all the arguments you point, I agree that we should remove this rule.

@utarwyn
Copy link
Member

utarwyn commented Dec 3, 2023

Hello! I have seen this resource with benchmarks: https://www-inf.telecom-sudparis.eu/COURS/cen/EfficaciteLogiciel/catalog.html#looping-over-an-array-or-an-arraylist

They seem to qualify by saying that it depends on the use case, in some cases using a list is more efficient than an array (in joules), but that's not always the case. They are using JoularFX to get these benchmarks. I would also say that this rule needs to be deprecated.

@dedece35
Copy link
Member

dedece35 commented Dec 8, 2023

As confirmed with core-team, this rule will be deprecated soon because of implementation too simple (maybe come back later after proving it with measurements)

@dedece35 dedece35 linked a pull request Dec 11, 2023 that will close this issue
@dedece35 dedece35 self-assigned this Dec 11, 2023
@dedece35 dedece35 changed the title [EC53][JAVA] Rule to challenge [EC53][JAVA] Rule to challenge - deprecation Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏗️ refactoring refactoring for best practices 🗃️ rule rule improvment or rule development or bug java
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants