Re: RFR: 8329597: C2: Intrinsify Reference.clear
On Thu, 11 Jul 2024 15:28:37 GMT, Aleksey Shipilev wrote: > [JDK-8240696](https://bugs.openjdk.org/browse/JDK-8240696) added the native > method for `Reference.clear`. The original patch skipped intrinsification of > this method, because we thought `Reference.clear` is not on a performance > sensitive path. However, it shows up prominently on simple benchmarks that > touch e.g. `ThreadLocal` cleanups. See the bug for an example profile with > `RRWL` benchmarks. > > Additional testing: > - [x] Linux x86_64 server fastdebug, `all` > - [ ] Linux AArch64 server fastdebug, `all` On Mac AArch64, which suffers from both native call and WX transition: Benchmark Mode Cnt Score Error Units # Intrinsic OFF ReferenceClear.phantom avgt9 52,297 ? 0,294 ns/op ReferenceClear.phantom_new avgt9 57,075 ? 0,296 ns/op ReferenceClear.soft avgt9 52,567 ? 0,393 ns/op ReferenceClear.soft_new avgt9 57,640 ? 0,264 ns/op ReferenceClear.weak avgt9 53,018 ? 1,285 ns/op ReferenceClear.weak_new avgt9 57,227 ? 0,483 ns/op # Intrinsic ON (default) ReferenceClear.phantom avgt9 0,780 ? 0,017 ns/op ReferenceClear.soft avgt9 0,784 ? 0,022 ns/op ReferenceClear.weak avgt9 0,793 ? 0,033 ns/op ReferenceClear.phantom_new avgt9 3,018 ? 0,015 ns/op ReferenceClear.soft_new avgt9 3,268 ? 0,014 ns/op ReferenceClear.weak_new avgt9 3,004 ? 0,057 ns/op On x86_64 m7a.16xlarge, which only suffers from the native call: Benchmark Mode Cnt Score Error Units # Intrinsic OFF ReferenceClear.phantom avgt9 14.643 ± 0.049 ns/op ReferenceClear.soft avgt9 14.939 ± 0.438 ns/op ReferenceClear.weak avgt9 14.648 ± 0.081 ns/op ReferenceClear.phantom_new avgt9 19.859 ± 2.405 ns/op ReferenceClear.soft_new avgt9 20.208 ± 1.805 ns/op ReferenceClear.weak_new avgt9 20.385 ± 2.570 ns/op # Intrinsic ON (default) ReferenceClear.phantom avgt9 0.821 ± 0.010 ns/op ReferenceClear.soft avgt9 0.817 ± 0.007 ns/op ReferenceClear.weak avgt9 0.819 ± 0.010 ns/op ReferenceClear.phantom_new avgt9 4.195 ± 0.729 ns/op ReferenceClear.soft_new avgt9 4.315 ± 0.599 ns/op ReferenceClear.weak_new avgt9 3.986 ± 0.596 ns/op - PR Comment: https://git.openjdk.org/jdk/pull/20139#issuecomment-2223248114
RFR: 8329597: C2: Intrinsify Reference.clear
[JDK-8240696](https://bugs.openjdk.org/browse/JDK-8240696) added the native method for `Reference.clear`. The original patch skipped intrinsification of this method, because we thought `Reference.clear` is not on a performance sensitive path. However, it shows up prominently on simple benchmarks that touch e.g. `ThreadLocal` cleanups. See the bug for an example profile with `RRWL` benchmarks. Additional testing: - [x] Linux x86_64 server fastdebug, `all` - [ ] Linux AArch64 server fastdebug, `all` - Commit messages: - Move the membar at the end - Revert C1 parts - Work Changes: https://git.openjdk.org/jdk/pull/20139/files Webrev: https://webrevs.openjdk.org/?repo=jdk=20139=00 Issue: https://bugs.openjdk.org/browse/JDK-8329597 Stats: 132 lines in 7 files changed: 132 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20139.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20139/head:pull/20139 PR: https://git.openjdk.org/jdk/pull/20139
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Tue, 11 Jun 2024 08:58:34 GMT, Aleksey Shipilev wrote: >> Sean Gwizdak has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains six additional >> commits since the last revision: >> >> - Remove trailing whitespace. >> - Move hashCode benchmark into the newly created MethodBenchmark file >> - Merge branch 'master' into method-hashcode-JDK-8332249 >> - Remove changes to JavaDoc per guidance. >> - Fix whitespace issues pointed by the bot >> - Micro-optimize Method.hashCode > > As usual in these cases, we need to make a footprint argument as well. > `Method` is a special class with lots of injected fields, so the only "true" > source of layout information is `-XX:+PrintFieldLayout`. It tells me there is > a 4-byte tail due to object alignment in compressed oops mode. This can > accommodate a new `int` field. There seems to be no space when compressed > oops are disabled. > > > # Out of the box, compressed oops enabled > Layout of class java/lang/reflect/Method > Instance fields: > @0 12/- RESERVED > @12 "override" Z 1/1 INHERITED > @13 "callerSensitive" B 1/1 REGULAR > @14 2/1 EMPTY > @16 "accessCheckCache" Ljava/lang/Object; 4/4 INHERITED > @20 "parameterData" Ljava/lang/reflect/Executable$ParameterData; 4/4 > INHERITED > @24 "declaredAnnotations" Ljava/util/Map; 4/4 INHERITED > @28 "slot" I 4/4 REGULAR > @32 "modifiers" I 4/4 REGULAR > @36 "clazz" Ljava/lang/Class; 4/4 REGULAR > @40 "name" Ljava/lang/String; 4/4 REGULAR > @44 "returnType" Ljava/lang/Class; 4/4 REGULAR > @48 "parameterTypes" [Ljava/lang/Class; 4/4 REGULAR > @52 "exceptionTypes" [Ljava/lang/Class; 4/4 REGULAR > @56 "signature" Ljava/lang/String; 4/4 REGULAR > @60 "genericInfo" Lsun/reflect/generics/repository/MethodRepository; 4/4 > REGULAR > @64 "annotations" [B 4/4 REGULAR > @68 "parameterAnnotations" [B 4/4 REGULAR > @72 "annotationDefault" [B 4/4 REGULAR > @76 "methodAccessor" Ljdk/internal/reflect/MethodAccessor; 4/4 REGULAR > @80 "root" Ljava/lang/reflect/Method; 4/4 REGULAR > Instance size = 88 bytes > > # -XX:-UseCompressedOops > Layout of class java/lang/reflect/Method > Instance fields: > @0 12/- RESERVED > @12 "override" Z 1/1 INHERITED > @13 "callerSensitive" B 1/1 REGULAR > @14 2/1 EMPTY > @16 "accessCheckCache" Ljava/lang/Object; 8/8 INHERITED > @24 "parameterData" Ljava/lang/reflect/Executable$ParameterData; 8/8 > INHERITED > @32 "declaredAnnotations" Ljava/util/Map; 8/8 INHERITED > @40 "slot" I 4/4 REGULAR > @44 "modifiers" I 4/4 REGULAR > @48 "clazz" Ljava/lang/Class; 8/8 REGULAR > @56 "name" Ljava/lang/String; 8/8 REGULAR > @64 "returnType" Ljava/lang/Class; 8/8 REGULAR > @72 "parameterTypes" [Ljava/lang/Class; 8/8 REGULAR > @80 "exceptionTypes" [Ljava/lang/Class; 8/8 REGULAR > @88 "signature" Ljava/lang/String; 8/8 REGULAR > @96 "genericInfo" Lsun/reflect/generics/repository/MethodRepository; 8/8 > REGULAR > @104 "annotations" [B 8/8 REGULAR > @112 "parameterAnnotations" [B 8/8 REGULAR > @120 "annotationDefault" [B 8/8 REGULAR > @128 "methodAccessor" Ljdk/internal/reflect/MethodAccessor; 8/8 REGULAR > @136 "root" Ljava/... > @shipilev Any recommendations on reducing footprint size? I see two ways from here: 1. There seems to be a 2-byte gap in the object, so if we can squeeze out 2 additional bytes, it would allow us to put another `int` in. It looks like `slot` does not have to be `int`: on VM side, it is `u2` (unsigned 2-byte), which might be just `short` or `char` with proper precautions about signs and conversions. That would probably be a bit ugly. 2. Ignore this regression and move on. It would have been a non-issue if `Method`-s were canonicalized/de-duplicated in some way. But AFAICS, `Method`-s are getting copied from `Class.getDeclaredMethods`, so it is probable we have quite a few instances floating around, and footprint regressions would stack up. - PR Comment: https://git.openjdk.org/jdk/pull/19433#issuecomment-532168
Integrated: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
On Thu, 27 Jun 2024 19:30:34 GMT, Aleksey Shipilev wrote: > I was auditing the current uses of `@Stable` before relaxing its barriers > ([JDK-8333791](https://bugs.openjdk.org/browse/JDK-8333791)), and this is an > easy spot. > > `resolvedEnum` is not `final`. So technically publishing the object via data > race can show `resolvedEnum` as `null`, which would break `test()` that does > not expect it. Currently not a practical problem since its safety is covered > by adjacent `final` fields, but should be more precise. This pull request has now been integrated. Changeset: 45c4eaa5 Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/45c4eaa5600016d3da5ca769b2519df53835e4f7 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final Reviewed-by: liach, jlahoda - PR: https://git.openjdk.org/jdk/pull/19933
Re: RFR: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
On Thu, 27 Jun 2024 19:30:34 GMT, Aleksey Shipilev wrote: > I was auditing the current uses of `@Stable` before relaxing its barriers > ([JDK-8333791](https://bugs.openjdk.org/browse/JDK-8333791)), and this is an > easy spot. > > `resolvedEnum` is not `final`. So technically publishing the object via data > race can show `resolvedEnum` as `null`, which would break `test()` that does > not expect it. Currently not a practical problem since its safety is covered > by adjacent `final` fields, but should be more precise. > FWIW, there are more `@Stable` uses here: #19906 > if you would have a moment to check that, it may be helpful. Thanks. That one looks fine: it is set outside of constructor, is intrinsically racy, and it has AFAICS the recovery paths when we read `null` out of `MappedEnumCache.generatedSwitch` or `MappedEnumCache.constantsMap`. - PR Comment: https://git.openjdk.org/jdk/pull/19933#issuecomment-2196506663
RFR: 8335274: SwitchBootstraps.ResolvedEnumLabels.resolvedEnum should be final
I was auditing the current uses of `@Stable` before relaxing its barriers ([JDK-8333791](https://bugs.openjdk.org/browse/JDK-8333791)), and this is an easy spot. `resolvedEnum` is not `final`. So technically publishing the object via data race can show `resolvedEnum` as `null`, which would break `test()` that does not expect it. Currently not a practical problem since its safety is covered by adjacent `final` fields, but should be more precise. - Commit messages: - Fix Changes: https://git.openjdk.org/jdk/pull/19933/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19933=00 Issue: https://bugs.openjdk.org/browse/JDK-8335274 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19933.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19933/head:pull/19933 PR: https://git.openjdk.org/jdk/pull/19933
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject @AlanBateman -- could you please take a look? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2178515650
Re: RFR: 8332249: Micro-optimize Method.hashCode [v2]
On Mon, 3 Jun 2024 18:00:35 GMT, Sean Gwizdak wrote: >> Improve the speed of Method.hashCode by caching the hashcode on first use. >> I've seen an application where Method.hashCode is a hot path, and this is a >> fairly simple speedup. The memory overhead is low. >> >> This addresses issue >> [JDK-8332249](https://bugs.openjdk.org/browse/JDK-8332249). >> >> Before: >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.843 ± 0.149 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 2.363 ± 0.091 ns/op >> >> >> >> After: >> >> >> Benchmark Mode Cnt Score Error Units >> # Intel Skylake >> MethodHashCode.benchmarkHashCode avgt5 1.121 ± 1.189 ns/op >> # Arm Neoverse N1 >> MethodHashCode.benchmarkHashCode avgt5 1.001 ± 0.001 ns/op > > Sean Gwizdak has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains six additional > commits since the last revision: > > - Remove trailing whitespace. > - Move hashCode benchmark into the newly created MethodBenchmark file > - Merge branch 'master' into method-hashcode-JDK-8332249 > - Remove changes to JavaDoc per guidance. > - Fix whitespace issues pointed by the bot > - Micro-optimize Method.hashCode As usual in these cases, we need to make a footprint argument as well. `Method` is a special class with lots of injected fields, so the only "true" source of layout information is `-XX:+PrintFieldLayout`. It tells me there is a 4-byte tail due to object alignment in compressed oops mode. This can accommodate a new `int` field. There seems to be no space when compressed oops are disabled. # Out of the box, compressed oops enabled Layout of class java/lang/reflect/Method Instance fields: @0 12/- RESERVED @12 "override" Z 1/1 INHERITED @13 "callerSensitive" B 1/1 REGULAR @14 2/1 EMPTY @16 "accessCheckCache" Ljava/lang/Object; 4/4 INHERITED @20 "parameterData" Ljava/lang/reflect/Executable$ParameterData; 4/4 INHERITED @24 "declaredAnnotations" Ljava/util/Map; 4/4 INHERITED @28 "slot" I 4/4 REGULAR @32 "modifiers" I 4/4 REGULAR @36 "clazz" Ljava/lang/Class; 4/4 REGULAR @40 "name" Ljava/lang/String; 4/4 REGULAR @44 "returnType" Ljava/lang/Class; 4/4 REGULAR @48 "parameterTypes" [Ljava/lang/Class; 4/4 REGULAR @52 "exceptionTypes" [Ljava/lang/Class; 4/4 REGULAR @56 "signature" Ljava/lang/String; 4/4 REGULAR @60 "genericInfo" Lsun/reflect/generics/repository/MethodRepository; 4/4 REGULAR @64 "annotations" [B 4/4 REGULAR @68 "parameterAnnotations" [B 4/4 REGULAR @72 "annotationDefault" [B 4/4 REGULAR @76 "methodAccessor" Ljdk/internal/reflect/MethodAccessor; 4/4 REGULAR @80 "root" Ljava/lang/reflect/Method; 4/4 REGULAR Instance size = 88 bytes # -XX:-UseCompressedOops Layout of class java/lang/reflect/Method Instance fields: @0 12/- RESERVED @12 "override" Z 1/1 INHERITED @13 "callerSensitive" B 1/1 REGULAR @14 2/1 EMPTY @16 "accessCheckCache" Ljava/lang/Object; 8/8 INHERITED @24 "parameterData" Ljava/lang/reflect/Executable$ParameterData; 8/8 INHERITED @32 "declaredAnnotations" Ljava/util/Map; 8/8 INHERITED @40 "slot" I 4/4 REGULAR @44 "modifiers" I 4/4 REGULAR @48 "clazz" Ljava/lang/Class; 8/8 REGULAR @56 "name" Ljava/lang/String; 8/8 REGULAR @64 "returnType" Ljava/lang/Class; 8/8 REGULAR @72 "parameterTypes" [Ljava/lang/Class; 8/8 REGULAR @80 "exceptionTypes" [Ljava/lang/Class; 8/8 REGULAR @88 "signature" Ljava/lang/String; 8/8 REGULAR @96 "genericInfo" Lsun/reflect/generics/repository/MethodRepository; 8/8 REGULAR @104 "annotations" [B 8/8 REGULAR @112 "parameterAnnotations" [B 8/8 REGULAR @120 "annotationDefault" [B 8/8 REGULAR @128 "methodAccessor" Ljdk/internal/reflect/MethodAccessor; 8/8 REGULAR @136 "root" Ljava/lang/reflect/Method; 8/8 REGULAR Instance size = 144 bytes - PR Comment: https://git.openjdk.org/jdk/pull/19433#issuecomment-2160170846
Re: RFR: 8325984: 4 jcstress tests are failing in Tier6 4 times each
On Wed, 5 Jun 2024 19:21:56 GMT, Jorn Vernee wrote: > These 4 tests were failing due to an incompatibility with jcstress. They were > problemlisted in past (https://bugs.openjdk.org/browse/JDK-8326062). > > Now that jcstress has been updated > (https://github.com/openjdk/jdk/pull/19332) with the relevant fix > (https://github.com/openjdk/jcstress/pull/147), we can re-enable these tests. > > Testing: I've verified that all 4 tests now pass on Linux-x64 I think this is fine and trivial. - PR Comment: https://git.openjdk.org/jdk/pull/19565#issuecomment-2154964073
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject I think this is good. I'll let Doug and Viktor to confirm their comments were addressed. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19527#pullrequestreview-2102178280
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 5 Jun 2024 14:56:26 GMT, jengebr wrote: > clone() performs a shallow copy, so there is currently no Object[] allocated > and therefore nothing to optimize. Yes, I believe so. - PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1629666551
Re: RFR: 8325984: 4 jcstress tests are failing in Tier6 4 times each
On Wed, 5 Jun 2024 19:21:56 GMT, Jorn Vernee wrote: > These 4 tests were failing due to an incompatibility with jcstress. They were > problemlisted in past (https://bugs.openjdk.org/browse/JDK-8326062). > > Now that jcstress has been updated > (https://github.com/openjdk/jdk/pull/19332) with the relevant fix > (https://github.com/openjdk/jcstress/pull/147), we can re-enable these tests. > > Testing: I've verified that all 4 tests now pass on Linux-x64 I think only Oracle CIs run these tests through jtreg wrappers? Anyway, this looks good to me. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19565#pullrequestreview-2101607822
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations
On Mon, 3 Jun 2024 16:47:20 GMT, jengebr wrote:
> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless
> cloning of Object[0] instances. This cloning is intended to prevent callers
> from changing array contents, but many `CopyOnWriteArrayList`s are allocated
> to size zero, or are otherwise maintained empty, so cloning is unnecessary.
>
> Results from the included JMH benchmark:
> Before:
>
> BenchmarkMode Cnt
> Score Error Units
> CopyOnWriteArrayListBenchmark.clear avgt5
> 74.487 ± 1.793 ns/op
> CopyOnWriteArrayListBenchmark.clearEmpty avgt5
> 27.918 ± 0.759 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5
> 16.656 ± 0.375 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5
> 15.415 ± 0.489 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5
> 21.608 ± 0.363 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5
> 15.374 ± 0.260 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5
> 15.688 ± 0.350 ns/op
>
>
> After:
>
> BenchmarkMode Cnt
> Score Error Units
> CopyOnWriteArrayListBenchmark.clear avgt5
> 75.365 ± 2.092 ns/op
> CopyOnWriteArrayListBenchmark.clearEmpty avgt5
> 20.803 ± 0.539 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5
> 16.808 ± 0.582 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5
> 12.980 ± 0.418 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5
> 21.627 ± 0.173 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5
> 12.864 ± 0.408 ns/op
> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5
> 12.931 ± 0.255 ns/op
Some initial comments. @DougLea might want to give it a sanity check.
Note there is a jcheck failure due to whitespace issues. Plus, I think the PR
should still be named "8332842: Optimize empty CopyOnWriteArrayList
allocations"?
src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java line
338:
> 336: */
> 337: public Object[] toArray() {
> 338: return getArray().length == 0 ? getArray() : getArray().clone();
Unfortunately, I think this is against the spec for this method, which
explicitly says the method must allocate a new array. Yes, this change would be
within the spirit of the spec ("You can modify the array", which you cannot if
the object is empty), but is against the letter of it.
-
PR Review: https://git.openjdk.org/jdk/pull/19527#pullrequestreview-2094435376
PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2145853813
PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1624791561
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v6]
On Fri, 31 May 2024 18:39:33 GMT, jengebr wrote: >> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of >> Class[0] instances. This cloning is intended to prevent callers from >> changing array contents, but many Methods have zero exceptions or zero >> parameters, and returning the original `Class[0]` is sufficient. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op >> >> >> After: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Fixing nits in benchmark I believe GHA failure (serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage, x86_32) is unrelated to this change, it is a known issue fixed in current master. We can proceed with integration. - PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2144546824
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v6]
On Fri, 31 May 2024 18:39:33 GMT, jengebr wrote: >> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of >> Class[0] instances. This cloning is intended to prevent callers from >> changing array contents, but many Methods have zero exceptions or zero >> parameters, and returning the original `Class[0]` is sufficient. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op >> >> >> After: >> >> BenchmarkMode Cnt Score Error Units >> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op >> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op >> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op >> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op >> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op >> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op >> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op >> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Fixing nits in benchmark Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091506122
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v5]
On Fri, 31 May 2024 17:59:18 GMT, jengebr wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but many Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>>
>> Results from the included JMH benchmark:
>> Before:
>>
>> BenchmarkMode Cnt Score Error Units
>> ConstructorBenchmark.getExceptionTypes avgt5 6.526 ± 0.183 ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty avgt5 5.803 ± 0.073 ns/op
>> ConstructorBenchmark.getParameterTypes avgt5 6.521 ± 0.188 ns/op
>> ConstructorBenchmark.getParameterTypesEmpty avgt5 5.747 ± 0.087 ns/op
>> MethodBenchmark.getExceptionTypesavgt5 6.525 ± 0.163 ns/op
>> MethodBenchmark.getExceptionTypesEmpty avgt5 5.783 ± 0.130 ns/op
>> MethodBenchmark.getParameterTypesavgt5 6.518 ± 0.195 ns/op
>> MethodBenchmark.getParameterTypesEmpty avgt5 5.742 ± 0.028 ns/op
>>
>>
>> After:
>>
>> BenchmarkMode Cnt Score Error Units
>> ConstructorBenchmark.getExceptionTypes avgt5 6.590 ± 0.124 ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty avgt5 1.351 ± 0.061 ns/op
>> ConstructorBenchmark.getParameterTypes avgt5 6.651 ± 0.132 ns/op
>> ConstructorBenchmark.getParameterTypesEmpty avgt5 1.353 ± 0.150 ns/op
>> MethodBenchmark.getExceptionTypesavgt5 6.701 ± 0.151 ns/op
>> MethodBenchmark.getExceptionTypesEmpty avgt5 1.422 ± 0.025 ns/op
>> MethodBenchmark.getParameterTypesavgt5 6.629 ± 0.142 ns/op
>> MethodBenchmark.getParameterTypesEmpty avgt5 1.273 ± 0.169 ns/op
>
> jengebr has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Fixing copyright message, returning values from benchmark
Thanks for adding the benchmark. This looks good to me. Other nits below, your
call if you want to fix them or not. (This is usually done if there are other
changes requested, but they are not important enough to require standalone
work.)
test/micro/org/openjdk/bench/java/lang/reflect/ConstructorBenchmark.java line
64:
> 62: throw new RuntimeException(e);
> 63: }
> 64:
Here and in another benchmark. Excess newline?
test/micro/org/openjdk/bench/java/lang/reflect/ConstructorBenchmark.java line
85:
> 83: public Object[] getParameterTypes() throws Exception {
> 84: return oneParameterConstructor.getParameterTypes();
> 85: }
Here and in another benchmark. Order looks weird: non-empty, empty, empty,
non-empty.
-
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091420074
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622774426
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622775072
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v2]
On Fri, 31 May 2024 16:21:36 GMT, jengebr wrote:
>> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
>> Class[0] instances. This cloning is intended to prevent callers from
>> changing array contents, but smany Methods have zero exceptions or zero
>> parameters, and returning the original `Class[0]` is sufficient.
>
> jengebr has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Adding JMH benchmark
Let's bikeshed about the benchmark a little :)
test/micro/org/openjdk/bench/java/lang/reflect/ExecutableParameterAndExceptionTypesBenchmark.java
line 49:
> 47: @Warmup(iterations = 3, time = 2, timeUnit = TimeUnit.SECONDS)
> 48: @Measurement(iterations = 5, time = 2, timeUnit = TimeUnit.SECONDS)
> 49: public class ExecutableParameterAndExceptionTypesBenchmark {
I think we want to have two benchmark classes, like we already have `Clazz` as
sibling.
Something like:
MethodBenchmark.getExceptionTypes
MethodBenchmark.getExceptionTypesEmpty
MethodBenchmark.getParameterTypes
MethodBenchmark.getParameterTypesEmtpy
ConstructorBenchmark.getExceptionTypes
ConstructorBenchmark.getExceptionTypesEmpty
ConstructorBenchmark.getParameterTypes
ConstructorBenchmark.getParameterTypesEmtpy
test/micro/org/openjdk/bench/java/lang/reflect/ExecutableParameterAndExceptionTypesBenchmark.java
line 66:
> 64: public void constructorExceptionsWithNoExceptions(Blackhole bh)
> throws Exception {
> 65: bh.consume(objectConstructor.getExceptionTypes());
> 66: }
Here and later, the common shape for these is:
@Benchmark
public Object[] constructorExceptionsWithNoExceptions() {
return objectConstructor.getExceptionTypes();
}
-
PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091270415
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622686626
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1622682560
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote: > Improve `java/lang/reflect/Method.java` by eliminating needless cloning of > Class[0] instances. This cloning is intended to prevent callers from > changing array contents, but smany Methods have zero exceptions or zero > parameters, and returning the original `Class[0]` is sufficient. This looks fine to me. Since this is a first-time contribution, someone else needs to take a look as well. You may want to put a simple microbenchmark somewhere here: https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/lang/reflect - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2089223720
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 14:32:23 GMT, Chen Liang wrote: >> Thanks. Unfortunately the variable `cloneArray` is actually a method >> parameter, and most of the callers pass in `false` - so using this utility >> method to control cloning would actually introduce cloning in several places >> where it is explicitly avoided. We may get a performance gain by modifying >> `Class.getInterfaces()` directly (the sole caller that passes `true`) then >> eliminating the parameter, then modifying each caller, etc. but that feels >> like a separate change inspired by this one. >> >> Thoughts? > > Why can't you do something like this: > > return cloneArray ? ReflectionFactory.copyClasses(interfaces) : interfaces; > > > Alternatively, your proposal is a good one too; we can rename this > `getInterfaces(boolean)` to `getInterfacesShared()` (like > `getSharedEnumConstants()`) and move this copy operation to `getInterfaces()` > as you suggest. I really see no reason to try and save on re-use of this one-line method for _everything_. In fact, I do not quite see a very compelling reason to even have the utility method. Sharing the code between `Method` and `Constructor` is clean enough and provides a good balance between reuse and cleanliness. Everything else can have the copy of the method definition, if needed. - PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1610112885
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Wed, 22 May 2024 19:48:49 GMT, Chen Liang wrote:
>> I really see no reason to try and save on re-use of this one-line method for
>> _everything_. In fact, I do not quite see a very compelling reason to even
>> have the utility method. Sharing the code between `Method` and `Constructor`
>> is clean enough and provides a good balance between reuse and cleanliness.
>> Everything else can have the copy of the method definition, if needed.
>
> Alternatively, if a utility method is overkill, we can inline these to
> `Executable`:
>
> public Class[] getParameterTypes() {
> var shared = getSharedParameterTypes();
> return shared.length == 0 ? shared : shared.clone();
> }
>
> And the overrides in `Method` and `Constructor` will simply call super; the
> declarations are kept to preserve the API documentation.
I had to read JLS to confirm that changing the `abstract` method to
non-abstract one does not break compatibility.
I am still thinking that we are overthinking this: the
readability/maintainability benefits for introducing a one-liner utility method
are slim at best. I believe we are spending the disproportionate time on this.
So if we cannot agree where to put the utility method -- which implies there is
no good place for it -- let's not do it at all. Inline the ternary selector in
4 affected places, and be done with it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1611304563
Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}
On Tue, 21 May 2024 13:49:18 GMT, jengebr wrote:
> Improve `java/lang/reflect/Method.java` by eliminating needless cloning of
> Class[0] instances. This cloning is intended to prevent callers from
> changing array contents, but smany Methods have zero exceptions or zero
> parameters, and returning the original `Class[0]` is sufficient.
Please also enable GHA testing: https://github.com/jengebr/jdk/actions. You may
need to trigger the test run manually after this, since the PR hook have
already missed it. Go to "OpenJDK Sanity Checks" on the left on that page, then
select your branch on in the drop-down on the right, and trigger the run.
src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line
249:
> 247: * the original can safely be reused.
> 248: */
> 249: public Class[] copyClasses(Class[] classes) {
There is no need to make this utility method non-static. This would also
obviate the need for having an instance of `ReflectionFactory` to access it.
Additionally, at the risk of more bikeshedding, I think this utility method is
better suited in `AccessibleObject` base class, with package-private
visibility. Putting the util methods in `ReflectionFactory` erodes this comment
a bit:
The methods in this class are extremely unsafe and can cause
subversion of both the language and the verifier. For this reason,
they are all instance methods, and access to the constructor of
this factory is guarded by a security check, in similar style to
{@link jdk.internal.misc.Unsafe}.
-
PR Comment: https://git.openjdk.org/jdk/pull/19327#issuecomment-2124338419
PR Review Comment: https://git.openjdk.org/jdk/pull/19327#discussion_r1609635793
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v3]
On Fri, 26 Apr 2024 08:45:45 GMT, Claes Redestad wrote:
>> Splitting out the ASM-based version from #18690 to push that first under the
>> JBS (to help backporting). Keeping #18690 open to rebase and follow-up on
>> this as a subtask. See discussion in that #18690 for more details,
>> discussion and motivation for this.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyright and comment about ProxyClassDumper
Looks okay, thanks. Only cosmetic nits:
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 108:
> 106: static {
> 107: String highArity =
> VM.getSavedProperty("java.lang.invoke.StringConcat.highArityThreshold");
> 108: HIGH_ARITY_THRESHOLD = Integer.parseInt(highArity != null ?
> highArity : "20");
Maybe write this as `= (highArity != null) ? Integer.parseInt(highArity) : 20`
to help interpreter a bit.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1105:
> 1103: for (int c = 0; c < constants.length; c++) {
> 1104: if (constants[c] != null)
> 1105: len += constants[c].length();
Braces?
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1118:
> 1116: );
> 1117:
> 1118:
Extra new-line?
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1164:
> 1162: }
> 1163:
> 1164:
Extra new-line?
-
Marked as reviewed by shade (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18953#pullrequestreview-2024514742
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580723293
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580723981
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580724174
PR Review Comment: https://git.openjdk.org/jdk/pull/18953#discussion_r1580724568
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v7]
On Wed, 24 Apr 2024 10:01:47 GMT, Claes Redestad wrote: > > I really wish this change was not done with ClassFile API, but with a > > simple bundled ASM, so it would be easily backportable, if we decide to. It > > does not look like CFA buys us a lot here readability/complexity wise: > > [d99b159](https://github.com/openjdk/jdk/commit/d99b1596c5ca57b110c1db88be430c6c54c3d599) > > I would be open to splitting out and pushing the ASM version first and do > this CFA port as a follow-up. That would be good, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2074585421
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases [v6]
On Thu, 18 Apr 2024 14:50:30 GMT, Claes Redestad wrote: >> This patch suggests a workaround to an issue with huge SCF MH expression >> trees taking excessive JIT compilation resources by reviving (part of) the >> simple bytecode-generating strategy that was originally available as an >> all-or-nothing strategy choice. >> >> Instead of reintroducing a binary strategy choice I propose a threshold >> parameter, controlled by >> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions >> below or at this threshold there's no change, for expressions with an arity >> above it we use the `StringBuilder`-chain bytecode generator. >> >> There are a few trade-offs at play here which influence the choice of >> threshold. The simple high arity strategy will for example not see any reuse >> of LambdaForms but strictly always generate a class per indy callsite, which >> means we might end up with a higher total number of classes generated and >> loaded in applications if we set this value too low. It may also produce >> worse performance on average. On the other hand there is the observed >> increase in C2 resource usage as expressions grow unwieldy. On the other >> other hand high arity expressions are likely rare to begin with, with less >> opportunities for sharing than the more common low-arity expressions. >> >> I turned the submitted test case into a few JMH benchmarks and did some >> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: >> >> Baseline strategy: >> 13 args: 6.3M >> 23 args: 18M >> 123 args: 868M >> >> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: >> 13 args: 2.11M >> 23 args: 3.67M >> 123 args: 4.75M >> >> For 123 args the memory overhead of the baseline strategy is 180x, but for >> 23 args we're down to a 5x memory overhead, and down to a 3x overhead for 13 >> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) >> I've conservatively chosen a threshold at arity 20. This keeps C2 resource >> pressure at a reasonable level (< 18M) while avoiding perturbing performance >> at the vast majority of call sites. >> >> I was asked to use the new class file API for mainline. There's a version of >> this patch implemented using ASM in 7c52a9f which might be a reasonable >> basis for a backport. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Minor SimpleStringBuilderStrategy:: overhead reduction I really wish this change was not done with ClassFile API, but with a simple bundled ASM, so it would be easily backportable, if we decide to. It does not look like CFA buys us a lot here readability/complexity wise: https://github.com/openjdk/jdk/pull/18690/commits/d99b1596c5ca57b110c1db88be430c6c54c3d599 - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2073284920
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Tue, 12 Mar 2024 18:42:48 GMT, Erik Joelsson wrote: >> Chad Rakoczy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Code cleanup > > I'm fine with just using VERSION_FEATURE. I think it's a simple and > straightforward enough simplification/approximation. If we think we need a > more exact version comparison, then I think we should use one of the version > strings instead of arbitrarily picking a set of version variables. @erikj79, are you good with this version? - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1996757094
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks I am good with this version, thanks! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1934831130
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 19:22:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with two additional > commits since the last revision: > > - Print to stdout instead of stderr > - Compare version using VERSION_STRING I was thinking what would happen for the first time we upgrade jspawnhelper like this. There would be no helpful message then, the `jspawnhelper` would just exit on argument count check. So I suggest we polish the failure messages a bit: always report version at `shutItDown`, and report more verbose failure messages too. See: [jspawnhelper-messages-1.patch](https://github.com/openjdk/jdk/files/14586196/jspawnhelper-messages-1.patch) Older JDK invoking new jspawnhelper would then fail like: % build/macosx-aarch64-server-fastdebug/images/jdk/lib/jspawnhelper 1:1:1 Incorrect number of arguments: 2 jspawnhelper version 23-internal-adhoc.shipilev.shipilev-jdk This command is not for general use and should only be run as the result of a call to ProcessBuilder.start() or Runtime.exec() in a java application - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1994051562
Re: RFR: 8325621: Improve jspawnhelper version checks [v3]
On Tue, 12 Mar 2024 23:44:45 GMT, Magnus Ihse Bursie wrote: >> Chad Rakoczy has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Print to stdout instead of stderr >> - Compare version using VERSION_STRING > > make/modules/java.base/Launcher.gmk line 81: > >> 79: SRC := $(TOPDIR)/src/$(MODULE)/unix/native/jspawnhelper, \ >> 80: OPTIMIZATION := LOW, \ >> 81: CFLAGS := $(CFLAGS_JDKEXE) \ > > There is no need to introduce a break after `$(CFLAGS_JDKEXE)`, for thing > like flags we try to fill the line. > Also, the indentation rules are that a broken line should be indented with > four spaces. See e.g. the CFLAGS line below in CoreLibraries.gmk. My fault, I suggested Chad to do this. So what's the preferred formatting here? Like BUILD_JEXEC block above does it? CFLAGS := $(CFLAGS_JDKEXE) $(VERSION_CFLAGS) \ -I$(TOPDIR)/src/$(MODULE)/unix/native/libjava, \ - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1522886607
Re: RFR: 8327998: Enable java/lang/ProcessBuilder/JspawnhelperProtocol.java on Mac
On Tue, 12 Mar 2024 21:52:31 GMT, Elif Aslan wrote: > This change enables to run JspawnhelperProtocol.java on MacOS. > > In addition to GHA , the test has been run on macos and linux. > > > Test report is stored in > build/macosx-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Finished building target 'test' in configuration > 'macosx-x86_64-server-fastdebug' > > > > > > Test report is stored in > build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR >jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java > 1 1 0 0 > == > TEST SUCCESS > > Stopping javac server > [ec2-user@ip-172-16-0-10 jdk]$ make test CONF=linux-x86_64-server-fastdebug > TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java Looks fine, thanks! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18253#pullrequestreview-1933545908
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote:
>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>>
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are
>> the same. Updates test to include check for version. Also tested manually by
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Code cleanup
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 173:
> 171: if (jdk_feature != VERSION_FEATURE || jdk_interim !=
> VERSION_INTERIM || jdk_update != VERSION_UPDATE || jdk_patch !=
> VERSION_PATCH) {
> 172: fprintf(stderr, "Expected jspawnhelper for Java %d.%d.%d.%d,
> ", jdk_feature, jdk_interim, jdk_update, jdk_patch);
> 173: fprintf(stderr, "but jspawnhelper for Java %d.%d.%d.%d was
> found.\n", VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH);
Minor: It is a bit odd to see that jspawnhelper found its own version odd. I'd
say:
fprintf(stderr, "jspawnhelper version check failed. jspawnhelper version:
%d.%d.%d.%d, JDK version: %d.%d.%d.%d\n",
VERSION_FEATURE, VERSION_INTERIM, VERSION_UPDATE, VERSION_PATCH,
jdk_feature, jdk_interim, jdk_update, jdk_patch);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520296858
Re: RFR: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 18:56:21 GMT, Bernd wrote: > with a protocol version you don’t have to care about micro versions and also > it is more tolerant about the usual cpu updates which do not introduce > incompatibilities most of the time. I think we can only reasonably guarantee that jspawnhelper built for a particular JDK is compatible. Protocol version does not exactly capture that: there might be a slight change in JDK that jspawnhelper needs to be aware about, but which does not readily manifest in handshake protocol. Version quadruplet seems to get us what we want automatically. Plus, as you said, protocol number would probably communicate a wrong message that there is some guarantee about the protocol. > btw just as a datapoint: we run into this issue with a longrunning Gerrit > server which could no longer invoke external ssh client for incoming hooks > (ad did not log this). It was not expected to use the system-vm which was > updated on the running system by ubuntu. Ouch! - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989231670
Re: RFR: 8325621: Improve jspawnhelper version checks
On Mon, 11 Mar 2024 18:16:53 GMT, Erik Joelsson wrote: > If you really want to require an exact match for jspawnhelper, then these 4 > numbers aren't necessarily enough, but a rather arbitrarily chosen > approximation. I think for our purposes, a version number quadruplet is enough to provide basic level of safety for jspawnhelper protocol updates across JDK versions, without version checking code being a safety problem itself. Putting in the full version string would raise more questions, at least for me. For example, are we guaranteed that version string always fits the argument line? Probably so. Would we get some interesting (Unicode?) symbols in version string that would break somewhere along the way? Would we need to dynamically allocate the buffer for arguments, instead of allocating enough to hold the version _integers_? Would we make a mistake while doing so? Etc. All in all, it feels better to silently accept some version mismatches not captured by the version quadruplet, rather than fail trying to do a perfect match. - PR Comment: https://git.openjdk.org/jdk/pull/18204#issuecomment-1989179598
Re: RFR: 8327675: jspawnhelper should be built on all unix platforms
On Fri, 8 Mar 2024 10:17:08 GMT, Magnus Ihse Bursie wrote: > We should match the building of jspawnhelper in > make/modules/java.base/Launcher.gmk with the usage for all unix platforms in > src/java.base/unix/classes/java/lang/ProcessImpl.java. > > Granted, the list of OSes in the makefile amounts to the current list of all > Unix OSes in the JDK, but there is no need to have this logical disparity. > > This was inspired by the discovery in > https://github.com/openjdk/jdk/pull/18112#discussion_r1517455696. Looks good! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18165#pullrequestreview-1924580539
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Fri, 8 Mar 2024 07:35:27 GMT, Magnus Ihse Bursie wrote: >> test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29: >> >>> 27: * @test >>> 28: * @bug 8325567 >>> 29: * @requires (os.family == "linux") | (os.family == "aix") >> >> Unless I'm mistaken, jspawn helper is used on Mac as well. > > Yes indeed, it is used for all Unix OSes (that is, everything but Windows). I think what matters for this test test most is which platforms we build `jspawnhelper` for, and those platforms indeed are: ifeq ($(call isTargetOs, macosx aix linux), true) $(eval $(call SetupJdkExecutable, BUILD_JSPAWNHELPER, \ So I'd say we just add `(os.family == "mac")` here. I would find it a bit weird to ask for `os.family != "windows"`, which would implicitly rely on exhaustiveness of current os family types. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517455696
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 20:04:13 GMT, Vladimir Petko wrote: > I wonder if it would make sense to add a test with a correct argument format? I would say that is what current jspawnhelper tests already test, and it is also tested implicitly with `Process` tests. Let's keep this test for testing that warning messages are printed on most common cases of misuse -- that is why `JspawnhelperWarnings` is a good name. - PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1517458419
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 17:56:21 GMT, Roger Riggs wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > I'm curious why this test is requires `vm.debug`? That means it generally > won't get run on release builds. Let's see if @RogerRiggs and @vpa1977 have any additional feedback. - PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1984137117
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]
On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan wrote: >> This change is intended to address the segmentation fault issue that occurs >> when jspawnhelper is called without arguments,. >> There is a new test added to verify the behavior in such cases. >> >> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test >> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java` >> >> >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java >> 1 1 0 0 >> == >> TEST SUCCESS > > Elif Aslan has updated the pull request incrementally with one additional > commit since the last revision: > > Add args[0] back I am good with this version. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1923081130
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v3]
On Thu, 7 Mar 2024 16:29:12 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use outputanalyzer
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 238:
> 236: }
> 237: }
> 238: }
This looks like a leftover, please revert.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 46:
> 44:
> 45: private static void jspawnhelperWithNArgs(int nArgs) throws Exception
> {
> 46: System.out.println("Running jspawnhelper with "+nArgs+" args");
Style: spaces around `+` operator.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 47:
> 45: private static void jspawnhelperWithNArgs(int nArgs) throws Exception
> {
> 46: System.out.println("Running jspawnhelper with "+nArgs+" args");
> 47: String[] args = new String[nArgs +1];
Style: spaces around `+` operator.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 54:
> 52: if (!p.waitFor(TIMEOUT, TimeUnit.SECONDS)) {
> 53: throw new RuntimeException("jspawnhelper timed out after " +
> TIMEOUT + " seconds");
> 54: }
I don't think we need to wait here, the `shouldHaveExitValue` waits for us.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516472478
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516472833
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516477172
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516476881
Re: RFR: 8325567: jspawnhelper without args fails with segfault [v4]
On Thu, 7 Mar 2024 16:33:11 GMT, Elif Aslan wrote:
>> This change is intended to address the segmentation fault issue that occurs
>> when jspawnhelper is called without arguments,.
>> There is a new test added to verify the behavior in such cases.
>>
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>>
>>
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>> 1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Revert JspawnhelperProtocol.java
So, the test is "passing" with `argc == 2` because jspawnhelper shuts down on
illegal `argv[1]`, right? This is probably fine.
More stylistic comments:
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 31:
> 29: * @requires (os.family == "linux") | (os.family == "aix")
> 30: * @library /test/lib
> 31: * @run main/othervm/timeout=300 JspawnhelperWarnings
This test does not have to be `othervm`, it could be `driver`:
@run driver JspawnhelperWarnings
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 61:
> 59: public static void main(String[] args) throws Exception {
> 60: for (int nArgs = 0; nArgs < 10; ++nArgs)
> 61: jspawnhelperWithNArgs(nArgs);
Style: braces for loop body. There is also no point in using `++nArgs`, when
`nArgs++` is sufficient.
test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 63:
> 61: jspawnhelperWithNArgs(nArgs);
> 62: }
> 63: }
Needs newline at the end of the file.
-
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1922949264
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516478694
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516481779
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516482020
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Tue, 5 Mar 2024 20:34:47 GMT, Vladimir Petko wrote:
> > The change in jspawnhelper looks good.
> > I think it would be simpler to have a separate
> > `jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java` test that simply
> > invokes the `jspawnhelper` and verifies the proper message is printed. It
> > would be more straightforward than trying to fit the _protocol_ test? Plus,
> > we can test 0, 1, 3, 4 args, not only 0 args in that test, and we can also
> > test 2 args with bad format.
>
> Yes, this makes the test cleaner[1].
Note that we have `OutputAnalyzer` for these test cases:
https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/process/OutputAnalyzer.java
-- lots of test use it, and it would be something like just:
Process p = ProcessTools.startProcess(...);
OutputAnalyzer oa = new OutputAnalyzer(p);
oa.shouldNotHaveExitValue(0);
oa.shouldContain("This command is not for general use");
-
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1980434240
Re: RFR: 8325567: jspawnhelper without args fails with segfault
On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan wrote:
> This change is intended to address the segmentation fault issue that occurs
> when jspawnhelper is called without arguments, and it includes an updated
> test to verify the behavior in such cases.
>
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without
>
>if (argc != 2) {
> shutItDown();
> }
>
> code block
>
>> Test results: failed: 1
>> Report written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >> 1 0 1 0
>> >> <<
>> ==
>> TEST FAILURE
>
>
>
> after added the code block back
>
>if (argc != 2) {
> shutItDown();
> }
>
>
> updated test passes
>
>
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
> \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
>
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
> \\
> -ea \\
> -esa \\
>
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
> \\
> com.sun.javatest.regtest.agent.MainWrapper
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...
The change in jspawnhelper looks good.
I think it would be simpler to have a separate
`jdk/java/lang/ProcessBuilder/JspawnhelperMisuse.java` test that simply invokes
the `jspawnhelper` and verifies the proper message is printed. It would be more
straightforward than trying to fit the _protocol_ test? Plus, we can test 0, 1,
3, 4 args, not only 0 args in that test, and we can also test 2 args with bad
format.
-
PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1916529453
Re: RFR: 8326461: tools/jlink/CheckExecutable.java fail after JDK-8325342
On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan wrote: > Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all > the files in build/linux-x86_64-server-release/images/jdk/bin are executable: > >  > > > After JDK-8325342, all the *.debuginfo files in > build/linux-x86_64-server-release/images/jdk/bin are not executable: > >  > > > This PR only modifies the testcase to adapt to the modification of the > corresponding build script, ignoring the check of debuginfo file executable > permissions, and the risk is low Looks fine to me. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17958#pullrequestreview-1895241663
Re: RFR: 8326370: Remove redundant and misplaced micros from StringBuffers
On Tue, 20 Feb 2024 20:58:50 GMT, Claes Redestad wrote: > Some microbenchmarks in org.openjdk.bench.java.lang.StringBuffers seem > out-of-place (not testing `StringBuffer`), redundant (covered by other tests > like StringSubstring or the various String concatenation tests), or both. > This cleans them out. > > My apologies to Sigurd. Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17937#pullrequestreview-1892606135
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v3]
On Tue, 20 Feb 2024 18:38:48 GMT, Claes Redestad wrote: >> JDK-8282429 accidentally removed an optimization (JDK-8240094) that ensured >> StringBuilder/StringBuffer::toString returns `""` when the builders are >> empty. >> >> >> Name Cnt Base Error Test Error Unit >> Change >> StringBuffers.emptyToString5 12,289 ±0,384 9,883 ± 0,721 ns/op >> 1,24x (p = 0,000*) >> :gc.alloc.rate 1862,398 ± 57,647 0,007 ± 0,000 MB/sec >> 0,00x (p = 0,000*) >> :gc.alloc.rate.norm 24,000 ±0,000 0,000 ± 0,000 B/op >> 0,00x (p = 0,000*) >> :gc.count31,000 0,000 counts >> :gc.time 21,000 ms >> StringBuilders.emptyToString 54,146 ±0,567 0,646 ± 0,003 ns/op >> 6,42x (p = 0,000*) >> :gc.alloc.rate 9208,656 ± 1234,399 0,007 ± 0,000 MB/sec >> 0,00x (p = 0,000*) >> :gc.alloc.rate.norm 40,000 ±0,000 0,000 ± 0,000 B/op >> 0,00x (p = 0,000*) >> :gc.count96,000 0,000 counts >> :gc.time 64,000 ms >> * = significant > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Revert accidental import Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17931#pullrequestreview-1891220541
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 18:04:18 GMT, Claes Redestad wrote:
>> src/java.base/share/classes/java/lang/StringBuilder.java line 478:
>>
>>> 476: }
>>> 477: // Create a copy, don't share the array
>>> 478: return new String(this, null);
>>
>> Ok, this got me a bit confused, but I think this just inlines the call to
>> this constructor:
>>
>>
>> public String(StringBuilder builder) {
>> this(builder, null);
>> }
>
> Yes, I was mostly reaching for increased consistency with `StringBuffer` here.
Good then, thanks.
>> test/micro/org/openjdk/bench/java/lang/StringBuilderToString.java line 33:
>>
>>> 31: import org.openjdk.jmh.annotations.Param;
>>> 32: import org.openjdk.jmh.annotations.Scope;
>>> 33: import org.openjdk.jmh.annotations.Setup;
>>
>> Is this needed?
>
> It's needed again now that I reverted the code removals.. :-)
Mhm. I don't see any new `@Setup` methods anywhere. Just checked out the PR
locally, removed this import and benchmarks still build, and
`StringBuilderToString` bench still runs. Am I missing something here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496297773
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496315384
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String [v2]
On Tue, 20 Feb 2024 18:00:42 GMT, Claes Redestad wrote:
>> test/micro/org/openjdk/bench/java/lang/StringBuffers.java line 49:
>>
>>> 47:
>>> 48: @Benchmark
>>> 49: public String emptyToString() {
>>
>> Do we actually need to remove the `substring` test here?
>
> I couldn't figure out why we'd want to have `String::substring` micros in a
> test `StringBuffers` (there's also a `StringSubstring` micro), though I could
> deal with that as an explicit cleanup RFE.
Yeah, I would like to keep this change clean for backports. Do the cleanup in a
separate PR?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496277852
Re: RFR: 8325730: StringBuilder.toString allocation for the empty String
On Tue, 20 Feb 2024 16:32:54 GMT, Claes Redestad wrote:
> JDK-8282429 accidentally removed an optimization (JDK-8240094) that ensured
> StringBuilder/StringBuffer::toString returns `""` when the builders are empty.
>
>
> Name Cnt Base Error Test Error Unit
> Change
> StringBuffers.emptyToString5 12,289 ±0,384 9,883 ± 0,721 ns/op
> 1,24x (p = 0,000*)
> :gc.alloc.rate 1862,398 ± 57,647 0,007 ± 0,000 MB/sec
> 0,00x (p = 0,000*)
> :gc.alloc.rate.norm 24,000 ±0,000 0,000 ± 0,000 B/op
> 0,00x (p = 0,000*)
> :gc.count31,000 0,000 counts
> :gc.time 21,000 ms
> StringBuilders.emptyToString 54,146 ±0,567 0,646 ± 0,003 ns/op
> 6,42x (p = 0,000*)
> :gc.alloc.rate 9208,656 ± 1234,399 0,007 ± 0,000 MB/sec
> 0,00x (p = 0,000*)
> :gc.alloc.rate.norm 40,000 ±0,000 0,000 ± 0,000 B/op
> 0,00x (p = 0,000*)
> :gc.count96,000 0,000 counts
> :gc.time 64,000 ms
> * = significant
OK, so before [JDK-8282429](https://bugs.openjdk.org/browse/JDK-8282429) this
was handled by `String{Latin1|UTF16}.newString`, gotcha.
src/java.base/share/classes/java/lang/StringBuilder.java line 478:
> 476: }
> 477: // Create a copy, don't share the array
> 478: return new String(this, null);
Ok, this got me a bit confused, but I think this just inlines the call to this
constructor:
public String(StringBuilder builder) {
this(builder, null);
}
test/micro/org/openjdk/bench/java/lang/StringBuffers.java line 49:
> 47:
> 48: @Benchmark
> 49: public String emptyToString() {
Do we actually need to remove the `substring` test here?
test/micro/org/openjdk/bench/java/lang/StringBuilderToString.java line 33:
> 31: import org.openjdk.jmh.annotations.Param;
> 32: import org.openjdk.jmh.annotations.Scope;
> 33: import org.openjdk.jmh.annotations.Setup;
Is this needed?
test/micro/org/openjdk/bench/java/lang/StringBuilders.java line 407:
> 405:
> 406: private void generateData() {
> 407: sb = new StringBuilder(length);
This looks weird, given there is a `sb` initialization two lines below. Is this
`sbEmpty`, really?
-
PR Review: https://git.openjdk.org/jdk/pull/17931#pullrequestreview-1890982087
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496181978
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496178639
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496177666
PR Review Comment: https://git.openjdk.org/jdk/pull/17931#discussion_r1496182725
Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]
On Tue, 13 Feb 2024 18:15:59 GMT, Dan Lutker wrote: >> Just ignore the binary name and only check for argv1 is imho enough. > > I couldn't think if a better way to detect what the host system had and > adding this seemed inline with the busybox check. > > The choice of running `sleep` seems like it was arbitrary and the test should > use something that is under JDK control rather than a platform specific tool > that can change from distro to distro. Is there any reason we don't use the > `java`, something else in the image, or even something built as part of the > test-image. To be fair, this looks similar to what `isMusl()` does: searching for strings in outputs. But I do wonder if `--help` would include "sleep" for some innocuous description somewhere, and this code would accidentally match. @lutkerd, what's the output for `coreutils --help` for a single executable binary? - PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488374538
Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary
On Fri, 9 Feb 2024 23:40:00 GMT, Dan Lutker wrote:
> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils
> package and no coreutils executable as well as AmazonLinux 2023 that uses
> `--enable-single-binary`
test/jdk/java/lang/ProcessHandle/InfoTest.java line 321:
> 319: String[] args = info.arguments().get();
> 320: if (args.length > 0) {
> 321: if (timeIsLastParam) {
I gotta ask: is it true that time argument is the last param in _all_ testing
modes? If so, we don't need `timeIsLastParam` at all?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1487540871
Re: [jdk22] RFR: 8324858: [vectorapi] Bounds checking issues when accessing memory segments
On Tue, 6 Feb 2024 16:50:10 GMT, Paul Sandoz wrote: > This pull request contains a backport of commit > [1ae85138](https://github.com/openjdk/jdk/commit/1ae851387f881263ccc6aeace5afdd0f49d41d33) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Paul Sandoz on 2 Feb 2024 and was > reviewed by Maurizio Cimadamore and Jatin Bhateja. Looks fine. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/109#pullrequestreview-1867277615
Re: RFR: JDK-8324930: java/lang/StringBuilder problem with concurrent jtreg runs
On Tue, 30 Jan 2024 09:08:28 GMT, Matthias Baesken wrote: > On some Windows machines we see sometimes OOM errors because of high resource > (memory/swap) consumption. This is especially seen when the jtreg runs have > higher concurrency. A solution is to put the java/lang/StringBuilder tests in > the exclusiveAccess.dirs group so that they are not executed concurrently, > which helps to mitigate the resource shortages. > Of course this has the downside that on very large machines the concurrent > execution is not done any more. Can we maybe see if we can fix these tests without exclusive-accessing them? I find it surprising that `java/lang/StringBuilder` tests are problematic, but `java/lang/StringBuffer` tests are not. Which tests fail? - PR Review: https://git.openjdk.org/jdk/pull/17625#pullrequestreview-1851921699
Integrated: 8323717: Introduce test keyword for tests that need external dependencies
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev wrote: > Some jtreg tests require resolvable external dependencies. This resolution is > delegated to JIB, which is not used in vanilla OpenJDK testing. It would be > convenient to add a keyword that marks tests that require these external > dependencies, so that we could exclude those tests from runs. This would > allow us to: a) run all tests in hotspot:tier4, which now excludes > `applications/` specifically; b) make all tests runs (#17422) cleaner on many > environments. > > I provisionally call this flag `external-dep`, but I am open for other > suggestions. > > Note that some tests that pull `@Artifact`-s provide special paths that do > limited testing anyway. However, there are tests which cannot run without > external dependencies at all. These include at least `applications/jcstress` > and `applications/scimark` tests. > > Ironically, I cannot run the jcstress test generator because the dependencies > are lacking here. I regenerated those test using a self-built jcstress 0.16 > bundle. > > Additional testing: > - [x] `make test TEST=applications/` fails > - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, > skipping most of the tests This pull request has now been integrated. Changeset: 12b89cd2 Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/12b89cd2eeb5c2c43a2ce425c96fc4f718e30514 Stats: 62 lines in 32 files changed: 32 ins; 0 del; 30 mod 8323717: Introduce test keyword for tests that need external dependencies Reviewed-by: dholmes, lmesnik - PR: https://git.openjdk.org/jdk/pull/17421
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev wrote: > Some jtreg tests require resolvable external dependencies. This resolution is > delegated to JIB, which is not used in vanilla OpenJDK testing. It would be > convenient to add a keyword that marks tests that require these external > dependencies, so that we could exclude those tests from runs. This would > allow us to: a) run all tests in hotspot:tier4, which now excludes > `applications/` specifically; b) make all tests runs (#17422) cleaner on many > environments. > > I provisionally call this flag `external-dep`, but I am open for other > suggestions. > > Note that some tests that pull `@Artifact`-s provide special paths that do > limited testing anyway. However, there are tests which cannot run without > external dependencies at all. These include at least `applications/jcstress` > and `applications/scimark` tests. > > Ironically, I cannot run the jcstress test generator because the dependencies > are lacking here. I regenerated those test using a self-built jcstress 0.16 > bundle. > > Additional testing: > - [x] `make test TEST=applications/` fails > - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, > skipping most of the tests Awesome, thanks. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1910723514
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Thu, 25 Jan 2024 14:48:16 GMT, Maurizio Cimadamore
wrote:
> I don't 100% buy the `MethodHandleImpl` analogy. In that case the check is
> not simply used to save a branch, but to spare spinning of a completely new
> lambda form.
Doing this to save a few intructions would not likely to worth the hassle
outside the _really performance critical paths_, but even then it might be
useful for hot JDK code. On larger examples, you can avoid memory accesses,
allocations, etc. by coding up the constant-foldable path that you know
compiler would not be able to extract when propagating constants through the
generic code. For example, giving quantitative substance to my previous example:
diff --git a/src/java.base/share/classes/java/lang/Integer.java
b/src/java.base/share/classes/java/lang/Integer.java
index 1c5b3c414ba..d50748c369e 100644
--- a/src/java.base/share/classes/java/lang/Integer.java
+++ b/src/java.base/share/classes/java/lang/Integer.java
@@ -28,4 +28,5 @@
import jdk.internal.misc.CDS;
import jdk.internal.misc.VM;
+import jdk.internal.vm.ConstantSupport;
import jdk.internal.vm.annotation.ForceInline;
import jdk.internal.vm.annotation.IntrinsicCandidate;
@@ -416,4 +417,7 @@ private static void formatUnsignedIntUTF16(int val, int
shift, byte[] buf, int l
}
+@Stable
+static final String[] TO_STRINGS = { "-1", "0", "1" };
+
/**
* Returns a {@code String} object representing the
@@ -428,4 +432,8 @@ private static void formatUnsignedIntUTF16(int val, int
shift, byte[] buf, int l
@IntrinsicCandidate
public static String toString(int i) {
+if (ConstantSupport.isCompileConstant(i) &&
+(i >= -1) && (i <= 1)) {
+return TO_STRINGS[i + 1];
+}
int size = stringSize(i);
if (COMPACT_STRINGS) {
diff --git a/test/micro/org/openjdk/bench/java/lang/Integers.java
b/test/micro/org/openjdk/bench/java/lang/Integers.java
index 43ceb5d18d2..28248593a73 100644
--- a/test/micro/org/openjdk/bench/java/lang/Integers.java
+++ b/test/micro/org/openjdk/bench/java/lang/Integers.java
@@ -91,4 +91,18 @@ public void decode(Blackhole bh) {
}
+@Benchmark
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+public String toStringConstYay() {
+return Integer.toString(0);
+}
+
+int v = 0;
+
+@Benchmark
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+public String toStringConstNope() {
+return Integer.toString(v);
+}
+
/** Performs toString on small values, just a couple of digits. */
@Benchmark
Benchmark (size) Mode Cnt Score
Error Units
Integers.toStringConstNope500 avgt 15 3,599 ?
0,034 ns/op
Integers.toStringConstNope:gc.alloc.rate.norm 500 avgt 15 48,000 ?
0,001B/op
Integers.toStringConstNope:gc.time500 avgt 15223,000
ms
Integers.toStringConstYay 500 avgt 15 0,568 ?
0,046 ns/op
Integers.toStringConstYay:gc.alloc.rate.norm 500 avgt 15 ? 10??
B/op
Think about it as simplifying/avoiding the need for full compiler intrinsics. I
could, in principle, do this by intrinsifying `Integer.toString` completely,
check the same `isCon`, and then either construct the access to some String
constant, or arrange the call to actual toString slow path. That would not be
as simple as doing the similar thing in plain Java, with just a little of
compiler support in form of `ConstantSupport`.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1910449450
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Wed, 24 Jan 2024 21:28:29 GMT, Leonid Mesnik wrote: >> Some jtreg tests require resolvable external dependencies. This resolution >> is delegated to JIB, which is not used in vanilla OpenJDK testing. It would >> be convenient to add a keyword that marks tests that require these external >> dependencies, so that we could exclude those tests from runs. This would >> allow us to: a) run all tests in hotspot:tier4, which now excludes >> `applications/` specifically; b) make all tests runs (#17422) cleaner on >> many environments. >> >> I provisionally call this flag `external-dep`, but I am open for other >> suggestions. >> >> Note that some tests that pull `@Artifact`-s provide special paths that do >> limited testing anyway. However, there are tests which cannot run without >> external dependencies at all. These include at least `applications/jcstress` >> and `applications/scimark` tests. >> >> Ironically, I cannot run the jcstress test generator because the >> dependencies are lacking here. I regenerated those test using a self-built >> jcstress 0.16 bundle. >> >> Additional testing: >> - [x] `make test TEST=applications/` fails >> - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, >> skipping most of the tests > > Marked as reviewed by lmesnik (Reviewer). @lmesnik, you good with the keyword name? - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1909740587
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Wed, 24 Jan 2024 18:51:27 GMT, Maurizio Cimadamore
wrote:
> Naive question: the right way to use this would be almost invariably be like
> this:
>
> ```
> if (isCompileConstant(foo) && fooHasCertainStaticProperties(foo)) {
> // fast-path
> }
> // slow path
> ```
>
> Right?
Yes, I think so.
> Then the expectation is that during interpreter and C1, `isCompileConstant`
> always returns false, so we just never take the fast path (but we probably
> still pay for the branch, right?).
Yes, I think so. For C1, we would still prune the "dead" path, because C1 is
able to know that `if (false)` is never taken. We do pay with the branch and
the method call in interpreter. (There are ways to special-case these
intrinsics for interpreter too, if we choose to care.)
> And, when we get to C2 and this method is inlined, at this point we know that
> either `foo` is constant or not. If it is constant we can check other
> conditions on foo (which presumably is cheap because `foo` is constant) and
> maybe take the fast-path. In both cases, there's no branch in the generated
> code because we know "statically" when inlining if `foo` has the right shape
> or not. Correct?
Yes. I think the major use would be using `constexpr`-like code on "const"
path, so that the entire "const" branch constant-folds completely. In [my
experiments](https://github.com/openjdk/jdk/pull/17527#issuecomment-1906379544)
with `Integer.toString` that certainly happens. But that is not a requirement,
and we could probably still reap some benefits from partial constant folds; but
at that point we would need to prove that a "partially const" path is better
than generic "non-const" path under the same conditions.
I agree it would be convenient to put some examples in javadoc.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1908736651
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v6]
On Wed, 24 Jan 2024 10:33:05 GMT, Quan Anh Mai wrote: >> Hi, >> >> This patch introduces `JitCompiler::isConstantExpression` which can be used >> to statically determine whether an expression has been constant-folded by >> the Jit compiler, leading to more constant-folding opportunities. For >> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to >> eliminate the lifetime check on global sessions without imposing additional >> branches on other non-global sessions. This is similar to >> `__builtin_constant_p` in GCC and clang. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > address reviews src/java.base/share/classes/jdk/internal/vm/ConstantSupport.java line 32: > 30: /** > 31: * Just-in-time-compiler-related queries > 32: */ This looks like a stale comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465397036
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Wed, 24 Jan 2024 07:15:12 GMT, Quan Anh Mai wrote: >> This seems really weird to me for Java code. The method doesn't get the >> original "expression" it only gets the value of that expression after it has >> been evaluated. Is there some kind of weird "magic" happening here? > > @dholmes-ora Indeed it's a compiler magic, albeit not really weird. While the > method execution only receives the evaluated value of `expr`, the method > compilation has the expression in its original form. As a result, it can > determine the result based on this information. It is still weird to talk about expressions at this level. We really check if the value is constant, like the method name suggests now. Yes, this implicitly tests that the expression that produced that value is fully constant-folded. But that's a detail that we do not need to capture here. Let's rename `expr` -> `val`, and tighten up the javadoc for the method to mention we only test the constness of the final value. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1465401456
Re: RFR: 8324647: Invalid test group of lib-test after JDK-8323515
On Wed, 24 Jan 2024 15:20:37 GMT, Jie Fu wrote: > This patch tries to fix the invalid test group definition of lib-test. > Please review. > Thanks. Ooof. I wonder how that happened. This would not show up before we try to run the actual libtest tests. What is extra wild is that GHA reports green, even though the logs say that TEST.groups is incorrect! I am going to go and fix that... The fix looks good and trivial. Thanks for catching it! - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17558#pullrequestreview-1841971948
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:49:49 GMT, Quan Anh Mai wrote:
>> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 32:
>>
>>> 30: * Just-in-time-compiler-related queries
>>> 31: */
>>> 32: public class JitCompiler {
>>
>> An alternative name and location is `jdk.internal.vm.ConstantSupport` with
>> initial class doc:
>>
>> Defines methods to test if a value has been evaluated to a compile-time
>> constant value by the HotSpot VM.
>
> That sounds like a better name for the class, although I think
> `jdk.internal.misc` is more suitable than `jdk.internal.vm`. Do you have any
> preference? Thanks.
+1 to `ConstantSupport`. I think `jdk.internal.vm` is a proper place for it.
There is adjacent `jdk.internal.vm.vector.VectorSupport`, and whole
`jdk.internal.vm.annotations` package is there too.
`jdk.internal.misc` sounds like a place for utility classes. `Unsafe` is a
historical exception, I think.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464551793
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 22:41:44 GMT, Quan Anh Mai wrote: >> src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119: >> >>> 117: * @see #isCompileConstant(boolean) >>> 118: */ >>> 119: @IntrinsicCandidate >> >> Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. >> These methods should have `@Hidden` too then? Probably applies to other >> entries too. > > I don't understand why this needs to be `@Hidden`, the javadoc says that a > function annotated with `@Hidden` is omitted from the stacktraces. This > function does not call anything so what is the point of hiding it? I suspect there is a code that counts stack traces somewhere that relies on it in MH parts. There is no harm for doing `@Hidden` here, I think. - PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1464541674
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 16:01:05 GMT, Aleksey Shipilev wrote:
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
>> Would it be possible to list further examples where this might be used?
>> Asking because I'm wondering about the usability and maintainability of
>> if-then-else code.
>
> A similar thing is already used in JDK:
> https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
>
> Extending this for more common use allows doing things like optimizing
> `Integer.toString(int)`:
>
>
> @Stable
> static final String[] CONST_STRINGS = {"-1", "0", "1"};
>
> @IntrinsicCandidate
> public static String toString(int i) {
> if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
> return CONST_STRINGS[i + 1];
> }
> ...
>
>
> Note how this code would fold away to one of the paths, depending on whether
> the compiler knows it is a constant or not. Generated-code-wise it is a
> zero-cost thing :)
> @shipilev Thanks a lot for the detailed reviews and suggestions, I hope I
> have addressed all of them.
Sure thing, I just effectively merged my draft implementation into yours :)
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906602556
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v5]
On Tue, 23 Jan 2024 17:21:47 GMT, Quan Anh Mai wrote:
>> Hi,
>>
>> This patch introduces `JitCompiler::isConstantExpression` which can be used
>> to statically determine whether an expression has been constant-folded by
>> the Jit compiler, leading to more constant-folding opportunities. For
>> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to
>> eliminate the lifetime check on global sessions without imposing additional
>> branches on other non-global sessions. This is similar to
>> `__builtin_constant_p` in GCC and clang.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Quan Anh Mai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> ident
A few more stylistic comments :)
Still thinking the better home for these might be just
`jdk.internal.misc.VM`... But I would not insist, if others are happy.
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 56:
> 54: */
> 55: @IntrinsicCandidate
> 56: public static boolean isCompileConstant(boolean expr) {
Here and in other places: probably not `expr`, but just `val` or something?
src/java.base/share/classes/jdk/internal/misc/JitCompiler.java line 119:
> 117: * @see #isCompileConstant(boolean)
> 118: */
> 119: @IntrinsicCandidate
Note how the Java entry for MH intrinsic we have replaced had `@Hidden`. These
methods should have `@Hidden` too then? Probably applies to other entries too.
-
PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839475907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463705907
PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463703771
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>>> jtreg:test/hotspot/jtreg:all 6731 670229 0
>>>> <<
>>>> jtreg:test/jdk:all 9962 995111 0
>>>> <<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Thank you all!
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1906520760
Integrated: 8323515: Create test alias "all" for all test roots
On Mon, 15 Jan 2024 11:05:09 GMT, Aleksey Shipilev wrote:
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
This pull request has now been integrated.
Changeset: 8b9bf758
Author:Aleksey Shipilev
URL:
https://git.openjdk.org/jdk/commit/8b9bf758801400e4491326cd4c90fc117b9d97e1
Stats: 49 lines in 5 files changed: 42 ins; 5 del; 2 mod
8323515: Create test alias "all" for all test roots
Reviewed-by: dholmes, alanb, joehw, lmesnik
-
PR: https://git.openjdk.org/jdk/pull/17422
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 15:52:29 GMT, Alan Bateman wrote:
> Would it be possible to list further examples where this might be used?
> Asking because I'm wondering about the usability and maintainability of
> if-then-else code.
A similar thing is already used in JDK:
https://github.com/openjdk/jdk/blob/2a01c798d346656a0ee3553c0964feab75b5dfb6/src/java.base/share/classes/java/lang/invoke/Invokers.java#L622-L624
Extending this for more common use allows doing things like optimizing
`Integer.toString(int)`:
@Stable
static final String[] CONST_STRINGS = {"-1", "0", "1"};
@IntrinsicCandidate
public static String toString(int i) {
if (isCompileConstant(i) && (i >= -1) && (i <= 1)) {
return CONST_STRINGS[i + 1];
}
...
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906379544
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 GMT, Quan Anh Mai wrote: >> Hi, >> >> This patch introduces `JitCompiler::isConstantExpression` which can be used >> to statically determine whether an expression has been constant-folded by >> the Jit compiler, leading to more constant-folding opportunities. For >> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to >> eliminate the lifetime check on global sessions without imposing additional >> branches on other non-global sessions. This is inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads All right, this is very close :) I now have stylistic comments: src/hotspot/share/classfile/vmIntrinsics.hpp line 912: > 910: do_intrinsic(_getAndSetInt, jdk_internal_misc_Unsafe, > getAndSetInt_name, getAndSetInt_signature, F_R) \ > 911:do_name( getAndSetInt_name, > "getAndSetInt") \ > 912:do_alias(getAndSetInt_signature, > /*"(Ljava/lang/Object;JI)I"*/ getAndAddInt_signature) \ I don't think we need to do these formatting changes in this PR. src/hotspot/share/classfile/vmIntrinsics.hpp line 927: > 925: > \ > 926: do_class(jdk_internal_misc_JitCompiler, > "jdk/internal/misc/JitCompiler") > \ > 927: do_intrinsic(_isConstantExpressionZ, > jdk_internal_misc_JitCompiler,isConstantExpression_name, bool_bool_signature, > F_S) \ It would be cleaner to follow the current naming for existing intrinsic: do_intrinsic(_isCompileConstant, java_lang_invoke_MethodHandleImpl, isCompileConstant_name, isCompileConstant_signature, F_S) \ do_name( isCompileConstant_name, "isCompileConstant") \ do_alias(isCompileConstant_signature, object_boolean_signature) \ I.e. rename `isConstantExpression` -> `isCompileConstant`. It clearly communicates that we are not dealing with expressions as arguments, and that we underline this is the (JIT) _compile_ constant, not just a constant expression from JLS 15.28 "Constant Expressions". Maybe even replace that `MHImpl` method with the new intrinsic. src/hotspot/share/opto/c2compiler.cpp line 727: > 725: case vmIntrinsics::_storeStoreFence: > 726: case vmIntrinsics::_fullFence: > 727: case vmIntrinsics::_isConstantExpressionZ: Move this closer to `vmIntrinsics::_isCompileConstant:`, if not outright replace it? src/hotspot/share/opto/library_call.hpp line 2: > 1: /* > 2: * Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights > reserved. Unnecessary update? - PR Review: https://git.openjdk.org/jdk/pull/17527#pullrequestreview-1839148507 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463490470 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463493124 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497227 PR Review Comment: https://git.openjdk.org/jdk/pull/17527#discussion_r1463497518
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>>> jtreg:test/hotspot/jtreg:all 6731 670229 0
>>>> <<
>>>> jtreg:test/jdk:all 9962 995111 0
>>>> <<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
All right, thanks! @lmesnik, I realized I forgot to ask if you had objections
to this.
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1906338893
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Tue, 23 Jan 2024 14:42:20 GMT, Roger Riggs wrote: > Is there any place to document the new keyword or its usage; it does not seem > very discoverable just existing in the TEST.ROOT and some tests. I don't think there is a place to describe keywords, except in the relevant `TEST.ROOT`-s. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1906324934
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Tue, 23 Jan 2024 01:32:45 GMT, David Holmes wrote: > Seems quite reasonable. Thanks! I shall wait for more reviewers, in case someone has an issue with `external-dep` as the flag name. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1905719123
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 09:31:51 GMT, Quan Anh Mai wrote: > Maybe I am ignorant but doesn't the definition of an intrinsics contain the > signature of the method as well? The definitions in `vmIntrinsics`, sure, they require full signature for `@IntrinsicCandidate` methods. It would yield some unfortunate duplication. But after that, we can map on the same `inline_isCompileConstant` intrinsic that just asks `arg(0)->is_Con()`, and it would not care about the type of the constant. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905667006
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:16:07 GMT, Aleksey Shipilev wrote:
>> Hi,
>>
>> This patch introduces `JitCompiler::isConstantExpression` which can be used
>> to statically determine whether an expression has been constant-folded by
>> the Jit compiler, leading to more constant-folding opportunities. For
>> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to
>> eliminate the lifetime check on global sessions without imposing additional
>> branches on other non-global sessions. This is inspired by
>> `std::is_constant_evaluated` in C++.
>>
>> Please kindly give your opinion as well as your reviews, thanks very much.
>
> Nice. I had a similar thing stashed in my todo queue. Note that there is
> already `isCompileConstant` that does similar thing:
> https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
> -- maybe we should just expose that more widely. I would suggest we just do
> the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
> to that intrinsic.
> @shipilev Thanks a lot for your suggestions. Yes I can just use
> `inline_isCompileConstant` instead.
>
> Regarding the place of the method, I'm not really sure as putting in
> `java.lang.Long` seems out-of-place for an internal mechanism that is
> obviously not only used in `java.lang`, which will force a new entry in
> `JavaLangAccess`.
Ah yes, if you need to use it across module boundaries, putting the
private/protected method would require `JavaLangAccess`, which is burdensome. I
am just icky about introducing a whole new internal class for this. Is there
anything in current `jdk.internal.vm.*` that fits it? Maybe `misc.Unsafe` or
`misc.VM`?
> Finally, I think accepting a `long` would be enough (maybe `double`, too?)
> since `int`, `boolean` etc can be converted losslessly to `long`.
Right, that would work for primitives, since we could probably rely on
conversion for constants to be folded. But I also see the value for asking
`isCompileConstant(Object)`, which is not easily convertible. So I would just
do the overloads for all primitives and `Object`. The C2 intrinsic would not
care about the `arg(0)` type, it would reply `isCon` on those constants just
the same.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905655407
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler
On Tue, 23 Jan 2024 08:10:54 GMT, Quan Anh Mai wrote:
> Hi,
>
> This patch introduces `JitCompiler::isConstantExpression` which can be used
> to statically determine whether an expression has been constant-folded by the
> Jit compiler, leading to more constant-folding opportunities. For example, it
> can be used in `MemorySessionImpl::checkValidStateRaw` to eliminate the
> lifetime check on global sessions without imposing additional branches on
> other non-global sessions. This is inspired by `std::is_constant_evaluated`
> in C++.
>
> Please kindly give your opinion as well as your reviews, thanks very much.
Nice. I had a similar thing stashed in my todo queue. Note that there is
already `isCompileConstant` that does similar thing:
https://github.com/openjdk/jdk/blob/5a74c2a67ebcb47e51732f03c4be694bdf920469/src/hotspot/share/opto/library_call.cpp#L8189-L8193
-- maybe we should just expose that more widely. I would suggest we just do
the private `java.lang.{Integer,...}.isCompileConstant` methods and bind them
to that intrinsic.
-
PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1905504206
Re: RFR: 8323717: Introduce test keyword for tests that need external dependencies
On Mon, 15 Jan 2024 10:48:23 GMT, Aleksey Shipilev wrote: > Some jtreg tests require resolvable external dependencies. This resolution is > delegated to JIB, which is not used in vanilla OpenJDK testing. It would be > convenient to add a keyword that marks tests that require these external > dependencies, so that we could exclude those tests from runs. This would > allow us to: a) run all tests in hotspot:tier4, which now excludes > `applications/` specifically; b) make all tests runs (#17422) cleaner on many > environments. > > I provisionally call this flag `external-dep`, but I am open for other > suggestions. > > Note that some tests that pull `@Artifact`-s provide special paths that do > limited testing anyway. However, there are tests which cannot run without > external dependencies at all. These include at least `applications/jcstress` > and `applications/scimark` tests. > > Ironically, I cannot run the jcstress test generator because the dependencies > are lacking here. I regenerated those test using a self-built jcstress 0.16 > bundle. > > Additional testing: > - [x] `make test TEST=applications/` fails > - [x] `JTREG_KEYWORDS=!external-dep make test TEST=applications/` passes, > skipping most of the tests Any takers? Maybe the audience should include core-libs too. - PR Comment: https://git.openjdk.org/jdk/pull/17421#issuecomment-1903486053
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 09:01:35 GMT, Aleksey Shipilev wrote:
>> Since recent work to improve `tier4` performance, we actually test
>> `tier{1,2,3,4}` often, which includes all the tests in current tree. It
>> would be more convenient to just have the `all` test group/alias, so that we
>> can do `make test TEST=all`. This also gives a parallelism / run time
>> benefit, as we do not wait for tests in each tier to complete before moving
>> to next tier.
>>
>> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
>> environments one also needs to supply a few keywords like `!printer` to skip
>> tests that cannot complete without failure due to misconfiguration. I left
>> the keywords as is to show how would a failing run look. There is also an
>> existing shortcut in build system that allows to run this with `make
>> test-all`.
>>
>>
>> % make test TEST=all
>>
>> Test selection 'all', will run:
>> * jtreg:test/hotspot/jtreg:all
>> * jtreg:test/jdk:all
>> * jtreg:test/langtools:all
>> * jtreg:test/jaxp:all
>> * jtreg:test/lib-test:all
>>
>> (...about 6 hours later...)
>>
>> ==
>> Test summary
>> ==
>>TEST TOTAL PASS FAIL ERROR
>>
>>>> jtreg:test/hotspot/jtreg:all 6731 670229 0
>>>> <<
>>>> jtreg:test/jdk:all 9962 995111 0
>>>> <<
>>jtreg:test/langtools:all 4469 4469 0 0
>>
>>jtreg:test/jaxp:all 513 513 0 0
>>
>>jtreg:test/lib-test:all 3232 0 0
>>
>> ==
>> TEST FAILURE
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Catch-all -> All tests
Any other reviews needed for this? Nominally, this changes the test groups in
langtools, so maybe @lahodaj or @biboudis want to take a look. For jaxp,
@JoeWang-Java, maybe?
-
PR Comment: https://git.openjdk.org/jdk/pull/17422#issuecomment-1895680732
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
On Tue, 16 Jan 2024 08:52:03 GMT, Alan Bateman wrote: >> Tried not to introduce new `*_all` groups here. `jdk_all` would be the same >> as `jdk:all`, TBH. But we still can do it for symmetry reasons, see new >> commit. > > "all" looks okay but the comment "Catch-all" suggests something else, > shouldn't be "All tests"? Yeah, we can do "All tests" instead. See new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1453113607
Re: RFR: 8323515: Create test alias "all" for all test roots [v3]
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Catch-all -> All tests
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17422/files
- new: https://git.openjdk.org/jdk/pull/17422/files/78f5f9bd..def2f39b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17422=02
- incr: https://webrevs.openjdk.org/?repo=jdk=17422=01-02
Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/17422.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17422/head:pull/17422
PR: https://git.openjdk.org/jdk/pull/17422
Re: RFR: 8323515: Create test alias "all" for all test roots [v2]
On Mon, 15 Jan 2024 22:37:36 GMT, David Holmes wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> jdk_all and lib_test_all groups > > test/jdk/TEST.groups line 28: > >> 26: # >> 27: >> 28: all = \ > > Why no `jdk_all` definition in this case? Tried not to introduce new `*_all` groups here. `jdk_all` would be the same as `jdk:all`, TBH. But we still can do it for symmetry reasons, see new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/17422#discussion_r1453098855
Re: RFR: 8323515: Create test alias "all" for all test roots [v2]
> Since recent work to improve `tier4` performance, we actually test
> `tier{1,2,3,4}` often, which includes all the tests in current tree. It would
> be more convenient to just have the `all` test group/alias, so that we can do
> `make test TEST=all`. This also gives a parallelism / run time benefit, as we
> do not wait for tests in each tier to complete before moving to next tier.
>
> Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
> environments one also needs to supply a few keywords like `!printer` to skip
> tests that cannot complete without failure due to misconfiguration. I left
> the keywords as is to show how would a failing run look. There is also an
> existing shortcut in build system that allows to run this with `make
> test-all`.
>
>
> % make test TEST=all
>
> Test selection 'all', will run:
> * jtreg:test/hotspot/jtreg:all
> * jtreg:test/jdk:all
> * jtreg:test/langtools:all
> * jtreg:test/jaxp:all
> * jtreg:test/lib-test:all
>
> (...about 6 hours later...)
>
> ==
> Test summary
> ==
>TEST TOTAL PASS FAIL ERROR
>
>>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>>> jtreg:test/jdk:all 9962 995111 0 <<
>jtreg:test/langtools:all 4469 4469 0 0
>
>jtreg:test/jaxp:all 513 513 0 0
>
>jtreg:test/lib-test:all 3232 0 0
>
> ==
> TEST FAILURE
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
jdk_all and lib_test_all groups
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17422/files
- new: https://git.openjdk.org/jdk/pull/17422/files/7f6797b6..78f5f9bd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17422=01
- incr: https://webrevs.openjdk.org/?repo=jdk=17422=00-01
Stats: 8 lines in 2 files changed: 8 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/17422.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17422/head:pull/17422
PR: https://git.openjdk.org/jdk/pull/17422
RFR: 8323515: Create test alias "all" for all test roots
Since recent work to improve `tier4` performance, we actually test
`tier{1,2,3,4}` often, which includes all the tests in current tree. It would
be more convenient to just have the `all` test group/alias, so that we can do
`make test TEST=all`. This also gives a parallelism / run time benefit, as we
do not wait for tests in each tier to complete before moving to next tier.
Sample run on out-of-the-box Linux x86_64 fastdebug is below. For some
environments one also needs to supply a few keywords like `!printer` to skip
tests that cannot complete without failure due to misconfiguration. I left the
keywords as is to show how would a failing run look. There is also an existing
shortcut in build system that allows to run this with `make test-all`.
% make test TEST=all
Test selection 'all', will run:
* jtreg:test/hotspot/jtreg:all
* jtreg:test/jdk:all
* jtreg:test/langtools:all
* jtreg:test/jaxp:all
* jtreg:test/lib-test:all
(...about 6 hours later...)
==
Test summary
==
TEST TOTAL PASS FAIL ERROR
>> jtreg:test/hotspot/jtreg:all 6731 670229 0 <<
>> jtreg:test/jdk:all 9962 995111 0 <<
jtreg:test/langtools:all 4469 4469 0 0
jtreg:test/jaxp:all 513 513 0 0
jtreg:test/lib-test:all 3232 0 0
==
TEST FAILURE
-
Commit messages:
- Fix
Changes: https://git.openjdk.org/jdk/pull/17422/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17422=00
Issue: https://bugs.openjdk.org/browse/JDK-8323515
Stats: 41 lines in 5 files changed: 34 ins; 5 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17422.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17422/head:pull/17422
PR: https://git.openjdk.org/jdk/pull/17422
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Fri, 12 Jan 2024 07:33:07 GMT, Sergey Bylokhov wrote: > Just curious if this was found by inspection or when debugging some issue > with LDAP authentication? Asking on whether it is feasible or not to have > more tests in this area. No need, that one is an easy target for static analyzers. This bug was found by one :) - PR Comment: https://git.openjdk.org/jdk/pull/17365#issuecomment-1888946629
Re: RFR: 8323562: SaslInputStream.read() may return wrong value
On Thu, 11 Jan 2024 06:28:51 GMT, Sergey Bylokhov wrote: > SaslInputStream.read() should return a value in the range from 0 to 255 per > the spec of InputStream.read() but it returns the signed byte from the inBuf > as is. Looks fine. I think the common style is to use capitalized `0xFF`, but that one is not enforced consistently. This will do as well. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17365#pullrequestreview-1818007975
Re: RFR: 8323508: Remove TestGCLockerWithShenandoah.java line from TEST.groups
On Wed, 10 Jan 2024 12:09:07 GMT, Stefan Karlsson wrote: > TestGCLockerWithShenandoah.java was recently removed, but the TEST.groups > file still has a reference to it. This causes problems in our CI pipeline. Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17344#pullrequestreview-1813120038
Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing [v2]
On Wed, 3 Jan 2024 21:29:57 GMT, Joshua Cao wrote: >> ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` -> >> `transfer()`. When coming from the copy constructor, the Map is empty, so >> there is nothing to transfer. But `transfer()` will still copy all the empty >> nodes from the old table to the new table. >> >> This patch avoids this work by only calling `tryPresize()` if the table is >> already initialized. If `table` is null, the initialization is deferred to >> `putVal()`, which calls `initTable()`. >> >> --- >> >> ### JMH results for testCopyConstructor >> >> before patch: >> >> >> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": >> 937395.686 ±(99.9%) 99074.324 ns/op [Average] >> (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184 >> CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution) >> >> >> after patch: >> >> >> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": >> 620871.469 ±(99.9%) 59195.406 ns/op [Average] >> (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419 >> CI (99.9%): [561676.063, 680066.875] (assumes normal distribution) >> >> >> Average time is decreased by about 33%. > > Joshua Cao has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup benchmark I am good this this version. I would like a second Reviewer to ack this too. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1803734314
Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing
On Fri, 15 Dec 2023 01:16:55 GMT, Joshua Cao wrote:
> ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` ->
> `transfer()`. When coming from the copy constructor, the Map is empty, so
> there is nothing to transfer. But `transfer()` will still copy all the empty
> nodes from the old table to the new table.
>
> This patch avoids this work by only calling `tryPresize()` if the table is
> already initialized. If `table` is null, the initialization is deferred to
> `putVal()`, which calls `initTable()`.
>
> ---
>
> ### JMH results for testCopyConstructor
>
> before patch:
>
>
> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
> 937395.686 ±(99.9%) 99074.324 ns/op [Average]
> (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184
> CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution)
>
>
> after patch:
>
>
> Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor":
> 620871.469 ±(99.9%) 59195.406 ns/op [Average]
> (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419
> CI (99.9%): [561676.063, 680066.875] (assumes normal distribution)
>
>
> Average time is decreased by about 33%.
All right, good! I have comments about the benchmark:
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java line
851:
> 849: */
> 850: public ConcurrentHashMap(Map m) {
> 851: this(m.size(), LOAD_FACTOR, 1);
This looks like just `this(m.size())`, right? Not sure if we want to save an
additional chained constructor call, though.
test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 83:
> 81: for (int i = 0; i < nkeys; ++i) {
> 82: staticMap.put(rng.next(), rng.next());
> 83: }
Can just merge this loop with the one above, reusing `rng.next()` for both key
and values for `staticMap`.
test/micro/org/openjdk/bench/java/util/concurrent/Maps.java line 122:
> 120: public void testCopyConstructor() {
> 121: ConcurrentHashMap clone = new
> ConcurrentHashMap<>(staticMap);
> 122: dummy = clone;
Is this for preventing dead-code elimination? If so, just do:
return new ConcurrentHashMap<>(staticMap);
-
PR Review: https://git.openjdk.org/jdk/pull/17116#pullrequestreview-1802240545
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440523638
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440517305
PR Review Comment: https://git.openjdk.org/jdk/pull/17116#discussion_r1440515260
Re: RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing
On Fri, 15 Dec 2023 01:16:55 GMT, Joshua Cao wrote: > ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` -> > `transfer()`. When coming from the copy constructor, the Map is empty, so > there is nothing to transfer. But `transfer()` will still copy all the empty > nodes from the old table to the new table. > > This patch avoids this work by only calling `tryPresize()` if the table is > already initialized. If `table` is null, the initialization is deferred to > `putVal()`, which calls `initTable()`. > > --- > > ### JMH results for testCopyConstructor > > before patch: > > > Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": > 937395.686 ±(99.9%) 99074.324 ns/op [Average] > (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184 > CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution) > > > after patch: > > > Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": > 620871.469 ±(99.9%) 59195.406 ns/op [Average] > (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419 > CI (99.9%): [561676.063, 680066.875] (assumes normal distribution) > > > Average time is decreased by about 33%. We need @DougLea to take a look :) - PR Comment: https://git.openjdk.org/jdk/pull/17116#issuecomment-1875046493
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final [v2]
On Wed, 6 Dec 2023 17:42:47 GMT, Brett Okken wrote: >> The static AtomicInteger used for the nextHashCode should be final. > > Brett Okken has updated the pull request incrementally with one additional > commit since the last revision: > > Update full name @bokken, you are good to `/integrate`. - PR Comment: https://git.openjdk.org/jdk/pull/16987#issuecomment-1845016772
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final
On Wed, 6 Dec 2023 00:52:48 GMT, Brett Okken wrote: > The static AtomicInteger used for the nextHashCode should be final. Looks okay to me! Since this is new contribution, I would like someone else to take a look. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16987#pullrequestreview-1768229984
Re: RFR: 8321470: ThreadLocal.nextHashCode can be static final
On Wed, 6 Dec 2023 00:52:48 GMT, Brett Okken wrote: > The static AtomicInteger used for the nextHashCode should be final. Submitted: https://bugs.openjdk.org/browse/JDK-8321470 Please change this PR title to "8321470: ThreadLocal.nextHashCode can be static final", and bots would do the rest. - PR Comment: https://git.openjdk.org/jdk/pull/16987#issuecomment-1842976493
Re: ThreadLocal nextHashCode
On 05.12.23 21:02, Brett Okken wrote: Should the private static AtomicInteger nextHashCode also be final? https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/ThreadLocal.java#L100 Yes, I think so. Feel free to submit a PR! -Aleksey Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Re: RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 15:48:11 GMT, Jorn Vernee wrote: > This test is problematic on Zero due to > https://bugs.openjdk.org/browse/JDK-8321064 > > Disable it for now on that platform, until a Zero maintainer has a chance to > look into it. > > Testing: running TestHandshake on zero to see that it is skipped. Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16906#pullrequestreview-1757865043
Re: RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 16:18:42 GMT, Jorn Vernee wrote: > > Hold on, can we figure out if Zero can actually be fixed before we go and > > disable the test? I think we only disable the tests like this if there is > > an intrinsic reason why the test does not apply to a platform. > > I would problem list it, but we can't problem list tests specifically on Zero. Ah, that is unfortunate. Lacking problem-list support, disabling the test with `@requires` is okay. - PR Comment: https://git.openjdk.org/jdk/pull/16906#issuecomment-1834096799
Re: Integrated: 8321127: ProblemList java/util/stream/GatherersTest.java
On Thu, 30 Nov 2023 16:08:54 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList java/util/stream/GatherersTest.java. All right! Hope it would be fixed soon. Trivial. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16909#pullrequestreview-1757841717
Re: RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 15:48:11 GMT, Jorn Vernee wrote: > This test is problematic on Zero due to > https://bugs.openjdk.org/browse/JDK-8321064 > > Disable it for now on that platform, until a Zero maintainer has a chance to > look into it. > > Testing: running TestHandshake on zero to see that it is skipped. Hold on, can we figure out if Zero can actually be fixed before we go and disable the test? I think we only disable the tests like this if there is an intrinsic reason why the test does not apply to a platform. - PR Comment: https://git.openjdk.org/jdk/pull/16906#issuecomment-1834084558
Re: RFR: JDK-8320940: Fix typo in java.lang.Double
On Wed, 29 Nov 2023 02:00:14 GMT, Joe Darcy wrote: > Typo fix to to the new text added in JDK-8295391. Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16872#pullrequestreview-1754814207
Re: RFR: 8318776: Require supports_cx8 to always be true [v6]
On Thu, 23 Nov 2023 03:14:27 GMT, David Holmes wrote: >> As discussed in JBS all platforms (some tweaks to Zero are in progress) >> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip >> out the locked-based alternatives to using it and just add a guarantee that >> it is true at runtime. And all platforms except some ARM variants set >> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: >> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not >> defined >> - Assertions for `supports_cx8()` are removed >> - Compiler predicates requiring `supports_cx8()` are removed >> - Access backend is greatly simplified without the need for lock-based >> alternative >> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the >> need for a lock-based alternative >> >> I did consider moving all the ARM `kuser_helper` related code to be only >> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a >> theoretical risk this could change the behaviour if ARMv7 binaries were run >> on other ARM CPU's. I added a note to that effect in >> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up >> further if desired. >> >> Testing: >> - All Oracle tiers 1-5 builds (which includes an ARMv7 build) >> - GHA builds/tests >> - Oracle tiers 1-3 sanity testing >> >> Zero changes coming in via >> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged >> when they arrive. >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Fix typo Ran full jcstress on linux-arm-zero-release on RPi 4 without problem. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16625#pullrequestreview-1745896266
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 18:26:12 GMT, Daniel D. Daugherty wrote: >> src/hotspot/cpu/x86/vm_version_x86.cpp line 819: >> >>> 817: } >>> 818: >>> 819: _supports_cx8 = supports_cmpxchg8(); >> >> I think we should leave the runtime check here (under `ifndef`, like in >> ARM?). This covers the remaining case of running on legacy x86 without CX8 >> implemented: the init guarantee would then fire and prevent any other >> surprises at runtime. Sure, it would be hard to come up with such a platform >> today, but it would be safer to refuse to run there right away on the >> off-chance someone actually has it :) > > @shipilev - Do you have a particular legacy x86 in mind? My point is that it is such an easy thing to do: leave the "cx8" flag sensing code in, and keep setting up `_supports_cx8` based on it. This both provides more safety by failing cleanly on non-CX8 platform, and gives other platforms some guidance: if you can check something is supported, check it. But now that you nerd-sniped me into this... I think non-CX8 platforms would probably predate Pentium. The oldest real machine my lab has is Z530, which already has CX8. But it was easy to also go to my QEMU-driven build-test server, ask for `i486` as platform there, and et voila, no `cx8` in CPU flags: buildworker-debian12-32:~$ lscpu Architecture:i486 CPU op-mode(s):32-bit Address sizes: 36 bits physical, 32 bits virtual Byte Order:Little Endian CPU(s): 4 On-line CPU(s) list: 0-3 Vendor ID: GenuineIntel Model name:486 DX/4 CPU family: 4 Model: 8 Thread(s) per core: 4 Core(s) per socket: 1 Socket(s): 1 Stepping:0 BogoMIPS:5699.99 Flags: fpu vme pse apic ht cpuid tsc_known_freq x2apic hypervisor cpuid_fault And mainline JDK even starts there! (with interpreter, there are some asserts firing in compiler code, having to do with odd instruction selection on some paths): $ jdk/bin/java -Xint -version openjdk version "22-testing" 2024-03-19 OpenJDK Runtime Environment (fastdebug build 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121) OpenJDK Server VM (fastdebug build 22-testing-builds.shipilev.net-openjdk-jdk-b627-20231121, interpreted mode, sharing) - PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1402738580
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 08:57:11 GMT, Aleksey Shipilev wrote: > Zero tests are running. Caught the `guarantee` on linux-arm-zero-fastdebug! But that is actually the fault in my previous patch: #16779. - PR Comment: https://git.openjdk.org/jdk/pull/16625#issuecomment-1822510325
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote:
>> As discussed in JBS all platforms (some tweaks to Zero are in progress)
>> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip
>> out the locked-based alternatives to using it and just add a guarantee that
>> it is true at runtime. And all platforms except some ARM variants set
>> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes:
>> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not
>> defined
>> - Assertions for `supports_cx8()` are removed
>> - Compiler predicates requiring `supports_cx8()` are removed
>> - Access backend is greatly simplified without the need for lock-based
>> alternative
>> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the
>> need for a lock-based alternative
>>
>> I did consider moving all the ARM `kuser_helper` related code to be only
>> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a
>> theoretical risk this could change the behaviour if ARMv7 binaries were run
>> on other ARM CPU's. I added a note to that effect in
>> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up
>> further if desired.
>>
>> Testing:
>> - All Oracle tiers 1-5 builds (which includes an ARMv7 build)
>> - GHA builds/tests
>> - Oracle tiers 1-3 sanity testing
>>
>> Zero changes coming in via
>> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged
>> when they arrive.
>>
>> Thanks.
>
> David Holmes has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - Merge with master and update Zero code accordingly
> - Merge branch 'master' into 8318776-supports_cx8
> - Remove unnecessary includes of vm_version.hpp.
>Fix copyright years.
> - Remove cx8 comment as no longer relevant (the spinlock is used regardless
> of cx8)
> - Remove suports_cx8() checks from gtest
> - Remove test for VMSupportsCX8
> - 8318776: Require supports_cx8 to always be true
src/hotspot/share/runtime/vm_version.cpp line 33:
> 31: void VM_Version_init() {
> 32: VM_Version::initialize();
> 33: guarantee(VM_Version::supports_cx8(), "Support for 64-bit atomic
> operations in required in this release");
Typo: "in required in". Also, no need to mention "this release" at all?
Suggestion for message: "JVM requires platform support for 64-bit atomic
operations"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1401743607
Re: RFR: 8318776: Require supports_cx8 to always be true [v5]
On Wed, 22 Nov 2023 02:09:38 GMT, David Holmes wrote: >> As discussed in JBS all platforms (some tweaks to Zero are in progress) >> actually do support `cx8` i.e. 64-bit compare-and-exchange, so we can strip >> out the locked-based alternatives to using it and just add a guarantee that >> it is true at runtime. And all platforms except some ARM variants set >> `SUPPORTS_NATIVE_CX8`, so we can greatly simplify things. Summary of changes: >> - `_supports_cx8` field is only needed when `SUPPORTS_NATIVE_CX8` is not >> defined >> - Assertions for `supports_cx8()` are removed >> - Compiler predicates requiring `supports_cx8()` are removed >> - Access backend is greatly simplified without the need for lock-based >> alternative >> - `java.util.concurrent.AtomicLongFieldUpdater` is simplified without the >> need for a lock-based alternative >> >> I did consider moving all the ARM `kuser_helper` related code to be only >> defined when `SUPPORTS_NATIVE_CX8` is not defined, but there was a >> theoretical risk this could change the behaviour if ARMv7 binaries were run >> on other ARM CPU's. I added a note to that effect in >> vm_version_linux_arm32.cpp so the ARM port maintainers could clean this up >> further if desired. >> >> Testing: >> - All Oracle tiers 1-5 builds (which includes an ARMv7 build) >> - GHA builds/tests >> - Oracle tiers 1-3 sanity testing >> >> Zero changes coming in via >> [JDK-8319777](https://bugs.openjdk.org/browse/JDK-8319777) will be merged >> when they arrive. >> >> Thanks. > > David Holmes has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Merge with master and update Zero code accordingly > - Merge branch 'master' into 8318776-supports_cx8 > - Remove unnecessary includes of vm_version.hpp. >Fix copyright years. > - Remove cx8 comment as no longer relevant (the spinlock is used regardless > of cx8) > - Remove suports_cx8() checks from gtest > - Remove test for VMSupportsCX8 > - 8318776: Require supports_cx8 to always be true Thanks! Zero tests are running. The PR looks great, except extra safety suggestion in x86 part: src/hotspot/cpu/x86/vm_version_x86.cpp line 819: > 817: } > 818: > 819: _supports_cx8 = supports_cmpxchg8(); I think we should leave the runtime check here (under `ifndef`, like in ARM?). This covers the remaining case of running on legacy x86 without CX8 implemented: the init guarantee would then fire and prevent any other surprises at runtime. Sure, it would be hard to come up with such a platform today, but it would be safer to refuse to run there right away on the off-chance someone actually has it :) - PR Review: https://git.openjdk.org/jdk/pull/16625#pullrequestreview-1743847107 PR Review Comment: https://git.openjdk.org/jdk/pull/16625#discussion_r1401696816
