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++;
        }

[LuisOsv]

hi @vlsi I think it's best to keep classic for since we need index access inside loops. What do you think?

[vlsi]

Yeah, I missed the index is needed there.
However, managers variable does not mutate, so JVM should be able to inline managers.length here.

Frankly, I find managers.length much easier to read than the variant with an extra variable, and I bet extracting managers.length to a variable does not improve performance. So I would suggest keeping the old code as is.

[LuisOsv]

Old code was reverted @vlsi

[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++;
        }

[LuisOsv]

old code reverted @vlsi

[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.

[vlsi]

Frankly, I think the performance gains are negligible here, and the change makes the code slightly harder to follow

[LuisOsv]

hi @vlsi I think the change improves readability and maintainability. and It does not affect functionality. Are you still agree with reverting changes?

[vlsi]

I suggest we revert the change as well as it hardly yields any performance gains, yet it makes the code harder to follow

[vlsi]

Please revert as discussed previously.

[vlsi]

copiedNodes.length was computed one-time only, there's no point in adding a variable

[vlsi]

Please revert