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

8263436: Silly array comparison in GaloisCounterMode.overlapDetection #2938

Closed
wants to merge 1 commit into from

Conversation

@shipilev
Copy link
Contributor

@shipilev shipilev commented Mar 11, 2021

SonarCloud reports:
Use "Arrays.equals(array1, array2)" or the "==" operator instead of using the "Object.equals(Object obj)" method.

        } else if (!src.isDirect() && !dst.isDirect()) {
            if (!src.isReadOnly()) {
                // If using the heap, check underlying byte[] address.
                if (!src.array().equals(dst.array()) ) { // <--- here

Additional testing:

  • Linux x86_64 fastdebug jdk_security

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8263436: Silly array comparison in GaloisCounterMode.overlapDetection

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2938/head:pull/2938
$ git checkout pull/2938

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 11, 2021

👋 Welcome back shade! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

@shipilev The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the security label Mar 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Webrevs

Loading

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Mar 11, 2021

Can you explain why the silly way it is written now is any different than what you are proposing? When running jshell your proposed change returns the same result as the existing code:

one ==> byte[5] { 1, 2, 3, 4, 5 }
two ==> byte[5] { 1, 2, 3, 4, 5 }
jshell> one != two
$9 ==> true

jshell> !one.equals(two)
$10 ==> true

jshell> one != one
$11 ==> false

jshell> !one.equals(one)
$12 ==> false

Is the analysis tool thinking equals() is comparing the contents?

Loading

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Mar 11, 2021

Can you explain why the silly way it is written now is any different than what you are proposing? Is the analysis tool thinking equals() is comparing the contents?

See: https://rules.sonarsource.com/java/RSPEC-2159.

Using equals raises the question if actual array contents comparisons was implied (which would require Arrays.equals). I know it is not implied here. Using == makes the intent clear. This is why the rule asks to choose either == or Arrays.equals. This is the only instance where that rule triggers in whole OpenJDK code.

From a micro-optimization perspective, equals is a method. This means interpreters are would have to do that call. Compilers would hopefully inline the call, but then it would still imply the null-check, because NPE should be thrown if one side is null.

So, why call equals(), when == is better?

Loading

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Mar 11, 2021

meh

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

@ascarpino Only the author (@shipilev) is allowed to issue the reviewer command.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 11, 2021

@shipilev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8263436: Silly array comparison in GaloisCounterMode.overlapDetection

Reviewed-by: ascarpino

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 47 new commits pushed to the master branch:

  • 7ed46bd: 8241716: Jpackage functionality to let users choose whether to create shortcuts
  • 3820ab9: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages
  • 273f8bd: 8263248: IGV: accept graphs without node categories
  • a9b4f03: 8263069: Exclude some failing tests from security/infra/java/security/cert/CertPathValidator
  • 470b150: 8143041: Unify G1CollectorPolicy::PauseKind and G1YCType
  • f6b4ba0: 8261931: IGV: quick search fails on multi-line node labels
  • 7988c1d: 8262443: GenerateOopMap::do_interpretation can spin for a long time.
  • 32cbd19: 8263105: security-libs doclint cleanup
  • 6971c23: 8262351: Extra '0' in java.util.Formatter for '%012a' conversion with a sign character
  • c6d74bd: 8262910: Cleanup THREAD/TRAPS/naming and typing issues in ObjectMonitor and related code
  • ... and 37 more: https://git.openjdk.java.net/jdk/compare/17853ee92cfba43d25f3ac984d0d3666bbe9c086...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Mar 11, 2021
@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Mar 11, 2021

Here is a demo, if anyone interested:

@State(Scope.Thread)
public class ArrayEquals {
    byte[] b1 = new byte[1];
    byte[] b2 = new byte[1];

    @Benchmark
    public boolean acmp() { return b1 == b2; }

    @Benchmark
    public boolean eq() { return b1.equals(b2); }
}
Benchmark                        Mode  Cnt   Score    Error   Units

# C2 (Default)
ArrayEquals.acmp                 avgt   5     0.678 ± 0.001   ns/op
ArrayEquals.acmp:cycles          avgt         2.576           #/op
ArrayEquals.acmp:instructions    avgt        10.199           #/op

ArrayEquals.eq                   avgt   5     0.680 ± 0.007   ns/op
ArrayEquals.eq:cycles            avgt         2.540           #/op
ArrayEquals.eq:instructions      avgt        13.032           #/op ; <--- null check

# C1
ArrayEquals.acmp                 avgt   5     1.086 ± 0.006   ns/op
ArrayEquals.acmp:cycles          avgt         4.110           #/op
ArrayEquals.acmp:instructions    avgt        16.325           #/op

ArrayEquals.eq                   avgt   5     4.606 ± 0.001   ns/op ; <--- equals() inline failed
ArrayEquals.eq:cycles            avgt        17.328           #/op
ArrayEquals.eq:instructions      avgt        42.520           #/op

# -Xint
ArrayEquals.acmp                 avgt   5   207.550 ± 1.126   ns/op
ArrayEquals.acmp:cycles          avgt       763.037           #/op
ArrayEquals.acmp:instructions    avgt       570.161           #/op

ArrayEquals.eq                   avgt   5   300.449 ± 26.939  ns/op ; <--- interpreter call cost
ArrayEquals.eq:cycles            avgt      1068.852           #/op
ArrayEquals.eq:instructions      avgt       722.228           #/op

Loading

@ascarpino
Copy link
Contributor

@ascarpino ascarpino commented Mar 11, 2021

The performance data is more compelling rationale to me.

Loading

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Mar 12, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@shipilev Since your change was applied there have been 52 commits pushed to the master branch:

  • ad1f605: 8263353: assert(CompilerOracle::option_matches_type(option, value)) failed: Value must match option type
  • cf1c021: 8263480: ProblemList two jpackage tests on Windows
  • f3bd801: 8263403: [JVMCI] output written to tty via HotSpotJVMCIRuntime can be garbled
  • b92abac: 8263433: Shenandoah: Don't expect forwarded objects in set_concurrent_mark_in_progress()
  • 15dacca: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
  • 7ed46bd: 8241716: Jpackage functionality to let users choose whether to create shortcuts
  • 3820ab9: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages
  • 273f8bd: 8263248: IGV: accept graphs without node categories
  • a9b4f03: 8263069: Exclude some failing tests from security/infra/java/security/cert/CertPathValidator
  • 470b150: 8143041: Unify G1CollectorPolicy::PauseKind and G1YCType
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/17853ee92cfba43d25f3ac984d0d3666bbe9c086...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9f6b1d7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@shipilev shipilev deleted the JDK-8263436-gcm-silly branch Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants