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

[java] minor performance improvements and code cleanup #14054

Merged
merged 21 commits into from
Jun 20, 2024

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented May 30, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

changes have been made to improve performance in various parts of the code.

Description

These changes help optimize the code and make it more efficient.
I also replaced some deprecated method invocations

  • Replaced empty string checks with isEmpty().
  • Used System.arraycopy() instead of manual array copying.
  • Replaced String.format calls with printf.
  • Replaced some deprecated method invocations.

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests


Description

  • Replaced empty string checks with isEmpty() in multiple classes for better readability and performance.
  • Used System.arraycopy() instead of manual array copying in PrintOptions and EventFiringDecorator.
  • Replaced String.format calls with printf in CompletionCommand.
  • Replaced deprecated setScriptTimeout with scriptTimeout in various test files.
  • Replaced Arrays.asList with List.of for immutable list creation in test files.
  • Simplified map initialization in ChromiumOptions by using constructor initialization.
  • Optimized list addition in RemoteLogs by using addAll.

Changes walkthrough 📝

Relevant files
Enhancement
10 files
Cookie.java
Optimize empty string checks in `Cookie` class.                   

java/src/org/openqa/selenium/Cookie.java

  • Replaced empty string checks with isEmpty().
+2/-2     
ChromiumOptions.java
Simplify map initialization in `ChromiumOptions`.               

java/src/org/openqa/selenium/chromium/ChromiumOptions.java

  • Replaced manual map population with constructor initialization.
+1/-2     
FileExtension.java
Optimize empty string checks in `FileExtension` class.     

java/src/org/openqa/selenium/firefox/FileExtension.java

  • Replaced empty string checks with isEmpty().
+1/-1     
CompletionCommand.java
Use `printf` instead of `String.format` in `CompletionCommand`.

java/src/org/openqa/selenium/grid/commands/CompletionCommand.java

  • Replaced String.format calls with printf.
+10/-12 
ConcatenatingConfig.java
Optimize empty string checks in `ConcatenatingConfig` class.

java/src/org/openqa/selenium/grid/config/ConcatenatingConfig.java

  • Replaced empty string checks with isEmpty().
+1/-1     
LogLevelMapping.java
Optimize empty string checks in `LogLevelMapping` class. 

java/src/org/openqa/selenium/logging/LogLevelMapping.java

  • Replaced empty string checks with isEmpty().
+1/-1     
PrintOptions.java
Use `System.arraycopy` in `PrintOptions`.                               

java/src/org/openqa/selenium/print/PrintOptions.java

  • Replaced manual array copy with System.arraycopy.
+2/-3     
RemoteLogs.java
Simplify list addition in `RemoteLogs`.                                   

java/src/org/openqa/selenium/remote/RemoteLogs.java

  • Replaced manual list addition with addAll.
+1/-3     
EventFiringDecorator.java
Use `System.arraycopy` in `EventFiringDecorator`.               

java/src/org/openqa/selenium/support/events/EventFiringDecorator.java

  • Replaced manual array copy with System.arraycopy.
+3/-6     
BazelBuild.java
Optimize empty string checks in `BazelBuild` class.           

java/test/org/openqa/selenium/build/BazelBuild.java

  • Replaced empty string checks with isEmpty().
+1/-1     
Tests
5 files
ExecutingAsyncJavascriptTest.java
Update deprecated methods and optimize list creation in tests.

