Caching the size/length in loops to slightly improve performance #15852

LuisOsv · 2025-06-04T02:27:18Z

User description

🔗 Related Issues

N/A — performance and readability enhancement

💥 What does this PR do?

This PR improves performance and readability by caching the size or length of collections and arrays before entering loops.
Instead of repeatedly calling .size() or .length in the loop condition, the value is stored in a local variable and reused.

🔧 Implementation Notes

This change is based on a common Java performance recommendation highlighted in High Performance with Java by Javier Fernandez Gonzalez (O’Reilly, 2024).
The book explains that repeatedly calling methods like .size() inside loop conditions is less efficient than caching their values in local variables. This approach improves both runtime performance and code readability.
Example refactor:

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

Instead of:

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

💡 Additional Considerations

🔒 No behavioral or logic changes were made.
✅ 100% backwards compatible
🔍 The refactor strictly targets loop conditions for micro-optimization and readability.
🧼 Improves code maintainability by reducing repetition.

🔄 Types of changes

✅ Cleanup (formatting, renaming)
✅ Performance improvement (non-breaking change)

🧪 Tests Executed
To ensure nothing was broken by this cleanup, the following affected test targets were executed:

bazel test //java/test/org/openqa/selenium/grid/node/docker:small-tests
bazel test //java/test/org/openqa/selenium/grid/node/relay:medium-tests
bazel test //java/test/org/openqa/selenium/json:SmallTests
bazel test //java/test/org/openqa/selenium/remote:small-tests
bazel test //java/test/org/openqa/selenium/remote/codec/w3c:SmallTests
bazel test //java/test/org/openqa/selenium/remote/http:small-tests

✅ All tests passed successfully.

ℹ️ Note on Loop Style
In some cases within this PR, the classic for loop with an index is intentionally retained rather than switching to a for-each or Java Stream approach. This is because the existing logic requires indexed access or manual index manipulation.
For such scenarios, the traditional indexed for loop is both more straightforward and readable than using other alternatives.

PR Type

Enhancement

Description

Cache collection/array size in loops for performance
Refactor loop conditions for improved readability
No behavioral or logic changes introduced
Consistent micro-optimization across multiple Java files

Changes walkthrough 📝

Relevant files

I-performance

9 files

JavadocJarMaker.java `Cache args length before loop for efficiency`	+2/-1
ModuleGenerator.java `Cache args length before loop for efficiency`	+2/-1
DockerOptions.java `Cache config list size before loop for efficiency`	+3/-2
RelayOptions.java `Cache config list size before loop for efficiency`	+3/-2
JsonInput.java `Cache string length before loop for efficiency`	+2/-1
WebElementToJsonConverter.java `Cache array length before loop for efficiency`	+2/-1
AbstractHttpCommandCodec.java `Cache list size before loops for efficiency`	+5/-3
Route.java `Cache prefix path array length before loops`	+3/-2
UrlTemplate.java `Cache group list size before switch and loop`	+3/-2

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

…readability

