On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo <pra...@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