java/test/org/openqa/selenium/ExecutingAsyncJavascriptTest.java

  • Replaced deprecated setScriptTimeout with scriptTimeout.
  • Replaced Arrays.asList with List.of.
  • +12/-12 
    ExecutingJavascriptTest.java
    Optimize list creation in `ExecutingJavascriptTest`.         

    java/test/org/openqa/selenium/ExecutingJavascriptTest.java

    • Replaced Arrays.asList with List.of.
    +1/-1     
    SessionQueueGridWithTimeoutTest.java
    Optimize list creation in `SessionQueueGridWithTimeoutTest`.

    java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java

    • Replaced Arrays.asList with List.of.
    +1/-1     
    InternetExplorerDriverTest.java
    Optimize empty string checks in `InternetExplorerDriverTest`.

    java/test/org/openqa/selenium/ie/InternetExplorerDriverTest.java

    • Replaced empty string checks with isEmpty().
    +1/-1     
    RemoteWebDriverUnitTest.java
    Update deprecated methods in `RemoteWebDriverUnitTest`.   

    java/test/org/openqa/selenium/remote/RemoteWebDriverUnitTest.java

    • Replaced deprecated setScriptTimeout with scriptTimeout.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward, mostly involving syntax improvements and method updates across several files. The PR is well-organized and the changes are consistent, making it relatively easy to review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The change in PrintOptions.java from a manual array copy to System.arraycopy might introduce an off-by-one error. The original code seems to have an incorrect loop condition (i < ranges.length should be i < ranges.length + 1), and the new code might not correctly handle all entries if not adjusted properly.

    🔒 Security concerns

    No

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Readability
    Simplify the condition to improve clarity

    The condition ranges.length - 1 >= 0 is always true if ranges.length is non-negative.
    Consider simplifying the condition to ranges.length > 0 for clarity.

    java/src/org/openqa/selenium/print/PrintOptions.java [70]

    -if (ranges.length - 1 >= 0)
    +if (ranges.length > 0)
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies a simplification in the condition check. Changing ranges.length - 1 >= 0 to ranges.length > 0 improves readability and understanding of the condition.

    7

    Copy link

    codecov bot commented May 30, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 58.72%. Comparing base (57f8398) to head (1181d73).
    Report is 528 commits behind head on trunk.

    Current head 1181d73 differs from pull request most recent head 0dd4c1e

    Please upload reports for the commit 0dd4c1e to get more accurate results.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #14054      +/-   ##
    ==========================================
    + Coverage   58.48%   58.72%   +0.23%     
    ==========================================
      Files          86       86              
      Lines        5270     5298      +28     
      Branches      220      227       +7     
    ==========================================
    + Hits         3082     3111      +29     
    + Misses       1968     1960       -8     
    - Partials      220      227       +7     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @titusfortner
    Copy link
    Member

    Looks like the event firing decorator tests are failing. Can you investigate?

    @iampopovich
    Copy link
    Contributor Author

    Based on the trace of the failed tests, I suspect that the requireNonNull check is breaking the test.
    System.arraycopy(Objects.requireNonNull(args), 0, args2, 1, argsLength);
    The original code did not include a check that args could be null. I will try removing the null check, and if the test continues to fail, I will revert the changes back to the original code.

    @iampopovich
    Copy link
    Contributor Author

    In the code segment that led to the test failure, there is a tricky mechanism for handling the situation where args == null

    int argsLength = args != null ? args.length : 0;
     Object[] args2 = new Object[argsLength + 1];
     args2[0] = target.getOriginal();
     for (int i = 0; i < argsLength; i++) {
       args2[i + 1] = args[i];
     }
    

    if args is null, the args2 array is created with a size sufficient only to store target.getOriginal(). Since argsLength will be zero, the for loop will not be executed, and there will be no errors in accessing the elements of args. I replaced it witharraycopy()and explicit check if(args != null)
    if(args != null) System.arraycopy(args, 0, args2, 1, argsLength);

    @titusfortner titusfortner merged commit 6704db0 into SeleniumHQ:trunk Jun 20, 2024
    31 of 37 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    )
    
    * replaced empty string comparison with isEmpty() invoking
    
    * replaced manual array copy with System.arraycopy()
    
    * replaced redundant String.format invoking with printf()
    
    * replaced deprecated setScriptTimeout with scriptTimeout  
    according to instruction "Use scriptTimeout(Duration)"
    
    * replaced iterators with bulk methods invoking
    
    * replaced list creations with List.of()
    
    * there is no need to create mutable lists in tests to only get elements
    
    ---------
    
    Co-authored-by: Puja Jagani <puja.jagani93@gmail.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants