Caching the length/size in a variable for collections in order to avo…

[LuisOsv]

Description

Caching the length/size in a variable for collections in order to avoid repeated calls within loops, which can slightly improve performance and readability.

Motivation and Context

While reading High Performance with Java (link), I came across a useful performance tip: it's recommended to cache the size of a collection before entering a loop, instead of calling .size() in the loop condition.

Instead of:

for (int i = 0; i < list.size(); i++) {
    // logic
}

Prefer:

int size = list.size();
for (int i = 0; i < size; i++) {
    // logic
}

How Has This Been Tested?

• gradlew build was executed and 99% passed (only testInterfaces() failed)
• gradlew createDist was executed and play with compiled .jar file locally

Types of changes

• Refactor foor loops

[ra0077]

Hi,

Thanks to the PR
Do you have bench results (with JMH) to check the performance gain?

Thanks

[LuisOsv]

Thanks for the feedback. Since the caching was applied in multiple places, benchmarking every change would be tricky. I can run a benchmark on a representative use case — could you suggest a core functionality or flow you'd like me to focus on?

[vlsi]

Could it better be changed to for-each loop instead of factoring managersCount variable?

[LuisOsv]

Hi @vlsi agreed, but the new version will still use index since there is insertion on the array given the index. Are you ok with this change?

Example:

int indexManager = 0;
        for (KeyManager manager : managers) {
            if (manager instanceof X509KeyManager) {
                newManagers[indexManager] = new WrappedX509KeyManager((X509KeyManager) manager, keys);
            } else {
                newManagers[indexManager] = manager;
            }
            indexManager++;
        }

[vlsi]

WDYT of for-each loop?

[LuisOsv]

Same here, are you agree with new version with index?

int indexTrustManager = 0;
        for (TrustManager trustManager : trustmanagers) {
            if (trustManager instanceof X509TrustManager) {
                trustmanagers[indexTrustManager] = new CustomX509TrustManager(
                        (X509TrustManager) trustManager);
            }
            indexTrustManager++;
        }

[vlsi]

There are three types of changes here:

1. There are valid improvements, and in theory they could make the code a tiny bit faster.

   int columnCount = tableModel.getColumnCount();
   for (int i = 0; i < columnCount; i++) {

2. There are changes when the loop iteration goes backward. I believe the changes are not justified, and they do not affect performance.

   for (int i = rowsSelected.length - 1; i >= 0; i--) {

3. There are cases when the loop could be refactored to for-each. I would prefer for-each loops rather than manual for loops.

I suggest we should cleanup 2&3 before merging the PR.
However, what bothers me the most is I can't understand how could one "test" the PR. In other words, is there any automation that could detect "suboptimal loops" and refactor them to the optimal case?
The thing is the new code is slightly harder to read, and I could easily imagine someone could file a PR that refactors the code to the old style. It would be great if there was an automated refactoring to implement the change.