JDK-8274686 : java.util.UUID#hashCode() should use Long.hashCode()

[JarvisCraft]

This is trivial fix of JDK-8274686 which replaces manually-computed int-based long hash-code.

Because Long#hashCode(long) uses other hashing function than the one currently used here:

jdk/src/java.base/share/classes/java/lang/Long.java

Lines 1440 to 1442 in 75d6688

	public static int hashCode(long value) {
	return (int)(value ^ (value >>> 32));
	}

the value of hashCode() will produce different result, however this does not seem to be a breaking change as UUID#hashCode() is not specified.

---

Note: the comment to the bug states:

Moved to JDK for further discussions of the proposed enhancement.

But as there seemed to be no corresponding discussion in core-libs-dev and the change looks trivial, I considered that it is appropriate to open a PR which (if needed) may be used for discussion (especially, considering the fact that its comments get mirrored to the ML).

---

Progress

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

Issue

• JDK-8274686: java.util.UUID#hashCode() should use Long.hashCode()

Reviewers

• @stsypanov (no known github.com user name / role)
• Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5811/head:pull/5811
$ git checkout pull/5811

Update a local copy of the PR:
$ git checkout pull/5811
$ git pull https://git.openjdk.java.net/jdk pull/5811/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5811

View PR using the GUI difftool:
$ git pr show -t 5811

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5811.diff

[bridgekeeper]

👋 Welcome back JarvisCraft! 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.

[openjdk]

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

• core-libs

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.

[mlbridge]

Webrevs

• 00: Full (47df5966)

[stsypanov]

Good catch, this is what I've missed in #4309

[openjdk]

⚠️ @JarvisCraft the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8274686-uuid-use-long-hashCode
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

[openjdk]

@JarvisCraft 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:

8274686: java.util.UUID#hashCode() should use Long.hashCode()

Reviewed-by: rriggs

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 24 new commits pushed to the master branch:

• 7ad74d8: 8274299: Make Method/Constructor/Field accessors @stable
• 1459180: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module
• 8609ea5: 8273342: Null pointer dereference in classFileParser.cpp:2817
• a5080ef: 8272564: Incorrect attribution of method invocations of Object methods on interfaces
• a914ee7: 8274632: Possible pointer overflow in PretouchTask chunk claiming
• 8f7a37c: 8274434: move os::get_default_process_handle and os::dll_lookup to os_posix for POSIX platforms
• 3953e07: 8271459: C2: Missing NegativeArraySizeException when creating StringBuilder with negative capacity
• 53d7e95: 8274635: Use String.equals instead of String.compareTo in jdk.accessibility
• e43f540: 8274651: Possible race in FontDesignMetrics.KeyReference.dispose
• 2e542e3: 8274349: ForkJoinPool.commonPool() does not work with 1 CPU
• ... and 14 more: https://git.openjdk.java.net/jdk/compare/9ca6bf0d22ac39d4ee29c756903038f09087ca12...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@RogerRiggs) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[JarvisCraft]

/integrate

[openjdk]

@JarvisCraft
Your change (at version 47df596) is now ready to be sponsored by a Committer.

[plevart]

You do know that this might break things? If there are multiple versions of JDK present in some distributed system where participants do not agree on the hash code of an UUID value, it can behave erratically. For example using UUID as a key in a distributed cache like Infinispan is known to be troublesome if the hashCode of some key is not the same across the cluster. Usually there will not be a problem since all nodes in a cluster would use the same JDK version, but what about a rolling upgrade then? It would not be possible. I think at least this change needs to be documented in release notes.

[AlanBateman]

@plevart may have a point and since this patch doesn't really have any benefits then maybe this PR/issue can be closed.

[plevart]

Sorry, I was mislead by the comment above that because the original hashCode function is different, the hashCode will be different too. But I think this is not the case here. Original function is this:

    long hilo = mostSigBits ^ leastSigBits;
    return ((int)(hilo >> 32)) ^ (int) hilo;

new function (with inlined Long.hashCode(long)) looks like this:

    long hilo = mostSigBits ^ leastSigBits;
    return (int)(hilo ^ hilo >>> 32);

The difference is two-fold:

1. original function uses arithmetic shift right (division by 2^32 preserving the sign) while new function uses logical shift right (shifting in zeros from the left to the right)
2. original function casts individual arguments to int before doing XOR between them while new function XORs long arguments and applies cast to int at the end.

But those two differences do not affect the lower 32 bits of any of the intermediate results and therefore the end result is the same (unless I missed something). So I think no release notes is needed for this change.

I think that #4309 also dealt with similar change which was then reverted as a consequence (I'll have to check to be sure).

[plevart]

Yes, the reverted change to BitSet.hashCode in #4309 has the same property. It could be applied without any visible effect.

[stsypanov]

@plevart should I then file a follow-up ticket for BitSet? And should we change the JavaDoc providing there we have computation formula explicitly specified?

[bridgekeeper]

@JarvisCraft This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[JarvisCraft]

Hi there, just a friendly reminder on this PR (as @Bridgekeeper has attempted to mark it as stale).

Is there anything what should be done in order to have this PR integrated or is it just waiting for its time?

[RogerRiggs]

/sponsor

Thanks for the ping.

[openjdk]

Going to push as commit cc2cac1.
Since your change was applied there have been 461 commits pushed to the master branch:

• 0c2d00b: 8275097: Wrong span of the 'default' tag
• fa754b8: 8276149: jshell throws EOF error when throwing exception inside switch expression
• 4c14edd: 8274734: the method jdk.jshell.SourceCodeAnalysis documentation not working
• ff6863c: 8276540: Howl Full CardSet container iteration marks too many cards
• 5448139: 8271056: C2: "assert(no_dead_loop) failed: dead loop detected" due to cmoving identity
• 0395e4e: 8276768: Snippet copy feature should use button instead of link
• d8b0dee: 8276239: Better tables in java.util.random package summary
• fc0fe25: 8273235: tools/launcher/HelpFlagsTest.java Fails on Windows 32bit
• 3934fe5: 8275847: Scheduling fails with "too many D-U pinch points" on small method
• 44047f8: 8274328: C2: Redundant CFG edges fixup in block ordering
• ... and 451 more: https://git.openjdk.java.net/jdk/compare/9ca6bf0d22ac39d4ee29c756903038f09087ca12...master

Your commit was automatically rebased without conflicts.

[openjdk]

@RogerRiggs @JarvisCraft Pushed as commit cc2cac1.

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