Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Thu, 30 May 2024 08:34:59 GMT, Claes Redestad wrote: >> @cl4es, do you want me to delete that test file altogether? > > I thought you verified that the non-constant type test still provoke a crash > (on x86) if you back out the code changes in > https://github.com/openjdk/jdk/commit/969f6a37e4649079c7acea1952f5537fd9ba2f0a > ? If so that test is still somewhat useful to guard against future coding > mistakes by verifying that the bail out doesn't mess things up. The constant > type tests have less utility, perhaps. I'd keep it as is unless there's a > strong desire to reduce test runtime (these should be pretty quick). I did verify it. Sorry, momentary lapse of the ability to reason. I'll integrate this PR very shortly then. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1620268479
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 09:18:51 GMT, Pavel Rappo wrote: >> The non-constant test was added because that very bailout caused a crash. >> The other test is actually less interesting since it'll likely be covered >> indirectly by regular use. But as we are hiding these away this gets ever >> more obscure and perhaps the test could be dropped entirely. > > @cl4es, do you want me to delete that test file altogether? I thought you verified that the non-constant type test still provoke a crash (on x86) if you back out the code changes in https://github.com/openjdk/jdk/commit/969f6a37e4649079c7acea1952f5537fd9ba2f0a ? If so that test is still somewhat useful to guard against future coding mistakes by verifying that the bail out doesn't mess things up. The constant type tests have less utility, perhaps. I'd keep it as is unless there's a strong desire to reduce test runtime (these should be pretty quick). - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1620259115
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 15:50:05 GMT, Pavel Rappo wrote: >> @cl4es, here are some results from my machine (macosx-aarch64): >> >> Name (size) Cnt BaseError TestError >> Unit Change >> ArraysHashCode.bytes1 150.715 ± 0.004 0.725 ± 0.029 >> ns/op 0.99x (p = 0.182 ) >> ArraysHashCode.bytes 10 153.753 ± 0.024 3.747 ± 0.011 >> ns/op 1.00x (p = 0.322 ) >> ArraysHashCode.bytes 100 15 69.731 ± 0.15769.737 ± 0.092 >> ns/op 1.00x (p = 0.891 ) >> ArraysHashCode.bytes1 15 9369.386 ± 1.449 9372.008 ± 6.678 >> ns/op 1.00x (p = 0.133 ) >> ArraysHashCode.chars1 150.719 ± 0.024 0.734 ± 0.024 >> ns/op 0.98x (p = 0.076 ) >> ArraysHashCode.chars 10 153.744 ± 0.005 3.746 ± 0.004 >> ns/op 1.00x (p = 0.308 ) >> ArraysHashCode.chars 100 15 69.741 ± 0.11269.714 ± 0.044 >> ns/op 1.00x (p = 0.365 ) >> ArraysHashCode.chars1 15 9367.123 ± 5.320 9371.325 ± 6.407 >> ns/op 1.00x (p = 0.046 ) >> ArraysHashCode.ints 1 150.711 ± 0.013 0.706 ± 0.006 >> ns/op 1.01x (p = 0.137 ) >> ArraysHashCode.ints10 153.750 ± 0.002 3.752 ± 0.004 >> ns/op 1.00x (p = 0.283 ) >> ArraysHashCode.ints 100 15 69.753 ± 0.08669.711 ± 0.016 >> ns/op 1.00x (p = 0.065 ) >> ArraysHashCode.ints 1 15 9376.225 ± 5.845 9376.218 ± 12.181 >> ns/op 1.00x (p = 0.999 ) >> ArraysHashCode.multibytes 1 150.741 ± 0.001 0.740 ± 0.001 >> ns/op 1.00x (p = 0.038 ) >> ArraysHashCode.multibytes 10 152.737 ± 0.001 2.826 ± 0.136 >> ns/op 0.97x (p = 0.017 ) >> ArraysHashCode.multibytes 100 15 32.202 ± 0.05932.153 ± 0.006 >> ns/op 1.00x (p = 0.004*) >> ArraysHashCode.multibytes 1 15 4922.740 ± 25.590 4921.468 ± 7.372 >> ns/op 1.00x (p = 0.846 ) >> ArraysHashCode.multichars 1 150.740 ± 0.005 0.740 ± 0.000 >> ns/op 1.00x (p = 0.996 ) >> ArraysHashCode.multichars 10 152.732 ± 0.002 2.737 ± 0.003 >> ns/op 1.00x (p = 0.000*) >> ArraysHashCode.multichars 100 15 32.109 ± 0.01732.182 ± 0.028 >> ns/op 1.00x (p = 0.000*) >> ArraysHashCode.multichars 1 15 4925.750 ± 46.366 4930.684 ± 26.001 >> ns/op 1.00x (p = 0.704 ) >> ArraysHashCode.multiints1 150.740 ± 0.000 0.739 ± 0.000 >> ns/op 1.00x (p = 0.000*) >> ArraysHashCode.multiints 10 152.919 ± 0.002 2.953 ± 0.059 >> ns/op 0.99x (p = 0.033 ) >> ArraysHashCode.multiints 100 15 32.140 ± ... > > Just to clarify, the above comparison is between master and > https://github.com/openjdk/jdk/pull/19414/commits/534ff367bc50ec4150e4b206ce7203c7ff9f5cad. > For clarity, if you think it helps: I did that and a little bit more in https://github.com/openjdk/jdk/pull/19414/commits/534ff367bc50ec4150e4b206ce7203c7ff9f5cad. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1619130669
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 12:53:42 GMT, Pavel Rappo wrote:
>> 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
>
> To be honest, I don't think that this cast is needed. A better solution than
> to add a comment would be to delete all `(int)` casts from new `hashCode*`
> methods of `ArraysSupport`.
>
> Those `(int)` casts migrated from `hashCode` methods of `Arrays` where there
> were used if neither of two `+` operands were of type `int`. But in
> `ArraysSupport` it's no longer the case: `31 * initialValue` is always `int`
> because `initialValue` is. So, `a[fromIndex]` is promoted to `int` by the
> virtue of
> https://docs.oracle.com/javase/specs/jls/se22/html/jls-5.html#jls-5.6.
>
> For more confidence, consider that the `private static int hashCode` methods
> (implementation) of `ArraysSupport` do not have those casts.
Removed casts in
https://github.com/openjdk/jdk/pull/19414/commits/d8dabc68c4264520fd6c57c949049f2ab5c8e0ec.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1619127224
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 12:44:45 GMT, Pavel Rappo wrote: >> I don't care as long as microbenchmarks don't get a hiccup. > > @cl4es, here are some results from my machine (macosx-aarch64): > > Name (size) Cnt BaseError TestError > Unit Change > ArraysHashCode.bytes1 150.715 ± 0.004 0.725 ± 0.029 > ns/op 0.99x (p = 0.182 ) > ArraysHashCode.bytes 10 153.753 ± 0.024 3.747 ± 0.011 > ns/op 1.00x (p = 0.322 ) > ArraysHashCode.bytes 100 15 69.731 ± 0.15769.737 ± 0.092 > ns/op 1.00x (p = 0.891 ) > ArraysHashCode.bytes1 15 9369.386 ± 1.449 9372.008 ± 6.678 > ns/op 1.00x (p = 0.133 ) > ArraysHashCode.chars1 150.719 ± 0.024 0.734 ± 0.024 > ns/op 0.98x (p = 0.076 ) > ArraysHashCode.chars 10 153.744 ± 0.005 3.746 ± 0.004 > ns/op 1.00x (p = 0.308 ) > ArraysHashCode.chars 100 15 69.741 ± 0.11269.714 ± 0.044 > ns/op 1.00x (p = 0.365 ) > ArraysHashCode.chars1 15 9367.123 ± 5.320 9371.325 ± 6.407 > ns/op 1.00x (p = 0.046 ) > ArraysHashCode.ints 1 150.711 ± 0.013 0.706 ± 0.006 > ns/op 1.01x (p = 0.137 ) > ArraysHashCode.ints10 153.750 ± 0.002 3.752 ± 0.004 > ns/op 1.00x (p = 0.283 ) > ArraysHashCode.ints 100 15 69.753 ± 0.08669.711 ± 0.016 > ns/op 1.00x (p = 0.065 ) > ArraysHashCode.ints 1 15 9376.225 ± 5.845 9376.218 ± 12.181 > ns/op 1.00x (p = 0.999 ) > ArraysHashCode.multibytes 1 150.741 ± 0.001 0.740 ± 0.001 > ns/op 1.00x (p = 0.038 ) > ArraysHashCode.multibytes 10 152.737 ± 0.001 2.826 ± 0.136 > ns/op 0.97x (p = 0.017 ) > ArraysHashCode.multibytes 100 15 32.202 ± 0.05932.153 ± 0.006 > ns/op 1.00x (p = 0.004*) > ArraysHashCode.multibytes 1 15 4922.740 ± 25.590 4921.468 ± 7.372 > ns/op 1.00x (p = 0.846 ) > ArraysHashCode.multichars 1 150.740 ± 0.005 0.740 ± 0.000 > ns/op 1.00x (p = 0.996 ) > ArraysHashCode.multichars 10 152.732 ± 0.002 2.737 ± 0.003 > ns/op 1.00x (p = 0.000*) > ArraysHashCode.multichars 100 15 32.109 ± 0.01732.182 ± 0.028 > ns/op 1.00x (p = 0.000*) > ArraysHashCode.multichars 1 15 4925.750 ± 46.366 4930.684 ± 26.001 > ns/op 1.00x (p = 0.704 ) > ArraysHashCode.multiints1 150.740 ± 0.000 0.739 ± 0.000 > ns/op 1.00x (p = 0.000*) > ArraysHashCode.multiints 10 152.919 ± 0.002 2.953 ± 0.059 > ns/op 0.99x (p = 0.033 ) > ArraysHashCode.multiints 100 15 32.140 ± 0.01132.094 ± 0.004 > ns/op 1.00x (p = 0.000*) > ... Just to clarify, the above comparison is between master and https://github.com/openjdk/jdk/pull/19414/commits/534ff367bc50ec4150e4b206ce7203c7ff9f5cad. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1619125150
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v3]
> 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 three additional commits since the last revision: - Update copyright years Note: any commit hashes below might be outdated due to subsequent history rewriting (e.g. git rebase). + update src/java.base/share/classes/java/lang/CharacterName.java due to 4ed451d691c + update src/java.base/share/classes/java/lang/StringLatin1.java due to 4ed451d691c + update src/java.base/share/classes/java/nio/Heap-X-Buffer.java.template due to 4ed451d691c + update src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java due to 4ed451d691c + update src/java.base/share/classes/sun/security/util/DerValue.java due to 4ed451d691c + update src/java.base/unix/classes/sun/nio/fs/UnixPath.java due to 4ed451d691c + update test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java due to 4ed451d691c - Drop redundant (int) cast - Use Byte.toUnsignedInt not & 0xff - Changes: - all: https://git.openjdk.org/jdk/pull/19414/files - new: https://git.openjdk.org/jdk/pull/19414/files/adc7557d..53d4ed09 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19414=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19414=01-02 Stats: 12 lines in 8 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/19414.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19414/head:pull/19414 PR: https://git.openjdk.org/jdk/pull/19414
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:13:50 GMT, Jorn Vernee wrote:
>> 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
To be honest, I don't think that this cast is needed. A better solution than to
add a comment would be to delete all `(int)` casts from new `hashCode*` methods
of `ArraysSupport`.
Those `(int)` casts migrated from `hashCode` methods of `Arrays` where there
were used if neither of two `+` operands were of type `int`. But in
`ArraysSupport` it's no longer the case: `31 * initialValue` is always `int`
because `initialValue` is. So, `a[fromIndex]` is promoted to `int` by the
virtue of https://docs.oracle.com/javase/specs/jls/se22/html/jls-5.html#jls-5.6.
For more confidence, consider that the `private static int hashCode` methods
(implementation) of `ArraysSupport` do not have those casts.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618835934
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 20:40:30 GMT, Claes Redestad wrote:
>> 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]);
>
> I don't care as long as microbenchmarks don't get a hiccup.
@cl4es, here are some results from my machine (macosx-aarch64):
Name (size) Cnt BaseError TestError
Unit Change
ArraysHashCode.bytes1 150.715 ± 0.004 0.725 ± 0.029
ns/op 0.99x (p = 0.182 )
ArraysHashCode.bytes 10 153.753 ± 0.024 3.747 ± 0.011
ns/op 1.00x (p = 0.322 )
ArraysHashCode.bytes 100 15 69.731 ± 0.15769.737 ± 0.092
ns/op 1.00x (p = 0.891 )
ArraysHashCode.bytes1 15 9369.386 ± 1.449 9372.008 ± 6.678
ns/op 1.00x (p = 0.133 )
ArraysHashCode.chars1 150.719 ± 0.024 0.734 ± 0.024
ns/op 0.98x (p = 0.076 )
ArraysHashCode.chars 10 153.744 ± 0.005 3.746 ± 0.004
ns/op 1.00x (p = 0.308 )
ArraysHashCode.chars 100 15 69.741 ± 0.11269.714 ± 0.044
ns/op 1.00x (p = 0.365 )
ArraysHashCode.chars1 15 9367.123 ± 5.320 9371.325 ± 6.407
ns/op 1.00x (p = 0.046 )
ArraysHashCode.ints 1 150.711 ± 0.013 0.706 ± 0.006
ns/op 1.01x (p = 0.137 )
ArraysHashCode.ints10 153.750 ± 0.002 3.752 ± 0.004
ns/op 1.00x (p = 0.283 )
ArraysHashCode.ints 100 15 69.753 ± 0.08669.711 ± 0.016
ns/op 1.00x (p = 0.065 )
ArraysHashCode.ints 1 15 9376.225 ± 5.845 9376.218 ± 12.181
ns/op 1.00x (p = 0.999 )
ArraysHashCode.multibytes 1 150.741 ± 0.001 0.740 ± 0.001
ns/op 1.00x (p = 0.038 )
ArraysHashCode.multibytes 10 152.737 ± 0.001 2.826 ± 0.136
ns/op 0.97x (p = 0.017 )
ArraysHashCode.multibytes 100 15 32.202 ± 0.05932.153 ± 0.006
ns/op 1.00x (p = 0.004*)
ArraysHashCode.multibytes 1 15 4922.740 ± 25.590 4921.468 ± 7.372
ns/op 1.00x (p = 0.846 )
ArraysHashCode.multichars 1 150.740 ± 0.005 0.740 ± 0.000
ns/op 1.00x (p = 0.996 )
ArraysHashCode.multichars 10 152.732 ± 0.002 2.737 ± 0.003
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multichars 100 15 32.109 ± 0.01732.182 ± 0.028
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multichars 1 15 4925.750 ± 46.366 4930.684 ± 26.001
ns/op 1.00x (p = 0.704 )
ArraysHashCode.multiints1 150.740 ± 0.000 0.739 ± 0.000
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multiints 10 152.919 ± 0.002 2.953 ± 0.059
ns/op 0.99x (p = 0.033 )
ArraysHashCode.multiints 100 15 32.140 ± 0.01132.094 ± 0.004
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multiints1 15 4918.911 ± 3.512 4913.884 ± 11.618
ns/op 1.00x (p = 0.105 )
ArraysHashCode.multishorts 1 150.740 ± 0.001 0.739 ± 0.000
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multishorts 10 152.736 ± 0.002 2.733 ± 0.008
ns/op 1.00x (p = 0.159 )
ArraysHashCode.multishorts100 15 32.162 ± 0.03332.105 ± 0.008
ns/op 1.00x (p = 0.000*)
ArraysHashCode.multishorts 1 15 4916.984 ± 3.276 4912.000 ± 11.479
ns/op 1.00x (p = 0.103 )
ArraysHashCode.shorts 1 150.711 ± 0.023 0.709 ± 0.016
ns/op 1.00x (p = 0.818 )
ArraysHashCode.shorts 10 153.745 ± 0.003 3.739 ± 0.010
ns/op 1.00x (p = 0.049 )
ArraysHashCode.shorts 100 15 69.725 ± 0.08269.620 ± 0.051
ns/op 1.00x (p = 0.000*)
ArraysHashCode.shorts 1 15 9370.882 ± 8.306 9356.215 ± 3.996
ns/op 1.00x (p = 0.000*)
* = significant
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618821363
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo 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 Marked as reviewed by liach (Author). - PR Review: https://git.openjdk.org/jdk/pull/19414#pullrequestreview-2085162416
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 03:21:27 GMT, Chen Liang wrote:
>> 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 320:
>
>> 318: * @return the calculated hash value
>> 319: */
>> 320: public static int hashCode(Object[] a, int fromIndex, int length,
>> int initialValue) {
>
> Is the object variant necessary here? The object version is hard for JIT to
> profile as it's quite polymorphic compared to other arrays, and the initial
> value is always 1.
This is a cleanup/refactoring PR, so none of this is necessary. My motivation
for the `Object[]` variant was to provide reusable functionality for methods
like these:
-
https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083
-
https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618644177
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 20:21:34 GMT, Claes Redestad wrote:
>> 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
>
> The non-constant test was added because that very bailout caused a crash. The
> other test is actually less interesting since it'll likely be covered
> indirectly by regular use. But as we are hiding these away this gets ever
> more obscure and perhaps the test could be dropped entirely.
@cl4es, do you want me to delete that test file altogether?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618536122
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo 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 Marked as reviewed by redestad (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19414#pullrequestreview-2084714609
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Wed, 29 May 2024 03:16:02 GMT, Chen Liang wrote: >> In fact, if I change it to `2`, the following tests will fail: >> >> - `jdk/jdk/classfile/Utf8EntryTest.java` >> - `jdk/java/util/zip/ZipCoding.java` >> - `jdk/java/text/Format/MessageFormat/MessageRegression.java` > > Indeed, the actual length passed at call site is `value.length >> 1` instead > of `value.length`; this adjusted char-length carries on to > `vectorizedHashCode` call. Ah, sneaky. Might affect scores for the zero and one-char cases since the shift now happens unconditionally, but probably doesn't matter in practice. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618456668
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo 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 320:
> 318: * @return the calculated hash value
> 319: */
> 320: public static int hashCode(Object[] a, int fromIndex, int length,
> int initialValue) {
Is the object variant necessary here? The object version is hard for JIT to
profile as it's quite polymorphic compared to other arrays, and the initial
value is always 1.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618129002
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 22:20:39 GMT, Pavel Rappo wrote: >> I believe, it should be `1`. Hear me out. In this method, the `length` is >> scaled down, whereas in `StringUTF16` it is not. In this method, it's >> `length`, in `StringUTF16` it's `((byte[]) value).length`. > > In fact, if I change it to `2`, the following tests will fail: > > - `jdk/jdk/classfile/Utf8EntryTest.java` > - `jdk/java/util/zip/ZipCoding.java` > - `jdk/java/text/Format/MessageFormat/MessageRegression.java` Indeed, the actual length passed at call site is `value.length >> 1` instead of `value.length`; this adjusted char-length carries on to `vectorizedHashCode` call. - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1618126401
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 22:08:06 GMT, Pavel Rappo wrote: >> Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass >> functional testing since `1` can never happen in practice, and the default >> case should work for any value. There might be a String microbenchmark out >> there that might be slightly unhappy, though. > > I believe, it should be `1`. Hear me out. In this method, the `length` is > scaled down, whereas in `StringUTF16` it is not. In this method, it's > `length`, in `StringUTF16` it's `((byte[]) value).length`. In fact, if I change it to `2`, the following tests will fail: - `jdk/jdk/classfile/Utf8EntryTest.java` - `jdk/java/util/zip/ZipCoding.java` - `jdk/java/text/Format/MessageFormat/MessageRegression.java` - PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617950633
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 20:38:21 GMT, Claes Redestad wrote:
>> 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`.
>
> Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass
> functional testing since `1` can never happen in practice, and the default
> case should work for any value. There might be a String microbenchmark out
> there that might be slightly unhappy, though.
I believe, it should be `1`. Hear me out. In this method, the `length` is
scaled down, whereas in `StringUTF16` it is not. In this method, it's `length`,
in `StringUTF16` it's `((byte[]) value).length`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617941436
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:13:30 GMT, Jorn Vernee wrote:
>> 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 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]);
I don't care as long as microbenchmarks don't get a hiccup.
> 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`.
Yes, should be `2` (`a` is semantically a `char[]`). This typo likely pass
functional testing since `1` can never happen in practice, and the default case
should work for any value. There might be a String microbenchmark out there
that might be slightly unhappy, though.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617867797
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617865658
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Tue, 28 May 2024 19:19:51 GMT, Jorn Vernee wrote:
>> Pavel Rappo has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix incorrect utf16 hashCode adaptation
>
> 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
The non-constant test was added because that very bailout caused a crash. The
other test is actually less interesting since it'll likely be covered
indirectly by regular use. But as we are hiding these away this gets ever more
obscure and perhaps the test could be dropped entirely.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617848032
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo 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_r161798
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617784996
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/19414/files - new: https://git.openjdk.org/jdk/pull/19414/files/4ed451d6..adc7557d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19414=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19414=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19414.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19414/head:pull/19414 PR: https://git.openjdk.org/jdk/pull/19414
RFR: 8332826: Make hashCode methods in ArraysSupport friendlier
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` - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/19414/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19414=00 Issue: https://bugs.openjdk.org/browse/JDK-8332826 Stats: 258 lines in 13 files changed: 186 ins; 32 del; 40 mod Patch: https://git.openjdk.org/jdk/pull/19414.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19414/head:pull/19414 PR: https://git.openjdk.org/jdk/pull/19414