qodo-merge-pro · 2025-06-04T02:27:59Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌ 1234 - Not compliant Non-compliant requirements: • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click() • Ensure JavaScript alerts are triggered properly when clicking links with JavaScript in href • Make behavior consistent with previous version (2.47.1) • Fix specifically for Firefox 42.0 5678 - Not compliant Non-compliant requirements: • Fix "ConnectFailure (Connection refused)" error when instantiating ChromeDriver • Address issue where second and subsequent ChromeDriver instantiations fail • Fix specifically for Chrome 65.0.3325.181 with ChromeDriver 2.35 on Ubuntu 16.04.4
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Potential Bug The condition checking if we've reached the end of the configs array now uses the cached size variable (configsCount) instead of the original collection size. This could lead to different behavior if the collection size changes during iteration. if (i == configsCount) { throw new DockerException("Unable to find JSON config"); Potential Bug Similar to the DockerOptions class, the condition checking if we've reached the end of the configs array now uses the cached size variable (configsCount) instead of the original collection size, which could lead to different behavior if the collection size changes during iteration. if (i == configsCount) { throw new ConfigException("Unable to find stereotype config. " + allConfigs);

qodo-merge-pro · 2025-06-04T02:28:39Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix double increment issue The loop manually increments `i` inside the loop body, but also increments it in the loop statement. This could cause skipping elements. Consider using a while loop or incrementing only once. java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java [144-149] -for (int i = 0; i < configsCount; i++) { +for (int i = 0; i < configsCount; i += 2) { String imageName = allConfigs.get(i); - i++; - if (i == configsCount) { + if (i + 1 >= configsCount) { throw new DockerException("Unable to find JSON config"); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This identifies a serious logical bug where the loop both increments `i` in the for statement and manually inside the loop body, causing elements to be skipped. This could lead to incorrect configuration parsing.	High
	Fix double increment bug The loop manually increments `i` inside the loop body while also incrementing it in the loop statement. This leads to skipping elements. Use a while loop or increment only once. java/src/org/openqa/selenium/grid/node/relay/RelayOptions.java [157-165] -for (int i = 0; i < configsCount; i++) { +for (int i = 0; i < configsCount; i += 2) { int maxSessions; try { maxSessions = Integer.parseInt(extractConfiguredValue(allConfigs.get(i))); } catch (NumberFormatException e) { throw new ConfigException("Unable parse value as number. " + allConfigs.get(i)); } - i++; - if (i == configsCount) { + if (i + 1 >= configsCount) { throw new ConfigException("Unable to find stereotype config. " + allConfigs); } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 9 __ Why: Similar to suggestion 2, this fixes a critical logical bug where double incrementing causes elements to be skipped during configuration parsing, potentially leading to incorrect behavior.	High
	Prevent array index exception Incrementing `i` directly in the array access could lead to an `ArrayIndexOutOfBoundsException` if `i` is the last element. Consider checking if there's a next argument available before incrementing. java/src/dev/selenium/tools/javadoc/JavadocJarMaker.java [61] +if (i + 1 >= argCount) { + throw new IllegalArgumentException("Missing value for flag: " + flag); +} String next = args[++i]; Apply / Chat Suggestion importance[1-10]: 8 __ Why: This addresses a critical potential `ArrayIndexOutOfBoundsException` that could occur when `i` is the last element in the `args` array. The bounds check prevents runtime crashes.	Medium
Update

diemol

Thanks for your PR.

What's the performance gain?

LuisOsv · 2025-06-05T04:22:28Z

Hi @diemol, thanks for the feedback!

The performance improvement is minimal but measurable. I ran a quick JMH benchmark in a separate project and saw a slight gain from caching the size before iteration:

Benchmark	Mode	Cnt	Score	Error	Units
CachedSizeBenchmark.iterateWithForLoopCached	avgt	10	270.411	± 41.816	ns/op
CachedSizeBenchmark.iterateWithForLoopNotCached	avgt	10	281.851	± 61.816	ns/op

You can find the benchmark code here: https://github.com/LuisOsv/benchTests

Note: I haven't run this within the Selenium codebase directly, as I'm not sure how to integrate JMH into the current Bazel-based setup.

diemol

Thank you, @LuisOsv!

caching the size/length in loops to slightly improve performance and …

437adfb

…readability

selenium-ci added B-grid Everything grid and server related C-java Java Bindings labels Jun 4, 2025

qodo-merge-pro bot added the Review effort 2/5 label Jun 4, 2025

diemol reviewed Jun 4, 2025

View reviewed changes

Merge branch 'trunk' into improving_loops

eb0e09c

diemol approved these changes Jun 5, 2025

View reviewed changes

diemol merged commit f5382c9 into SeleniumHQ:trunk Jun 5, 2025
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Caching the size/length in loops to slightly improve performance #15852

Caching the size/length in loops to slightly improve performance #15852

LuisOsv commented Jun 4, 2025 •

edited by qodo-merge-pro bot

Loading

Uh oh!

qodo-merge-pro bot commented Jun 4, 2025

Uh oh!

qodo-merge-pro bot commented Jun 4, 2025 •

edited

Loading

Uh oh!

diemol left a comment

Uh oh!

LuisOsv commented Jun 5, 2025

Uh oh!

diemol left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caching the size/length in loops to slightly improve performance #15852

Caching the size/length in loops to slightly improve performance #15852

Conversation

LuisOsv commented Jun 4, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-merge-pro bot commented Jun 4, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

LuisOsv commented Jun 5, 2025

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LuisOsv commented Jun 4, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Jun 4, 2025 •

edited

Loading