<i18n dev> RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]

[Jorn Vernee]

On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Please review this PR, which supersedes a now withdrawn https://github.com/openjdk/jdk/pull/14831.
>> 
>> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more user-friendly methods. Here's a summary:
>> 
>> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the `vectorizedHashCode` method private
>> 
>> - Made the `vectorizedHashCode` method private, but didn't rename it. Renaming would dramatically increase this PR review cost, because that method's name is used by a lot of VM code. On a bright side, since the method is now private, it's no longer callable by clients of `ArraysSupport`, thus a problem of an inaccurate name is less severe.
>> 
>> - Made the `ArraysSupport.utf16HashCode` method private
>> 
>> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport`
>
> Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix incorrect utf16 hashCode adaptation

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 252:

> 250:         return switch (length) {
> 251:             case 0 -> initialValue;
> 252:             case 1 -> 31 * initialValue + (int) a[fromIndex];

Suggestion:

            case 1 -> 31 * initialValue + (int) a[fromIndex]; // sign extension

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 275:

> 273:         return switch (length) {
> 274:             case 0 -> initialValue;
> 275:             case 1 -> 31 * initialValue + (a[fromIndex] & 0xff);

For clarity, if you think it helps:
Suggestion:

            case 1 -> 31 * initialValue + Byte.toUnsignedInt(a[fromIndex]);

src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:

> 299:         return switch (length) {
> 300:             case 0 -> initialValue;
> 301:             case 1 -> 31 * initialValue + JLA.getUTF16Char(a, fromIndex);

There seems to be a mismatch here with the original code in StringUTF16, where the length that is tested for is `2` instead of `1`.

test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java line 88:

> 86:     private static int testIntrinsic(byte[] bytes, int type)
> 87:             throws InvocationTargetException, IllegalAccessException {
> 88:         return (int) vectorizedHashCode.invoke(null, bytes, 0, 256, 1, type);

Better to just call `hashCodeOfUnsigned` here I think.

The test for the non-constant type could be dropped. That is no longer a part of the 'API' of `ArraySupport`. It looks like the intrinsic bails out when the basic type is not constant any ways: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L6401-L6404

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778741
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778493
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617777798
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617784996