Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 22:05:06 GMT, Mandy Chung wrote: >>> because a conditionally-exported package is considered a >>> non-(unconditionally-)exported package. >> >> OK. I need to look into the right solution for this. The Proxy >> implementation uses null protection domain to define the proxy class whereas >> this patch uses the protection domain as the interface to define the MHProxy >> class. That's why this issue only occurs in this new implementation. > > If we can avoid implementing `sun.invoke.WrapperInstance`, this package > access check issue would go away. Do you think you can look into it? I think we can probably insert a static final field in a wrapper instance class to point to the implemented class and verify with the `PROXY_CLASS_INFOS` ClassValue, much like VarForm field in VarHandle implementations. This should have less reflective impact than the annotation-based approach, and can be cached in a ClassValue as well. Should we cache it though? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207426728
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang wrote: >> This patch moves the parameters to an immutable list, to avoid allocations >> on `parameterList` as well. In addition, it fixes 8304932, the bug where if >> a caller keeps a reference to the array passed into `MethodTypeDesc.of`, the >> caller may mutate the Desc via the array and can create invalid >> MethodTypeDesc. >> >> The latest benchmark results are available here: >> https://github.com/openjdk/jdk/pull/13186#issuecomment-1560378822 >> >> This patch has minor performance gains on `ofDescriptor` factory, even >> compared to Adam's patch that optimized `ofDescriptor` in #12945. >> >> Benchmark of Oracle JDK 20: >> https://gist.github.com/683c6219e183cbc2b336224fc2c0d50a >> Benchmark of this patch: >> https://gist.github.com/22be9371a2370fb4a7b44f1684750ec4 >> Benchmark of [asotona's >> patch](https://github.com/openjdk/jdk/pull/12945/files#diff-ac8e413d3e13532a2b0d34a90253c6ddd7a4f04082f792b9d076e9b5a33f2078): >> https://gist.github.com/eb98579c3b51cafae481049a95a78f80 >> >> [sotona vs >> this](https://jmh.morethan.io/?gists=eb98579c3b51cafae481049a95a78f80,22be9371a2370fb4a7b44f1684750ec4); >> [20 vs >> this](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,22be9371a2370fb4a7b44f1684750ec4); >> [20 vs >> sotona](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,eb98579c3b51cafae481049a95a78f80), >> for reference > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > reuse immutable list to avoid array allocation The benefits of a list implementation over an array implementation are: 1. `parameterList()` is much faster, allowing more beautiful user code compared to manually unpacked iteration (immutable list factory doesn't allow using non-Object backing arrays) 2. `of(ClassDesc, List)` allows users to share immutable parameter lists than having to copy > With https://github.com/openjdk/jdk/pull/13671, MethodTypeDesc is cached and > I wonder if this is no longer the bottleneck of ClassFile API performance. The parsing is no more, but `descriptorString` is still a bottleneck. The master in the last benchmark already had 8306842 integrated. In fact, I don't think argType array sharing goes far enough: in these modification methods and ofDescriptor, we can skip the array/list iteration validation if we can ensure the change parts alone have not produced an invalid method type (insertion of void, slot changes). I would postpone such in-depth sharing into a later issue, where we track the slot count information as well (to preemptively reject invalid resolveConstantDesc calls) > Converting `argTypes` to an immutable list seems to be an extra step to make > it immutable - we need frozen arrays!! Unfortunately, this is currently the only way general users have access to frozen arrays, where index-based access may be constant-folded, as `@Stable` is not generally available. - PR Comment: https://git.openjdk.org/jdk/pull/13186#issuecomment-1565013833
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 22:03:40 GMT, Mandy Chung wrote: >> If we keep a cache of all MHProxy classes, I think this would not need >> `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other >> class except `WrapperInstance` and the `ensureOriginalLookup` method would >> need to be in an internal class. So no problem of implementing >> `sun.invoke.WrapperInstance`. > >> because a conditionally-exported package is considered a >> non-(unconditionally-)exported package. > > OK. I need to look into the right solution for this. The Proxy > implementation uses null protection domain to define the proxy class whereas > this patch uses the protection domain as the interface to define the MHProxy > class. That's why this issue only occurs in this new implementation. If we can avoid implementing `sun.invoke.WrapperInstance`, this package access check issue would go away. Do you think you can look into it? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207416972
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:29:14 GMT, Chen Liang wrote: >>> This approach also requires the Class loading checks since the interface is >>> not unconditionally exported and fails security manager. >> >> Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy >> implements `sun.invoke.WrapperInstance`, it needs to export >> `java.base/sun.invoke` to the dynamic module.The change to >> `ClassLoader::checkPackageAccess` doesn't make sense for this. > > Yes. Unfortunately, it fails at > https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 > because a conditionally-exported package is considered a > non-(unconditionally-)exported package. If we keep a cache of all MHProxy classes, I think this would not need `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other class except `WrapperInstance` and the `ensureOriginalLookup` method would need to be in an internal class. So no problem of implementing `sun.invoke.WrapperInstance`. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207387430
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:31:40 GMT, Mandy Chung wrote: >> Yes. Unfortunately, it fails at >> https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 >> because a conditionally-exported package is considered a >> non-(unconditionally-)exported package. > > If we keep a cache of all MHProxy classes, I think this would not need > `sun.invoke.WrapperInstance`. OTOH, `sun.invoke` does not have any other > class except `WrapperInstance` and the `ensureOriginalLookup` method would > need to be in an internal class. So no problem of implementing > `sun.invoke.WrapperInstance`. > because a conditionally-exported package is considered a > non-(unconditionally-)exported package. OK. I need to look into the right solution for this. The Proxy implementation uses null protection domain to define the proxy class whereas this patch uses the protection domain as the interface to define the MHProxy class. That's why this issue only occurs in this new implementation. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207415941
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:06:36 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line
>> 209:
>>
>>> 207: if (intfc.isHidden())
>>> 208: throw newIllegalArgumentException("a hidden interface",
>>> intfc.getName());
>>> 209: if (!VM.isModuleSystemInited())
>>
>> I don't expect this is needed. I assume you are thinking for LMF to use
>> this API?
>
> Proxy-based impl had this check, so I'd assume MHP might want to defend
> against the same kind of improper usage.
Proxy is used by annotation implementation. I suspect that's why that check
was there.
For `MHP::asInterfaceInstance`, I would drop the `isModuleSystemInitied` check
until the API is being used during early startup. I don't expect it be called
before module system is initialized.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207391651
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 02:06:55 GMT, Joe Darcy wrote:
> Explain in java.lang.Class how unnamed classes are modeled in core reflection.
src/java.base/share/classes/java/lang/Class.java line 212:
> 210: * {@linkplain #getTypeName type name}, {@linkplain #getSimpleName
> 211: * simple name}, and {@linkplain #getCanonicalName canonical name} all
> 212: * return results equal to "{@code HelloWorld}".
I recommend moving the quotes into `{@code}` because this is how the file name
is represented above. Same is used for existing strings in code, like the
rendering of `{@value #STRING_FIELD}` values.
Suggestion:
* return results equal to {@code "HelloWorld"}.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14165#discussion_r1207393725
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v22]
On Fri, 26 May 2023 13:22:59 GMT, Jim Laskey wrote: >> Add flexible main methods and anonymous main classes to the Java language. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Remove trailing whitespace In case I overlooked them, it would be good to have some core reflection tests of what a unnamed class looks like reflectively, basically a core reflection analogue to the draft javac/javax.lang.model test: https://github.com/openjdk/jdk/pull/14140/files#diff-106fc5b2c4d391546c7fa8a75e68fbd963733964f8f7ddf577606b5afda9b889 - PR Comment: https://git.openjdk.org/jdk/pull/13689#issuecomment-1564983686
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:23:32 GMT, Mandy Chung wrote: >> John Rose argues against using class data for Leyden. Since class data is >> the only known way to defend against user-manufactured annotations, I >> switched back to an extra wrapper interface. This approach also requires the >> Class loading checks since the interface is not unconditionally exported and >> fails security manager. > >> This approach also requires the Class loading checks since the interface is >> not unconditionally exported and fails security manager. > > Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy > implements `sun.invoke.WrapperInstance`, it needs to export > `java.base/sun.invoke` to the dynamic module.The change to > `ClassLoader::checkPackageAccess` doesn't make sense for this. Yes. Unfortunately, it fails at https://github.com/openjdk/jdk/blob/bd21e68e679c41c98f39dd2fbd38270ae7d55ed9/src/java.base/share/classes/java/lang/SecurityManager.java#L1317 because a conditionally-exported package is considered a non-(unconditionally-)exported package. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207384208
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 21:05:39 GMT, Chen Liang wrote: > This approach also requires the Class loading checks since the interface is > not unconditionally exported and fails security manager. Are you referring to `ClassLoader::checkPackageAccess` change? As MHProxy implements `sun.invoke.WrapperInstance`, it needs to export `java.base/sun.invoke` to the dynamic module.The change to `ClassLoader::checkPackageAccess` doesn't make sense for this. - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207376506
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Fri, 26 May 2023 18:39:53 GMT, Mandy Chung wrote:
>> Chen Liang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove assertion, no longer true with teleport definition in MHP
>
> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line
> 209:
>
>> 207: if (intfc.isHidden())
>> 208: throw newIllegalArgumentException("a hidden interface",
>> intfc.getName());
>> 209: if (!VM.isModuleSystemInited())
>
> I don't expect this is needed. I assume you are thinking for LMF to use this
> API?
Proxy-based impl had this check, so I'd assume MHP might want to defend against
the same kind of improper usage.
> src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line
> 433:
>
>> 431:
>> 432: private static WrapperInstance asWrapperInstance(Object x) {
>> 433: if (x instanceof WrapperInstance wrapperInstance)
>
> In the previous version, `WrapperInstance` was not needed. Instead it checks
> if the class is a MHProxy class. Did you run into any issue that you
> resurrect `WrapperInstance`?Is it just because of accessing
> `ensureOriginalLookup`?
John Rose argues against using class data for Leyden. Since class data is the
only known way to defend against user-manufactured annotations, I switched back
to an extra wrapper interface. This approach also requires the Class loading
checks since the interface is not unconditionally exported and fails security
manager.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207350363
PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1207349212
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 02:06:55 GMT, Joe Darcy wrote:
> Explain in java.lang.Class how unnamed classes are modeled in core reflection.
src/java.base/share/classes/java/lang/Class.java line 192:
> 190: * Unnamed Classes
> 191: *
> 192: * The {@code class} file representing an unnnamed class is generated
Typo unnnamed (3 n's)
src/java.base/share/classes/java/lang/Class.java line 193:
> 191: *
> 192: * The {@code class} file representing an unnnamed class is generated
> 193: * by a Java compiler from a source file from an unnamed class. The
"for an unnamed class." ?
src/java.base/share/classes/java/lang/Class.java line 196:
> 194: * {@code Class} object representing an unnamed class is top-level,
> 195: * {@linkplain #isSynthetic synthetic}, and {@code final}. While an
> 196: * unnamed class does not have have a name in its Java source
typo: have have
src/java.base/share/classes/java/lang/Class.java line 201:
> 199: * representing an unnamed class. Conventionally, a Java compiler
> 200: * creates a class file where the class name matches the base name of
> 201: * the class file; for example, a source file for an unnamed class
s/the class file;/the source file;/ ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14165#discussion_r1206160539
PR Review Comment: https://git.openjdk.org/jdk/pull/14165#discussion_r1206160610
PR Review Comment: https://git.openjdk.org/jdk/pull/14165#discussion_r1206160814
PR Review Comment: https://git.openjdk.org/jdk/pull/14165#discussion_r1206161235
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 17:18:42 GMT, Mandy Chung wrote: > Looks fine. This version matches the current implementation where > `getCanonicalName` returns non-null. When the implementation of > `getCanonicalName` is updated to return null, the spec should be updated. > > Nit: `` is needed assuming the section of "Unnamed Classes" has 3 > paragraphs. Thanks @mlchung. Yes, the current spec matches what the implementation in @JimLaskey 's PR https://github.com/openjdk/jdk/pull/13689 does at the time of writing. Discussions are on-going with the JEP 445 team regarding whether and how to implement a "isUnnamedClass" predicate for core reflection which would allow the `getCanonicalName` method to screen for unnamed classes and return null, as would reasonably be implied by a reading of the most current draft JLS updates. - PR Comment: https://git.openjdk.org/jdk/pull/14165#issuecomment-1564901806
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 02:06:55 GMT, Joe Darcy wrote: > Explain in java.lang.Class how unnamed classes are modeled in core reflection. > > Looks fine. This version matches the current implementation where > > `getCanonicalName` returns non-null. When the implementation of > > `getCanonicalName` is updated to return null, the spec should be updated. > > Nit: `` is needed assuming the section of "Unnamed Classes" has 3 > > paragraphs. > > Thanks @mlchung. > > Yes, the current spec matches what the implementation in @JimLaskey 's PR > #13689 does at the time of writing. > > Discussions are on-going with the JEP 445 team regarding whether and how to > implement a "isUnnamedClass" predicate for core reflection which would allow > the `getCanonicalName` method to screen for unnamed classes and return null, > as would reasonably be implied by a reading of the most current draft JLS > updates. PS Depending on the result of those discussion, the specification for `getCanonicalName` may be changed to return `null`. - PR Comment: https://git.openjdk.org/jdk/pull/14165#issuecomment-1564920419
Re: RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
On Fri, 26 May 2023 02:06:55 GMT, Joe Darcy wrote: > Explain in java.lang.Class how unnamed classes are modeled in core reflection. Looks fine. This version matches the current implementation where `getCanonicalName` returns non-null. When the implementation of `getCanonicalName` is updated to return null, the spec should be updated. Nit: `` is needed assuming the section of "Unnamed Classes" has 3 paragraphs. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14165#pullrequestreview-1446650885
RFR: JDK-8308913: Update core reflection for JEP 445 (preview)
Explain in java.lang.Class how unnamed classes are modeled in core reflection. - Commit messages: - Cleanup and edit text. - Respond to review comments. - Appease jcheck. - JDK-8308913: Update core reflection for JEP 445 (preview) Changes: https://git.openjdk.org/jdk/pull/14165/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14165=00 Issue: https://bugs.openjdk.org/browse/JDK-8308913 Stats: 26 lines in 1 file changed: 25 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14165.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14165/head:pull/14165 PR: https://git.openjdk.org/jdk/pull/14165
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v5]
On Wed, 24 May 2023 21:02:00 GMT, Jiangli Zhou wrote: >> Original description for JDK-8307194 change: >> - >> This PR is branched from the makefile changes for >> https://bugs.openjdk.org/browse/JDK-8303796 and contains the following for >> handling the JDK/hotspot static libraries: >> >> - Build hotspot libjvm.a and JDK static libraries for >> static-libs-image/static-libs-bundles targets; This change does not affect >> the graal-builder-image target >> >> - For libjvm.a specifically, exclude operator_new.o >> >> - Filter out "external" .o files (those are the .o files included from a >> different JDK library and needed when creating the .so shared library only) >> from JDK .a libraries; That's to avoid linker failures caused by duplicate >> symbols >> - For libjli.a: Not include inflate.o inftrees.o inffast.o zadler32.o >> zcrc32.o zutil.o (compiled from zlib sources) if zlib is built as JDK bundled >> - For libawt_xawt.a and libawt_head.a: Not include systemScale.o, since >> it's provided in libawt.a >> >> - Handle long arguments case for static build in >> make/common/NativeCompilation.gmk >> >> - Address @erikj79's comment in >> https://github.com/openjdk/jdk/pull/13709#discussion_r1180750185 for >> LIBJLI_STATIC_EXCLUDE_OBJS >> - >> >> Updates to address build failures reported on macosx- platforms: >> >> - For gcc/clang, when building a static library first partially link (using >> the `-r` linking option) all object files into one object. The output object >> file from the partial linking is then passed to `ar` to create the static >> library. >> >> The original change for JDK-8307194 used @argument_file for all platforms >> when dealing with long arguments to `ar`, which caused failures on >> macosx- builds. On darwin (https://www.unix.com/man-page/osx/1/ar/), >> `ar` does not support @argument_file. The updated change avoids using >> @argument_file for `ar`. >> >> The partial linking change is done in make/common/NativeCompilation.gmk. The >> flag related change is done in make/autoconf/flags-ldflags.m4 mainly. > > Jiangli Zhou has updated the pull request incrementally with one additional > commit since the last revision: > > Update make/common/NativeCompilation.gmk > > Thanks you! > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> `with `make TARGETS="arm-linux-gnueabihf" BASE_OS=Fedora BASE_OS_VERSION=17`, also tried with BASE_OS_VERSION=17` Fix typo: `also tried with BASE_OS_VERSION=18` - PR Comment: https://git.openjdk.org/jdk/pull/14064#issuecomment-1564919758
Re: RFR: 8306889: Convert MethodTypeDescImpl parameter storage from array to immutable list [v4]
On Wed, 24 May 2023 02:29:09 GMT, Chen Liang wrote:
>> This patch moves the parameters to an immutable list, to avoid allocations
>> on `parameterList` as well. In addition, it fixes 8304932, the bug where if
>> a caller keeps a reference to the array passed into `MethodTypeDesc.of`, the
>> caller may mutate the Desc via the array and can create invalid
>> MethodTypeDesc.
>>
>> The latest benchmark results are available here:
>> https://github.com/openjdk/jdk/pull/13186#issuecomment-1560378822
>>
>> This patch has minor performance gains on `ofDescriptor` factory, even
>> compared to Adam's patch that optimized `ofDescriptor` in #12945.
>>
>> Benchmark of Oracle JDK 20:
>> https://gist.github.com/683c6219e183cbc2b336224fc2c0d50a
>> Benchmark of this patch:
>> https://gist.github.com/22be9371a2370fb4a7b44f1684750ec4
>> Benchmark of [asotona's
>> patch](https://github.com/openjdk/jdk/pull/12945/files#diff-ac8e413d3e13532a2b0d34a90253c6ddd7a4f04082f792b9d076e9b5a33f2078):
>> https://gist.github.com/eb98579c3b51cafae481049a95a78f80
>>
>> [sotona vs
>> this](https://jmh.morethan.io/?gists=eb98579c3b51cafae481049a95a78f80,22be9371a2370fb4a7b44f1684750ec4);
>> [20 vs
>> this](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,22be9371a2370fb4a7b44f1684750ec4);
>> [20 vs
>> sotona](https://jmh.morethan.io/?gists=683c6219e183cbc2b336224fc2c0d50a,eb98579c3b51cafae481049a95a78f80),
>> for reference
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> reuse immutable list to avoid array allocation
The microbenchmark shows the performance of using the `MethodTypeDesc` factory
methods. With [#13671](https://github.com/openjdk/jdk/pull/13671),
`MethodTypeDesc` is cached and I wonder if this is no longer the bottleneck of
ClassFile API performance.
[JDK-8304932](https://bugs.openjdk.org/browse/JDK-8304932) is a bug that can
simply be fixed by
diff --git a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
index 738c4d68a43..ed23887c9ef 100644
--- a/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
+++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDesc.java
@@ -95,7 +95,7 @@ public sealed interface MethodTypeDesc
* {@link ClassDesc} for {@code void}
*/
static MethodTypeDesc of(ClassDesc returnDesc, ClassDesc... paramDescs) {
-return new MethodTypeDescImpl(returnDesc, paramDescs);
+return new MethodTypeDescImpl(returnDesc, paramDescs.clone());
}
/**
diff --git
a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
index 4341c3fb56f..8586bfb5926 100644
--- a/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
+++ b/src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java
@@ -41,7 +41,7 @@ import static java.util.Objects.requireNonNull;
*/
final class MethodTypeDescImpl implements MethodTypeDesc {
private final ClassDesc returnType;
-private final ClassDesc[] argTypes;
+private final ClassDesc[] argTypes; // trusted array
/**
* Constructs a {@linkplain MethodTypeDesc} with the specified return type
@@ -102,14 +102,14 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
@Override
public MethodTypeDesc changeReturnType(ClassDesc returnType) {
-return MethodTypeDesc.of(returnType, argTypes);
+return new MethodTypeDescImpl(returnType, argTypes);
}
@Override
public MethodTypeDesc changeParameterType(int index, ClassDesc paramType) {
ClassDesc[] newArgs = argTypes.clone();
newArgs[index] = paramType;
-return MethodTypeDesc.of(returnType, newArgs);
+return new MethodTypeDescImpl(returnType, newArgs);
}
@Override
@@ -120,7 +120,7 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
ClassDesc[] newArgs = new ClassDesc[argTypes.length - (end - start)];
System.arraycopy(argTypes, 0, newArgs, 0, start);
System.arraycopy(argTypes, end, newArgs, start, argTypes.length - end);
-return MethodTypeDesc.of(returnType, newArgs);
+return new MethodTypeDescImpl(returnType, newArgs);
}
@Override
@@ -131,7 +131,7 @@ final class MethodTypeDescImpl implements MethodTypeDesc {
System.arraycopy(argTypes, 0, newArgs, 0, pos);
System.arraycopy(paramTypes, 0, newArgs, pos, paramTypes.length);
System.arraycopy(argTypes, pos, newArgs, pos+paramTypes.length,
argTypes.length - pos);
-return MethodTypeDesc.of(returnType, newArgs);
+return new MethodTypeDescImpl(returnType, newArgs);
}
```
I won't object to keep `argTypes` in an immutable list instead of an array.
However, `MethodTypeDescImpl::ofDescriptor` has to
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 16:58:41 GMT, Thomas Stuefe wrote: >> The crazy thing is that `malloc` is defined! That means all places where we >> use the term malloc are getting replaced without such a workaround. (E.g. >> for log tags.) > > So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc? > What about free? > > As ugly as defining malloc is (and I remember QADRT), I hesitate about > removing that define. > > Removing that define and hard-coding it here assumes 1) our replacement is > equivalent (ok, easy to check) 2) it will always be equivalent in future AIX > versions 3) pointers it returns work with the unchanged free() and realloc() > the system provides, and will always do so. > > I don't know... I would not do this just to get rid of a warning. This one is not just to get rid of a warning. We get real test errors because malloc gets replaced by vec_malloc in log tags. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207308708
Re: RFR: 8307858: [REDO] JDK-8307194 Add make target for optionally building a complete set of all JDK and hotspot libjvm static libraries [v4]
On Thu, 25 May 2023 01:19:11 GMT, Jiangli Zhou wrote: > > > > My build job is still running, but it has failed in two distinct ways > > > > already. See below for mac fix. Our cross build of arm32 fails with > > > > this message: > > > > ``` > > > > [2023-05-24T19:25:15,310Z] > > > > /opt/mach5/mesos/work_dir/jib-master/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/bin/ld: > > > > fatal error: cannot mix -r with dynamic object > > > > /opt/mach5/mesos/work_dir/jib-master/install/jpg/infra/builddeps/devkit-linux_x64-to-linux_arm/gcc8.2.0-Fedora27+1.0/devkit-linux_x64-to-linux_arm-gcc8.2.0-Fedora27+1.0.tar.gz/x86_64-linux-gnu-to-arm-linux-gnueabihf/bin/../lib/gcc/arm-linux-gnueabihf/8.2.0/../../../../arm-linux-gnueabihf/lib/libstdc++.so > > > > ``` > > > > > > > > > If this is a problem with your partial link solution, then perhaps just > > > employing the relative path trick for the `ar` command line on mac could > > > be enough to handle long paths and lots of object files? That appears to > > > be how we handle it with clang for linking already due to @-file problems. > > > > > > The partial linking was originally suggested by C++/Clang toolchain folks > > to mitigate linker overhead that was observed during final executable link > > time. For a static library containing any object file (`.o`) that was > > compiled with ThinLTO (https://clang.llvm.org/docs/ThinLTO.html) enabled, > > linking an executable using the static library without distributed ThinLTO > > could experience more overhead and slow down linking. Solving the macosx > > `ar` limitation is a side-effect/benefit of using partial linking. We > > probably would want to include the partial linking even without the `ar` > > limitation. > > Is it possible `$$($1_SYSROOT_LDFLAGS)` pulled in `libstdc++.so` as part of > > the input for partial linking with the linux-aarch64 cross build? > > Another possibility might be the user provided `BUILD_LDCXX` includes extra > options in the testing build (?). If that's the case, we probably could > define a separate `BUILD_LD_PARTIAL` with no added options. In our prototype > on JDK 11, we define a separate one for partial linking. I tried building `static-libs-image` for linux-aarch64 on Ubuntu machine using `devkit` to reproduce the failure. Also tried building for `linux-ppc64le-server-release` target using `devkit` on Ubuntu. Both built successfully with using `devkit`. I could not build a `devkit` for arm32 (with `make TARGETS="arm-linux-gnueabihf" BASE_OS=Fedora BASE_OS_VERSION=17`, also tried with BASE_OS_VERSION=17). @erikj79 Could you please help provide additional insight for the build failure you found for arm32? `BUILD_LIBJVM_partial_link.cmdline` might help shed some light. For the `aarch64-linux-gnu` build using `devkit`, I see following, which is ok. No unexpected options are included. /.../bin/aarch64-linux-gnu-g++ -r --sysroot=/.../aarch64-linux-gnu/sysroot -o /...linux-aarch64-server-release/hotspot/variant-server/libjvm/objs/static/libjvm_relocatable.o @/.../hotspot/variant-server/libjvm/objs/static/_BUILD_LIBJVM_objectfilenames.txt - PR Comment: https://git.openjdk.org/jdk/pull/14064#issuecomment-1564908324
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array [v2]
> This change prevents the contents of the internal string array from being > copied back when releasing it. Rudi Horn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: Use JNI_ABORT to release string bytes - Changes: https://git.openjdk.org/jdk/pull/14117/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14117=01 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14117.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14117/head:pull/14117 PR: https://git.openjdk.org/jdk/pull/14117
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
On Fri, 26 May 2023 17:36:12 GMT, Michael McMahon wrote: > There was an unrelated change to these functions recently. You might want to > rebase this PR with the latest version. Done - PR Comment: https://git.openjdk.org/jdk/pull/14117#issuecomment-1564855730
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote:
>> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`.
>>
>> A few implementation-detail methods in VarHandle are now documented with the
>> implied constraints to avoid subtle problems in the future. Changed
>> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy
>> VarHandle changes. Also changed VarHandleBaseTest to report the whole
>> incorrect type of exception caught than swallow it and leaving only a
>> message.
>>
>> Current problems:
>> - [ ] The lazy var handle is quite slow on the first invocation.
>>- As seen in the benchmark, users can first call
>> `Lookup::ensureInitialized` to create an eager handle.
>>- After that, the lazy handle has a performance on par with the regular
>> var handle.
>> - [ ] The class-loading-based test is not in a unit test
>>- The test frameworks don't seem to offer fine-grained control for
>> class-loading detection or reliable unloading
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ±
>> 0.003 ns/op
>> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ±
>> 0.002 ns/op
>>
>>
>> BenchmarkMode Cnt Score
>> Error Units
>> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ±
>> 7735.276 ns/op
>> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ±
>> 8482.883 ns/op
>> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ±
>> 9776.343 ns/op
>> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ±
>> 72894.865 ns/op
>> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ±
>> 3513.909 ns/op
>> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ±
>> 5144.540 ns/op
>> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ±
>> 10816.983 ns/op
>> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ±
>> 189370.448 ns/op
>
> Chen Liang 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 eight additional commits since
> the last revision:
>
> - Remove export for removed package
> - Merge branch 'master' into lazy-static-varhandle
> - Test the creation performance of handles, lazy one indeed better
> - Merge branch 'master' into lazy-static-varhandle
> - copyright year
> - Benchmarks. lazy initialize is SLOW
> - nuke meaningless overrides
> - make static field var handles lazy and fix NPE in isAccessModeSupported
src/java.base/share/classes/java/lang/invoke/VarHandles.java line 111:
> 109: else {
> 110: if (UNSAFE.shouldBeInitialized(refc)) {
> 111: return new LazyStaticVarHandle(refc, f,
> isWriteAllowedOnFinalFields, false);
This should probably call `maybeAdapt` on the result:
Suggestion:
return maybeAdapt(new LazyStaticVarHandle(refc, f,
isWriteAllowedOnFinalFields, false));
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1207261501
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v3]
On Fri, 26 May 2023 18:11:14 GMT, Maurizio Cimadamore
wrote:
>> As the FFM API evolved over time, some parts of the javadoc went out of
>> sync. Now that most of the API is stable, it is a good time to look again at
>> the javadoc as a whole, and bring some more consistency.
>>
>> While most of the changes in this PR are stylistic, I'd like to call out few
>> things which resulted in API changes:
>>
>> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified
>> requirement that its alignment parameter must be a power of two.
>>
>> * `MemoryLayout::sliceHandle` did not require the absence of dereference
>> paths in the provided layout path. While that is technically possible to
>> allow, currently the method is specified as returning a "slice"
>> corresponding to some "nested layout", so following pointers seems
>> incompatible with the spec. I have decided to disallow for now - we can
>> always compatibly enable it later, if required.
>>
>> * `MemorySegment::copy` - most of the overloads did not specify that
>> `UnsupportedOperationException` is thrown if the destination segment is
>> read-only.
>>
>> * In several places, an extra `@throws IllegalArgumentException` or `@throws
>> IllegalArgumentException` has been added to capture cases where element size
>> * index computation can overflow. This is true for all the element-wise
>> segment copy methods, for the indexed accessors in memory segment (e.g.
>> `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for
>> `SegmentAllocator::allocateArray(MemoryLayout, long)`.
>>
>> In all these cases (except for overflows), new tests have been added to
>> cover the additional API changes (a CSR will also be filed to cover these).
>>
>> The class with most changes is `MemoryLayout`. I realized that the javadoc
>> there was a bit sloppy around the definition of "layout paths". Now there
>> are several subsection in the class javadoc, which explain how different
>> kinds of paths can be used. I have introduced the notion of "open path
>> elements" to denote those path elements that are not fully specified, and
>> result in additional var handle coordinates to be added. There is also now a
>> definition of what it means for a layout path to be "well-formed", so that
>> all methods accepting a layout path can simply refer to it (this definition
>> is tricky to give inline in the javadoc of the various path-accepting
>> methods, as it depends on many factors).
>>
>> Another biggie change was in `MemorySegment` as now all dereference methods
>> state precisely their spatial checks requirements, as well also specifying
>> when the API can th...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Improve javadoc on supported linker layouts
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 187:
> 185: * ).withName("point")
> 186: * )
> 187: * ).withName("points")
Wrong indentation:
Suggestion:
* )
* ).withName("points")
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1207238878
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v2]
On Wed, 24 May 2023 16:26:28 GMT, Alan Bateman wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review suggestions
>
> src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 140:
>
>> 138: */
>> 139: /* package-private */
>> 140: static String toUpperCase(String str) {
>
> A bit icky to have OperatingSystem to expose a static method to convert
> Strings as it's nothing to do with OperatingSystem, is there anywhere else?
Delaying the initialization of OperatingSystem made it possible to revert to
String.toUpperCase().
Enabled by the fix in PR [14181](https://github.com/openjdk/jdk/pull/14181).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14063#discussion_r1207228241
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang wrote:
>> As John Rose has pointed out in this issue, the current j.l.r.Proxy based
>> implementation of MethodHandleProxies.asInterface has a few issues:
>> 1. Exposes too much information via Proxy supertype (and WrapperInstance
>> interface)
>> 2. Does not allow future expansion to support SAM[^1] abstract classes
>> 3. Slow (in fact, very slow)
>>
>> This patch addresses all 3 problems:
>> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method
>> clashes
>> 2. This patch obtains already generated classes from a ClassValue by the
>> requested interface type; the ClassValue can later be updated to compute
>> implementation generation for abstract classes as well.
>> 3. This patch's faster than old implementation in general.
>>
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15
>> 1.483 ±0.025 ns/op
>> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15
>> 0.264 ±0.006 ns/op
>> MethodHandleProxiesAsIFInstance.testCall avgt 15
>> 1.773 ±0.040 ns/op
>> MethodHandleProxiesAsIFInstance.testCreate avgt 15
>> 16.754 ±0.411 ns/op
>> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15
>> 19.609 ±1.598 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15
>> 0.424 ±0.024 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15
>> 1.936 ±0.008 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15
>> 1.766 ±0.014 ns/op
>> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15
>> 0.414 ±0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15
>> 0.271 ±0.006 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15
>> 0.263 ±0.004 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15
>> 0.266 ±0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15
>> 0.262 ±0.003 ns/op
>> MethodHandleProxiesAsIFInstanceCall.direct avgt 15
>> 0.264 ±0.005 ns/op
>> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15
>> 18.000 ±0.181 ns/op
>> MethodHandleProxiesAsIFInstanceCreat...
>
> Chen Liang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove assertion, no longer true with teleport definition in MHP
src/hotspot/share/prims/jvm.cpp line 1019:
> 1017: }
> 1018: }
> 1019: assert(Reflection::is_same_class_package(lookup_k, ik),
I use the Lookup of the proxy interface to define a hidden class in a dynamic
module as the dynamic module has no class yet and it's defined to the class
loader of the proxy interface.
We should keep the same package check if the defined class is a normal class or
a hidden nestmate class. It only allows the lookup class be in a different
package for defining a hidden non-nestmate class. This is only internal
capability.`Lookup::defineHiddenClass` API will throw IAE if the given
bytes denotes a class in a different package than the lookup class.
+ if ((!is_hidden || is_nestmate) &&
!Reflection::is_same_class_package(lookup_k, ik)) {
+// non-hidden class or nestmate class must be in the same package as the
Lookup class
+THROW_MSG_0(vmSymbols::java_lang_IllegalArgumentException(), "Lookup class
and defined class are in different packages");
+ }
+
src/java.base/share/classes/java/lang/ClassLoader.java line 685:
> 683: final SecurityManager sm = System.getSecurityManager();
> 684: if (sm != null) {
> 685: if (ReflectUtil.isNonPublicProxyClass(cls) ||
> ReflectUtil.isMethodHandleProxiesClass(cls)) {
Why do you need this? `Proxy` allows non-public interface whereas
`MethodHandleProxies` only allows public interface.
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 209:
> 207: if (intfc.isHidden())
> 208: throw newIllegalArgumentException("a hidden interface",
> intfc.getName());
> 209: if (!VM.isModuleSystemInited())
I don't expect this is needed. I assume you are thinking for LMF to use this
API?
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 245:
> 243: private record LocalMethodInfo(MethodTypeDesc desc, List
> thrown, String fieldName) {}
> 244:
> 245: private record ProxyClassInfo(MethodHandle constructor, Lookup
> originalLookup) {}
`ProxyClassInfo` for interface `I` will be stored in the class value of `I`.
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v7]
On Thu, 25 May 2023 15:25:40 GMT, Volker Simonis wrote:
>> Since JDK13, executing commands in a sub-process defaults to the so called
>> `POSIX_SPAWN` launching mechanism (i.e.
>> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
>> using `posix_spawn(3)` to firstly start a tiny helper program called
>> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
>> command data from the parent Java process over a Unix pipe and finally
>> executes (i.e. `execvp(3)`) the requested command.
>>
>> In cases where the parent process terminates abnormally before
>> `jspawnhelper` has read all the expected data from the pipe, `jspawnhelper`
>> will block indefinitely on the reading end of the pipe. This is especially
>> harmful if the parent process had open sockets, because in that case, the
>> forked `jspawnhelper` process will inherit them and keep all the
>> corresponding ports open effectively preventing other processes to bind to
>> them. Notice that this is not an abstract scenario. We've observed this
>> regularly in production with services which couldn't be restarted after a
>> crash after migrating to JDK 17.
>>
>> The fix of the issue is rather trivial. `jspawnhelper` has to close its
>> writing end of the pipe which connects it with the parent Java process
>> *before* starting to read from that pipe such that reads from the pipe can
>> immediately return with EOF if the parent process terminates abnormally.
>>
>> Also did some cleanup:
>> - in `jspawnhelper.c` correctly chek the result of `readFully()`
>> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
>> write the command data from the parent process to `jspawnhelper`
>> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
>> that's long gone.
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix for failing CloseRace test
Shaping up nicely, thanks for the careful attention to detail.
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13956#pullrequestreview-1446800955
Re: [External] : Re: Classes used in method body are loaded lazily or eagerly depending on method return type
My bad.
In the modifications of the original code, I proposed to replace
URL classFileB =
TestLoading.class.getResource(TestLoading.class.getSimpleName() +
"$ChildClass.class");
with
URL classFileB =
TestLoading.class.getResource(ChildClass.class.getSimpleName() + ".class");
However, this loads ChildClass just a few lines *before* it is removed
from the filesystem, so later uses of the class in the same run, in
particular during verification of method BaseClassReturner.getObject(),
are immune to the removal.
Replacing with
URL classFileB =
TestLoading.class.getResource(TestLoading.class.getPackageName() +
"ChildClass.class");
will remove the class without loading it, and will later throw, as in
the original setup.
Sorry for the noise
Raffaello
On 2023-05-26 08:10, David Holmes wrote:
On 25/05/2023 7:21 pm, Raffaello Giulietti wrote:
Yes, ChildClass.class is removed from the filesystem.
And here's, the relevant info when running with -Xlog:class+init,
showing that verification succeeds for both TestLoading$ObjectReturner
and TestLoading$BaseClassReturner:
loading: TestLoading$ObjectReturner...
[0.039s][info][class,init] Start class verification for:
TestLoading$ObjectReturner
[0.039s][info][class,init] End class verification for:
TestLoading$ObjectReturner
[0.039s][info][class,init] 500 Initializing
'TestLoading$ObjectReturner'(no method) (0x000801001800)
loading: TestLoading$BaseClassReturner...
[0.039s][info][class,init] Start class verification for:
TestLoading$BaseClassReturner
[0.039s][info][class,init] End class verification for:
TestLoading$BaseClassReturner
[0.039s][info][class,init] 501 Initializing
'TestLoading$BaseClassReturner'(no method) (0x000801001a08)
Can you enable -xlog:verification and class+load too please.
Thanks,
David
-
On 2023-05-25 04:57, David Holmes wrote:
On 25/05/2023 12:34 am, Raffaello Giulietti wrote:
As mentioned in my previous email, if you move the member class
ChildClass out of TestLoading (out of the nest), and make it a
top-level class like
public class ChildClass extends TestLoading.BaseClass {
}
and change
URL classFileB =
TestLoading.class.getResource(TestLoading.class.getSimpleName() +
"$ChildClass.class");
to
URL classFileB =
TestLoading.class.getResource(ChildClass.class.getSimpleName() +
".class");
rebuild everything and run, nothing is thrown.
deleting: /ChildClass.class
loading: TestLoading$ObjectReturner...
loading: TestLoading$BaseClassReturner...
I can't see any substantial difference, except that the nest rooted
at TestLoading lacks a member in the original setup and lacks
nothing in this setup.
What's an explanation for this difference?
Are you sure it actually deletes the file? What do you see when you
enable class+load/init and verification logging?
David
-
Greetings
Raffaello
On 2023-05-24 00:35, Remi Forax wrote:
- Original Message -
From: "David Holmes"
To: "Raffaello Giulietti" ,
"core-libs-dev"
Sent: Wednesday, May 24, 2023 12:23:24 AM
Subject: Re: Classes used in method body are loaded lazily or
eagerly depending on method return type
On 24/05/2023 12:50 am, Raffaello Giulietti wrote:
I think the problem here is that you are deleting a class in a nest.
During the verification of BaseClassReturner.getObject(), access
control
(JVMS, 5.4.4, second half) determines that the nest is broken, as
ChildClass is not present anymore.
Not sure access control gets involved at this stage of the
verification
process. But in any case turning on logging does not show anything
related to nestmates happening between BaseClass and ChildClass. It
seems to just be the resolution of the return type during
verification
of the method, that causes the loading of ChildClass and the
subsequent
CNFE if it has been removed.
If you move ChildClass out of TestLoading so that it becomes a
top-level
class extending TestLoading.BaseClass, and if you adapt the line
that
initializes the local var classFileB to refer to the new
location, the
code will not throw, despite ChildClass being deleted.
My simplified test shows it still throws when verifying
BaseClassReturner.
Nestmate checking is done lazily, so if you do not call a
method/access a field of a nested class, the VM should not trigger
a class loading.
Moreover, if you test with Java 8 (nestmates were introduced in
Java 11), it failed too.
That's another clue that the error is not related to nestmates
checking.
Cheers,
David
regards,
Rémi
Greetings
Raffaello
On 2023-05-23 13:20, Сергей Цыпанов wrote:
Hello,
originally this question was asked here:
https://urldefense.com/v3/__https://stackoverflow.com/q/76260269/12473843__;!!ACWV5N9M2RV99hQ!Mb5nhj7EbuftWzF7s4GX9auUZZlyPyCUnLs64c4mkmSGJm4pw0CNgRzQR5wOYuApyE_kHSAnVxGyTM9PHz5StCppGw$
,
the code sample for reproduction will be put below or can be
found via
the link
The
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v2]
On Thu, 25 May 2023 15:31:43 GMT, Maurizio Cimadamore wrote: >> As the FFM API evolved over time, some parts of the javadoc went out of >> sync. Now that most of the API is stable, it is a good time to look again at >> the javadoc as a whole, and bring some more consistency. >> >> While most of the changes in this PR are stylistic, I'd like to call out few >> things which resulted in API changes: >> >> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified >> requirement that its alignment parameter must be a power of two. >> >> * `MemoryLayout::sliceHandle` did not require the absence of dereference >> paths in the provided layout path. While that is technically possible to >> allow, currently the method is specified as returning a "slice" >> corresponding to some "nested layout", so following pointers seems >> incompatible with the spec. I have decided to disallow for now - we can >> always compatibly enable it later, if required. >> >> * `MemorySegment::copy` - most of the overloads did not specify that >> `UnsupportedOperationException` is thrown if the destination segment is >> read-only. >> >> * In several places, an extra `@throws IllegalArgumentException` or `@throws >> IllegalArgumentException` has been added to capture cases where element size >> * index computation can overflow. This is true for all the element-wise >> segment copy methods, for the indexed accessors in memory segment (e.g. >> `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for >> `SegmentAllocator::allocateArray(MemoryLayout, long)`. >> >> In all these cases (except for overflows), new tests have been added to >> cover the additional API changes (a CSR will also be filed to cover these). >> >> The class with most changes is `MemoryLayout`. I realized that the javadoc >> there was a bit sloppy around the definition of "layout paths". Now there >> are several subsection in the class javadoc, which explain how different >> kinds of paths can be used. I have introduced the notion of "open path >> elements" to denote those path elements that are not fully specified, and >> result in additional var handle coordinates to be added. There is also now a >> definition of what it means for a layout path to be "well-formed", so that >> all methods accepting a layout path can simply refer to it (this definition >> is tricky to give inline in the javadoc of the various path-accepting >> methods, as it depends on many factors). >> >> Another biggie change was in `MemorySegment` as now all dereference methods >> state precisely their spatial checks requirements, as well also specifying >> when the API can th... > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow I brought over some javadoc improvements from https://github.com/openjdk/jdk/pull/14037 (which has been withdrawn). These improvements should allow us to enable support for Linker in x86 platforms, as they define the notion of "what is a linker supported layout" much more precisely, and in a way that is not dependent on natural alignment (for value layouts). - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1564747719
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v3]
> As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Improve javadoc on supported linker layouts - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/df6c1239..6703eee9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14098=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14098=01-02 Stats: 69 lines in 3 files changed: 45 ins; 3 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
On Wed, 24 May 2023 09:24:46 GMT, Rudi Horn wrote: > This change prevents the contents of the internal string array from being > copied back when releasing it. There was an unrelated change to these functions recently. You might want to rebase this PR with the latest version. - PR Comment: https://git.openjdk.org/jdk/pull/14117#issuecomment-1564712601
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v6]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Simplify clinit
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14135/files
- new: https://git.openjdk.org/jdk/pull/14135/files/47375313..75455ca2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14135=05
- incr: https://webrevs.openjdk.org/?repo=jdk=14135=04-05
Stats: 6 lines in 1 file changed: 0 ins; 4 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v7]
On Thu, 25 May 2023 15:25:40 GMT, Volker Simonis wrote:
>> Since JDK13, executing commands in a sub-process defaults to the so called
>> `POSIX_SPAWN` launching mechanism (i.e.
>> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
>> using `posix_spawn(3)` to firstly start a tiny helper program called
>> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
>> command data from the parent Java process over a Unix pipe and finally
>> executes (i.e. `execvp(3)`) the requested command.
>>
>> In cases where the parent process terminates abnormally before
>> `jspawnhelper` has read all the expected data from the pipe, `jspawnhelper`
>> will block indefinitely on the reading end of the pipe. This is especially
>> harmful if the parent process had open sockets, because in that case, the
>> forked `jspawnhelper` process will inherit them and keep all the
>> corresponding ports open effectively preventing other processes to bind to
>> them. Notice that this is not an abstract scenario. We've observed this
>> regularly in production with services which couldn't be restarted after a
>> crash after migrating to JDK 17.
>>
>> The fix of the issue is rather trivial. `jspawnhelper` has to close its
>> writing end of the pipe which connects it with the parent Java process
>> *before* starting to read from that pipe such that reads from the pipe can
>> immediately return with EOF if the parent process terminates abnormally.
>>
>> Also did some cleanup:
>> - in `jspawnhelper.c` correctly chek the result of `readFully()`
>> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
>> write the command data from the parent process to `jspawnhelper`
>> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
>> that's long gone.
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix for failing CloseRace test
> Fixing the last bug :)
>
> If we close the writing end of the pipe to jspawnhelper early in
> `spawnChild()` right after the last write (which I still think is a good
> idea), we have to use `c->childenv` rather than `childenv` when closing the
> pipe descriptors at the end of `Java_java_lang_ProcessImpl_forkAndExec()` in
> order to avoid double closing:
>
> ```c++
> -closeSafely(childenv[0]);
> -closeSafely(childenv[1]);
> +/* We use 'c->childenv' here rather than 'childenv' because
> 'spawnChild()' might have
> + * already closed 'c->childenv[1]' and signaled this by setting
> 'c->childenv[1]' to '-1'.
> + * Otherwise 'c->childenv' and 'childenv' are the same because we just
> copied 'childenv'
> + * to 'c->childenv' (with 'copyPipe()') before calling 'startChild()'. */
> +closeSafely(c->childenv[0]);
> +closeSafely(c->childenv[1]);
> ```
>
> This also fixes `test/jdk/java/lang/ProcessBuilder/CloseRace.java` which
> could failed sporadically the previous version of the change.
I missed that. Christ this stuff is complex :(
Still think it would be cleaner and simpler to set the FD in the parent to
CLOEXEC, before doing posix_spawn, and at the same time set the childStuff
variable to -1 to prevent the child from attempting to re-close it. Reconsider?
-
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1564700036
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v5]
On Fri, 26 May 2023 15:30:29 GMT, Roger Riggs wrote: >> Yes, I used them for testing, but left them in, because it would be >> convenient to have them around for field tuning and diagnostics. This would >> require a CSR, right? > > I don't think there is a strong case for adding the system properties. They > just add to the surface area that has to be maintained and add very little > value except in very rare cases. If someone needs very special treatment > they can build it outside of UUID and use the new UUID(long, long) > constructor. That sounds okay to me. It simplifies the whole thing and does not require me to implement more test and have a CSR discussion about the options too :) Removed in the new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1207103570
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v5]
On Fri, 26 May 2023 15:30:29 GMT, Roger Riggs wrote: >> Yes, I used them for testing, but left them in, because it would be >> convenient to have them around for field tuning and diagnostics. This would >> require a CSR, right? > > I don't think there is a strong case for adding the system properties. They > just add to the surface area that has to be maintained and add very little > value except in very rare cases. If someone needs very special treatment > they can build it outside of UUID and use the new UUID(long, long) > constructor. That sounds okay to me. It simplifies the whole thing and does not require me to implement more test and have a CSR discussion about the options too :) Removed in the new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1207103570
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v5]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Remove the properties
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14135/files
- new: https://git.openjdk.org/jdk/pull/14135/files/4e9503d1..47375313
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14135=04
- incr: https://webrevs.openjdk.org/?repo=jdk=14135=03-04
Stats: 42 lines in 1 file changed: 0 ins; 37 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Thu, 25 May 2023 18:18:43 GMT, Martin Doerr wrote:
>> src/hotspot/share/utilities/globalDefinitions_xlc.hpp line 47:
>>
>>> 45: #undef malloc
>>> 46: extern void *malloc(size_t) asm("vec_malloc");
>>> 47: #endif
>>
>> Wow! And I don't mean that in a good way. I'm not questioning whether this
>> is correct, just commenting
>> on how crazy it seems that such a thing might be needed.
>
> The crazy thing is that `malloc` is defined! That means all places where we
> use the term malloc are getting replaced without such a workaround. (E.g. for
> log tags.)
So, we do this only for malloc? Not for calloc, posix_memalign, realloc etc?
What about free?
As ugly as defining malloc is (and I remember QADRT), I hesitate about removing
that define.
Removing that define and hard-coding it here assumes 1) our replacement is
equivalent (ok, easy to check) 2) it will always be equivalent in future AIX
versions 3) pointers it returns work with the unchanged free() and realloc()
the system provides, and will always do so.
I don't know... I would not do this just to get rid of a warning.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1207078176
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes [v2]
> Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Rename jdk.internal.util.Version to OSVersion - Changes: - all: https://git.openjdk.org/jdk/pull/14181/files - new: https://git.openjdk.org/jdk/pull/14181/files/f6332cd9..22e022c0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14181=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14181=00-01 Stats: 298 lines in 5 files changed: 135 ins; 136 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/14181.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14181/head:pull/14181 PR: https://git.openjdk.org/jdk/pull/14181
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 16:16:58 GMT, Mandy Chung wrote: > Alternatively, move `Version` record as a nested class in `OperatingSystem`. That would work WRT naming, but I thought the point was to not have to load OperatingSystem in order to load Version? Unless loading a static inner class doesn't force the loading of the outer class? - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564631561
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. For the time being, I think they should be independent and clearly named. The nesting creates a very unwieldy and long name. (Not withstanding imports) - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564640242
Re: RFR: 8308293: A linker should expose the layouts it supports [v6]
On Wed, 24 May 2023 09:36:34 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into linker_types
> - Drop "unsigned short"
>Beef up javadoc
> - Address further review comments
> - Address review comments
> - More javadoc tweaks
> - Tweak javadoc
> - Tweak javadoc
>Add char type
> - Initial push
Thanks for taking the time to review. After some more consideration, I will
withdraw this PR. While this API is largely not problematic, we need to make
sure that this API fits with how the FFM API will be evolved to support other
types besides the C basic types we know and love (e.g. `long double`, vectors,
`complex` types). So adding this API point now seems premature.
I will bring over relevant javadoc improvements in the other javadoc PR I have
open: https://github.com/openjdk/panama-foreign/pull/833
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1564641545
Withdrawn: 8308293: A linker should expose the layouts it supports
On Wed, 17 May 2023 17:15:06 GMT, Maurizio Cimadamore
wrote:
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. I mean static member class of `OperatingSystem`. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564637892
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang 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 eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported I'm aware of these PRs. I'll get to it when I get to it. - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1564639138
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 15:32:20 GMT, Roger Riggs wrote: > Should we take this opportunity to rename `jdk.internal.util.Version` into > something like `OsVersion` to make it more clear what its `current()` method > actually returns? Alternatively, move `Version` record as a nested class in `OperatingSystem`. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564625922
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 14:57:21 GMT, Roger Riggs wrote: > Decouple the jdk.internal.util OperatingSystem and Version classes to > simplify class loading and avoid an indirect cyclic initialization. > > Move the method to get the current OS version from OperatingSystem to the > Version class and its initialization. > Revert to using String.toUpperCase() instead of custom code to uppercase. > > Update the uses of OperatingSystem.version to be Version.current. Looks good. Thanks for fixing this. It will allow ClassFileDumper to use `Path`. - Marked as reviewed by mchung (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14181#pullrequestreview-1446540821
Re: RFR: 8306431: File.listRoots method description should be re-examined [v7]
On Fri, 26 May 2023 08:46:09 GMT, Nagata-Haruhito wrote: > Would you please review this PR ? We are in the last two weeks before JDK 21 rampdown phase 1 and so are rather busy. This PR has not been forgotten. Thanks for you patience. - PR Comment: https://git.openjdk.org/jdk/pull/13526#issuecomment-1564601445
Re: RFR: 8290499: new File(parent, "/") breaks normalization – creates File with slash at the end
On Tue, 23 May 2023 22:49:57 GMT, Brian Burkhalter wrote: > In `java.io.File`, change the constructors `File(File,String)` and > `File(String,String)` so that they do not for typical cases return a `File` > whose path has a trailing name separator. LGTM - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14109#pullrequestreview-1446514781
Re: RFR: 8308452: Extend internal Architecture enum with byte order and address size [v2]
> The internal enum jdk.internal.util.Architecture does not provide information > about the big or little endianness or the address size (64 or 32 bits). The > endian-ness and address size are intrinsic to the architecture. > > The values of the enum are extended to separately identify the big endian and > little-endian uses of the ISA. > For example, `PPC64` and `PPC64LE` for the big and little-endian versions. > The enum values directly reflect the build-time artifacts and resulting > executables. > > This information about an architecture will make the enum more useful > especially to identify a target platform in a cross-platform use case. A > method is added to map well known aliases for the platforms to the > Architecture enum. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Review suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/14063/files - new: https://git.openjdk.org/jdk/pull/14063/files/e9c8d89b..b384396b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14063=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14063=00-01 Stats: 29 lines in 3 files changed: 6 ins; 19 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14063.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14063/head:pull/14063 PR: https://git.openjdk.org/jdk/pull/14063
RFR: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
This change prevents the contents of the internal string array from being copied back when releasing it. - Commit messages: - Use JNI_ABORT to release string bytes Changes: https://git.openjdk.org/jdk/pull/14117/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14117=00 Issue: https://bugs.openjdk.org/browse/JDK-8308748 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14117.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14117/head:pull/14117 PR: https://git.openjdk.org/jdk/pull/14117
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v4]
On Thu, 25 May 2023 14:04:35 GMT, Aleksey Shipilev wrote:
>> src/java.base/share/classes/java/util/UUID.java line 149:
>>
>>> 147: } else {
>>> 148: try {
>>> 149: return SecureRandom.getInstance(PRNG_NAME);
>>
>> Part of the change here is that the RNG algorithm is configurable via a
>> system property. The naming of the naming (java.util.UUID.prngName) hints
>> that you might want it to be a standard system property but maybe it's for
>> testing?
>
> Yes, I used them for testing, but left them in, because it would be
> convenient to have them around for field tuning and diagnostics. This would
> require a CSR, right?
I don't think there is a strong case for adding the system properties. They
just add to the surface area that has to be maintained and add very little
value except in very rare cases. If someone needs very special treatment they
can build it outside of UUID and use the new UUID(long, long) constructor.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206962862
Re: RFR: 8308960: Decouple internal Version and OperatingSystem classes
On Fri, 26 May 2023 15:21:42 GMT, Daniel Fuchs wrote: > Should we take this opportunity to rename `jdk.internal.util.Version` into > something like `OsVersion` to make it more clear what its `current()` method > actually returns? ok, it will give a bit more flavor; there are already multiple version classes with little to distinguish them. - PR Comment: https://git.openjdk.org/jdk/pull/14181#issuecomment-1564567712
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v3]
On Fri, 26 May 2023 12:01:50 GMT, Andrei Pangin wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fine-tune exceptions > > src/java.base/share/classes/java/util/UUID.java line 226: > >> 224: // set variant to IETF >> 225: msb = (msb & (0x3FFF___L)) | >> 0x8000___L; >> 226: return new UUID(lsb, msb); > > Doesn't `msb` [come > first](https://github.com/openjdk/jdk/blob/deddaa6ea61f85e9822982a37dad51ff4310457b/src/java.base/share/classes/java/util/UUID.java#L314)? Yes. Swapped. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206925415
RFR: 8308960: Decouple internal Version and OperatingSystem classes
Decouple the jdk.internal.util OperatingSystem and Version classes to simplify class loading and avoid an indirect cyclic initialization. Move the method to get the current OS version from OperatingSystem to the Version class and its initialization. Revert to using String.toUpperCase() instead of custom code to uppercase. Update the uses of OperatingSystem.version to be Version.current. - Commit messages: - 8308960: Decouple internal Version and OperatingSystem classes Changes: https://git.openjdk.org/jdk/pull/14181/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14181=00 Issue: https://bugs.openjdk.org/browse/JDK-8308960 Stats: 58 lines in 4 files changed: 24 ins; 29 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14181.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14181/head:pull/14181 PR: https://git.openjdk.org/jdk/pull/14181
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v4]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request incrementally with one additional
commit since the last revision:
Swap lsb/msb
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14135/files
- new: https://git.openjdk.org/jdk/pull/14135/files/deddaa6e..4e9503d1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14135=03
- incr: https://webrevs.openjdk.org/?repo=jdk=14135=02-03
Stats: 13 lines in 1 file changed: 0 ins; 0 del; 13 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote: >> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk >> on AIX , we run into various "warnings as errors". >> Some of those are in shared codebase and could be addressed by small >> adjustments. >> A lot of those changes are in hotspot, some might be somewhere else in the >> OpenJDK C/C++ code. > > JoKern65 has updated the pull request incrementally with one additional > commit since the last revision: > > forgotton _ Just a side note: The warning elimination is new for AIX compilers. We had given it up at earlier times: https://bugs.openjdk.org/browse/JDK-8202325 I still appreciate if we can get warning free and it makes sense to review all of them. But, I wouldn't require it for JDK21. - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564406986
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v22]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Remove trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/4e54c17a..06aa43ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=21 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=20-21 Stats: 9 lines in 1 file changed: 0 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization [v3]
On Wed, 17 May 2023 17:20:37 GMT, Chen Liang wrote: >> Also fixed the bug with NPE in `IndirectVarHandle::isAccessModeSupported`. >> >> A few implementation-detail methods in VarHandle are now documented with the >> implied constraints to avoid subtle problems in the future. Changed >> `IndirectVarHandle` to call `asDirect` lazily to accomodate the lazy >> VarHandle changes. Also changed VarHandleBaseTest to report the whole >> incorrect type of exception caught than swallow it and leaving only a >> message. >> >> Current problems: >> - [ ] The lazy var handle is quite slow on the first invocation. >>- As seen in the benchmark, users can first call >> `Lookup::ensureInitialized` to create an eager handle. >>- After that, the lazy handle has a performance on par with the regular >> var handle. >> - [ ] The class-loading-based test is not in a unit test >>- The test frameworks don't seem to offer fine-grained control for >> class-loading detection or reliable unloading >> >> >> BenchmarkMode Cnt Score >> Error Units >> VarHandleLazyStaticInvocation.initializedInvocation avgt 30 0.769 ± >> 0.003 ns/op >> VarHandleLazyStaticInvocation.lazyInvocation avgt 30 0.767 ± >> 0.002 ns/op >> >> >> BenchmarkMode Cnt Score >> Error Units >> LazyStaticColdStart.methodHandleCreateEagerss 10 73140.100 ± >> 7735.276 ns/op >> LazyStaticColdStart.methodHandleCreateLazy ss 10 5.000 ± >> 8482.883 ns/op >> LazyStaticColdStart.methodHandleInitializeCallEagerss 10 91350.100 ± >> 9776.343 ns/op >> LazyStaticColdStart.methodHandleInitializeCallLazy ss 10 145540.200 ± >> 72894.865 ns/op >> LazyStaticColdStart.varHandleCreateEager ss 10 47069.900 ± >> 3513.909 ns/op >> LazyStaticColdStart.varHandleCreateLazyss 10 24780.300 ± >> 5144.540 ns/op >> LazyStaticColdStart.varHandleInitializeCallEager ss 10 85479.700 ± >> 10816.983 ns/op >> LazyStaticColdStart.varHandleInitializeCallLazyss 10 413630.100 ± >> 189370.448 ns/op > > Chen Liang 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 eight additional commits since > the last revision: > > - Remove export for removed package > - Merge branch 'master' into lazy-static-varhandle > - Test the creation performance of handles, lazy one indeed better > - Merge branch 'master' into lazy-static-varhandle > - copyright year > - Benchmarks. lazy initialize is SLOW > - nuke meaningless overrides > - make static field var handles lazy and fix NPE in isAccessModeSupported @mlchung Would you mind taking a look at this? - PR Comment: https://git.openjdk.org/jdk/pull/13821#issuecomment-1564386002
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v21]
> Add flexible main methods and anonymous main classes to the Java language. Jim Laskey has updated the pull request incrementally with one additional commit since the last revision: Add main tests for inferface/enum/record - Changes: - all: https://git.openjdk.org/jdk/pull/13689/files - new: https://git.openjdk.org/jdk/pull/13689/files/cfac821a..4e54c17a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13689=20 - incr: https://webrevs.openjdk.org/?repo=jdk=13689=19-20 Stats: 128 lines in 2 files changed: 27 ins; 22 del; 79 mod Patch: https://git.openjdk.org/jdk/pull/13689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13689/head:pull/13689 PR: https://git.openjdk.org/jdk/pull/13689
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v20]
On Fri, 26 May 2023 06:20:14 GMT, Rémi Forax wrote:
>> test/jdk/tools/launcher/InstanceMainTest.java line 31:
>>
>>> 29: * @run main InstanceMainTest
>>> 30: */
>>> 31: public class InstanceMainTest extends TestHelper {
>>
>> By my reading of the spec, "main" methods can be defined in record classes
>> and enum classes as well as normal classes.
>> Are there tests for record and enum main method operation?
>
> You can also have a `main` method inside an interface. For enum classes and
> interfaces, the main has to be static.
updating tests
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1206753443
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote:
>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> forgotton _
Here are the reasons for the disabled warnings in
make/modules/java.desktop/lib/Awt2dLibraries.gmk
/data/d042520/pr/jdk/src/java.desktop/unix/native/common/awt/awt_GraphicsEnv.h:53:12:
error: a function declaration without a prototype is deprecated in all
versions of C and is treated as a zero-parameter prototype in C2x, conflicting
with a previous declaration [-Werror,-Wdeprecated-non-prototype]
extern int XShmQueryExtension();
^
/usr/include/X11/extensions/XShm.h:91:6: note: conflicting prototype is here
Bool XShmQueryExtension(
^
solved by adding line (generic, because several source files are involved)
DISABLED_WARNINGS_clang_aix := deprecated-non-prototype, \
src/java.desktop/unix/native/libawt_xawt/xawt/awt_Taskbar.c:158:11: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (m = fp_unity_launcher_entry_get_quicklist(entry)) {
~~^~
solved by adding line
DISABLED_WARNINGS_clang_aix_awt_Taskbar.c := parentheses, \
src/java.desktop/share/native/common/java2d/opengl/OGLPaints.c:581:48: error:
format string is not a string literal [-Werror,-Wformat-nonliteral]
snprintf(cycleCode, sizeof(cycleCode), noCycleCode, texCoordCalcCode);
^~~
solved by adding line
DISABLED_WARNINGS_clang_aix_OGLPaints.c := format-nonliteral, \
src/java.desktop/share/native/common/java2d/opengl/OGLBufImgOps.c:153:48:
error: format string is not a string literal [-Werror,-Wformat-nonliteral]
snprintf(finalSource, sizeof(finalSource), convolveShaderSource,
^~~~
solved by adding line
DISABLED_WARNINGS_clang_aix_OGLBufImgOps.c := format-nonliteral, \
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c:1095:41: error:
'&&' within '||' [-Werror,-Wlogical-op-parentheses]
if ((synth_state & MOUSE_OVER) != 0 && (synth_state & PRESSED) == 0 ||
^~~ ~~
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c:1180:29: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (init_result = (NULL == gtk2_widgets[_GTK_CHECK_MENU_ITEM_TYPE]))
^~~
solved by adding line
DISABLED_WARNINGS_clang_aix_gtk2_interface.c := parentheses
logical-op-parentheses, \
src/java.desktop/unix/native/libawt_xawt/awt/gtk3_interface.c:903:29: error:
using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (init_result = (NULL == gtk3_widgets[_GTK_BUTTON_TYPE]))
^~
solved by adding line
DISABLED_WARNINGS_clang_aix_gtk3_interface.c := parentheses, \
src/java.desktop/unix/native/libawt_xawt/awt/sun_awt_X11_GtkFileDialogPeer.c:87:26:
error: using the result of an assignment as a condition without parentheses
[-Werror,-Wparentheses]
if (pendingException = (*env)->ExceptionOccurred(env)) {
~^~~~
solved by adding line
DISABLED_WARNINGS_clang_aix_sun_awt_X11_GtkFileDialogPeer.c := parentheses, \
src/java.desktop/aix/native/libawt_xawt/awt/awt_InputMethod.c:1969:32: error:
comparison of integers of different signs: 'Window' (aka 'unsigned long') and
'jlong' (aka 'long') [-Werror,-Wsign-compare]
if (currentFocusWindow != w) {
~~ ^ ~
solved by adding line
DISABLED_WARNINGS_clang_aix_awt_InputMethod.c := sign-compare, \
Here is the reason for the disabled warning in
make/modules/java.security.jgss/Lib.gmk
src/java.security.jgss/share/native/libj2gss/NativeUtil.h:30:
/data/d042520/pr/jdk/src/java.security.jgss/share/native/libj2gss/gssapi.h:48:5:
error: 'TARGET_OS_MAC' is not defined, evaluates to 0 [-Werror,-Wundef]
#if TARGET_OS_MAC && (defined(__ppc__) || defined(__ppc64__) ||
defined(__i386__) || defined(__x86_64__))
^
make/modules/java.security.jgss/Lib.gmk add line
DISABLED_WARNINGS_clang_aix := undef, \
Here is the reason for the disabled warning in
make/modules/jdk.jdwp.agent/Lib.gmk
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:718:33: error:
suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
struct in6_addr
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v3]
On Fri, 26 May 2023 11:43:11 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fine-tune exceptions
src/java.base/share/classes/java/util/UUID.java line 226:
> 224: // set variant to IETF
> 225: msb = (msb & (0x3FFF___L)) |
> 0x8000___L;
> 226: return new UUID(lsb, msb);
Doesn't `msb` [come
first](https://github.com/openjdk/jdk/blob/deddaa6ea61f85e9822982a37dad51ff4310457b/src/java.base/share/classes/java/util/UUID.java#L314)?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206680628
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v3]
On Fri, 26 May 2023 08:36:46 GMT, Aleksey Shipilev wrote:
>> src/java.base/share/classes/java/util/UUID.java line 224:
>>
>>> 222: // Try to pull the UUID from the current buffer at
>>> current position.
>>> 223: if (stamp != 0) {
>>> 224: int p = (int)VH_POS.getAndAdd(this,
>>> UUID_CHUNK);
>>
>> An atomic update together with an optimistic lock looks non-idiomatic use of
>> StampedLock to me. Won't a simple CAS loop be simpler? E.g. in pseudocode:
>>
>> while ((p = this.pos) + 16) < buf.length) {
>> long msb = getLong(buf, p);
>> long lsb = getLong(buf, p + 8);
>> if (cas(this.pos, p, p + 16)) {
>> return new UUID(msb, lsb);
>> }
>> }
>>
>> // refill buffer under lock
>
> Nope, I don't think you cannot do this, because there is a race on
> concurrently-updating `buf`. The StampedLock (RW lock), protects the
> buffer-reading threads from seeing the `buf` undergoing the initialization by
> buffer-writing threads. Atomic pos only arbitrates the concurrent
> buffer-threads. That atomic does not help to resolve the buf races.
Right, my example suffers from the ABA problem. It could be likely solved by
adding "generation" bits to the pos, but this will be a bit ugly. OK, let's
stick with the StampedLock.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206686499
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v2]
On Fri, 26 May 2023 11:24:10 GMT, Daniel Fuchs wrote:
>> Aleksey Shipilev has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Handle privileged properties
>> - Use ByteArray to convert. Do version/variant preparations straight on
>> locals. Move init out of optimistic lock section.
>
> src/java.base/share/classes/java/util/UUID.java line 144:
>
>> 142: BUFS = new Buffer[BUFS_COUNT];
>> 143: } catch (Exception e) {
>> 144: throw new InternalError(e);
>
> Would it be better to throw `ExceptionInInitializerError` here?
Sure, see new commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206653656
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v2]
On Fri, 26 May 2023 09:51:47 GMT, Aleksey Shipilev wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.823 ± 0.023 ops/us
>> UUIDRandomBench.max thrpt 15 6.561 ± 0.054 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.109 ± 0.026 ops/us
>> UUIDRandomBench.max thrpt 15 3.561 ± 0.071 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> Aleksey Shipilev has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Handle privileged properties
> - Use ByteArray to convert. Do version/variant preparations straight on
> locals. Move init out of optimistic lock section.
src/java.base/share/classes/java/util/UUID.java line 144:
> 142: BUFS = new Buffer[BUFS_COUNT];
> 143: } catch (Exception e) {
> 144: throw new InternalError(e);
Would it be better to throw `ExceptionInInitializerError` here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206636006
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 10:18:37 GMT, JoKern65 wrote:
> Here are the reasons for the disabled warnings in
> make/modules/java.base/lib/CoreLibraries.gmk
> DISABLED_WARNINGS_clang_aix_ProcessHandleImpl_unix.c := sign-compare,
> DISABLED_WARNINGS_clang_aix := gnu-pointer-arith, DISABLED_WARNINGS_clang :=
> gnu-pointer-arith format-nonliteral deprecated-non-prototype, \
>
> src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:638 comparison of
> integers of different signs: 'int' and 'unsigned long' if (ret <
> sizeof(psinfo_t)) { ^
>
`fread` returns a `size_t ` on Linux and AIX, could you please check Mac/BSD
too ?
https://github.com/openjdk/jdk/blob/d3b9b364da8c11c9b4dd14a6451a7b24f41202e7/src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c#L636
We should probably change to size_t ret (from type int), then the warning
would not occur, correct ?
-
PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564207011
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 10:18:37 GMT, JoKern65 wrote: > src/java.base/share/native/libjli/java.c:2311:22: error: format string is not > a string literal [-Werror,-Wformat-nonliteral] vfprintf(stderr, fmt, vl); ^~~ We disable this warning too for clang on all platforms in BUILD_LIBJLI ( DISABLED_WARNINGS_clang := **format-nonlitera**l deprecated-non-prototype, \ ), so I think we should do it the same way for BUILD_LIBJLI_STATIC on AIX. - PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564193886
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Fri, 26 May 2023 08:31:46 GMT, JoKern65 wrote:
>> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
>> on AIX , we run into various "warnings as errors".
>> Some of those are in shared codebase and could be addressed by small
>> adjustments.
>> A lot of those changes are in hotspot, some might be somewhere else in the
>> OpenJDK C/C++ code.
>
> JoKern65 has updated the pull request incrementally with one additional
> commit since the last revision:
>
> forgotton _
Here are the reasons for the disabled warnings in
make/modules/java.base/lib/CoreLibraries.gmk
DISABLED_WARNINGS_clang_aix_ProcessHandleImpl_unix.c := sign-compare, \
DISABLED_WARNINGS_clang_aix := gnu-pointer-arith, \
DISABLED_WARNINGS_clang := gnu-pointer-arith format-nonliteral
deprecated-non-prototype, \
src/java.base/unix/native/libjava/ProcessHandleImpl_unix.c:638 comparison of
integers of different signs: 'int' and 'unsigned long'
if (ret < sizeof(psinfo_t)) {
^
src/java.base/aix/native/libjli/java_md_aix.c:42:38: error: arithmetic on a
pointer to void is a GNU extension [-Werror,-Wgnu-pointer-arith]
addr < p->ldinfo_textorg + p->ldinfo_textsize) {
~ ^
src/java.base/share/native/libzip/zlib/inffast.c:74:20: error: a function
definition without a prototype is deprecated in all versions of C and is not
supported in C2x [-Werror,-Wdeprecated-non-prototype]
void ZLIB_INTERNAL inflate_fast(strm, start)
^
src/java.base/share/native/libjli/java.c:2311:22: error: format string is not a
string literal [-Werror,-Wformat-nonliteral]
vfprintf(stderr, fmt, vl);
^~~
-
PR Comment: https://git.openjdk.org/jdk/pull/14146#issuecomment-1564165195
Re: RFR: 8308803: Improve java/util/UUID/UUIDTest.java
On Wed, 24 May 2023 19:18:33 GMT, Aleksey Shipilev wrote: > UUID is very important class that is used to track identities of objects in > large scale systems. Yet, the coverage in JDK test is disappointing: it tests > only 100 of UUID instances per test, which is way too small to detect > collisions due to the bad randomness for example. > > I have some pending work to improve UUID performance, and tests should be > improved. > > The improved test still runs in less than 5 seconds on my laptop. Any takers? Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/14134#issuecomment-1564143336
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v2]
On Fri, 26 May 2023 08:37:49 GMT, Aleksey Shipilev wrote: >> `jdk.internal.util.ByteArray` has VarHandle-based methods ready for that > > Yes, I have that optimization in the pipeline, and wanted to do so > separately. I can still fold it here, let me see. Did so in new commit. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206511880
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v2]
On Thu, 25 May 2023 12:17:27 GMT, Alan Bateman wrote:
>> Aleksey Shipilev has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Handle privileged properties
>> - Use ByteArray to convert. Do version/variant preparations straight on
>> locals. Move init out of optimistic lock section.
>
> src/java.base/share/classes/java/util/UUID.java line 127:
>
>> 125: static {
>> 126: try {
>> 127: PRNG_NAME = System.getProperty(PROP_NAME_PRNG_NAME,
>> null);
>
> You'll have to use GetPropertyAction.privilegedGetProperty as this will
> otherwise fail if someone runs with a SM.
Good spot. Should be fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206512318
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access [v2]
> UUID is the very important class that is used to track identities of objects
> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
> of total CPU time, and is frequently a scalability bottleneck due to
> `SecureRandom` synchronization.
>
> The major issue with UUID code itself is that it reads from the single
> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
> is bashed with very small requests. This also has a chilling effect on other
> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>
> We can improve this by doing the bulk reads from the backing SecureRandom and
> possibly striping the reads across many instances of it.
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>
> # After
> UUIDRandomBench.single thrpt 15 4.421 ± 0.047 ops/us
> UUIDRandomBench.max thrpt 15 6.658 ± 0.092 ops/us ; positive scaling,
> ~1.5x
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before
> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative scaling
>
> # After
> BenchmarkMode Cnt Score Error Units
> UUIDRandomBench.single thrpt 15 3.099 ± 0.022 ops/us
> UUIDRandomBench.max thrpt 15 3.555 ± 0.062 ops/us ; positive
> scaling, ~1.2x
>
>
> Note that there is still a scalability bottleneck in current default random
> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
> out the synchronization story there. The scalability fix in current default
> `SecureRandom` would be much more intrusive and risky, since it would change
> a core crypto class with unknown bug fanout.
>
> Using the bulk reads even when the underlying PRNG is heavily synchronized is
> still a win. A more scalable PRNG would benefit from this as well. This PR
> adds a system property to select the PRNG implementation, and there we can
> clearly see the benefit with more scalable PRNG sources:
>
>
> Benchmark Mode Cnt Score Error Units
>
> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>
> # Before, hacked `new SecureRandom()` to
> `SecureRandom.getInstance("SHA1PRNG")`
> UUIDRandomBench.single thrpt 15 3.661 ± 0.008 ops/us
> UUIDRandomBench...
Aleksey Shipilev has updated the pull request incrementally with two additional
commits since the last revision:
- Handle privileged properties
- Use ByteArray to convert. Do version/variant preparations straight on
locals. Move init out of optimistic lock section.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14135/files
- new: https://git.openjdk.org/jdk/pull/14135/files/51dc2903..be38dffe
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14135=01
- incr: https://webrevs.openjdk.org/?repo=jdk=14135=00-01
Stats: 211 lines in 2 files changed: 103 ins; 21 del; 87 mod
Patch: https://git.openjdk.org/jdk/pull/14135.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14135/head:pull/14135
PR: https://git.openjdk.org/jdk/pull/14135
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Fri, 26 May 2023 08:39:10 GMT, Aleksey Shipilev wrote: >> src/java.base/share/classes/java/util/UUID.java line 260: >> >>> 258: buf[c + 8] &= 0x3f; /* clear variant*/ >>> 259: buf[c + 8] |= (byte) 0x80; /* set to IETF >>> variant */ >>> 260: } >> >> I'm not sure I understand the point about initialization. Why not just >> setting the required version bits right in UUID constructor without updating >> the array? This will be faster and simpler IMO. > > Right, we can do things straight on locals, without ever touching the heap, > let's see... Yup, that works beautifully. - PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206499212
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Fri, 26 May 2023 00:16:19 GMT, Andrei Pangin wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.421 ± 0.047 ops/us
>> UUIDRandomBench.max thrpt 15 6.658 ± 0.092 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.099 ± 0.022 ops/us
>> UUIDRandomBench.max thrpt 15 3.555 ± 0.062 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> src/java.base/share/classes/java/util/UUID.java line 226:
>
>> 224: int p = (int)VH_POS.getAndAdd(this, UUID_CHUNK);
>> 225: if (p < BUF_SIZE) {
>> 226: uuid = new UUID(buf, p);
>
> We can read msb/lsb from the buffer here and move object allocation outside
> the lock to reduce the length of the critical section.
Yes, I am doing this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206496030
Re: RFR: 8308842: Consolidate exceptions thrown from Class-File API [v2]
On Thu, 25 May 2023 22:45:01 GMT, ExE Boss wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed javadoc
>
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 389:
>
>> 387: // Check for empty try block
>> 388: if (tryBlock.isEmpty()) {
>> 389: throw new IllegalArgumentException("The body of the try
>> block is empty");
>
> This exception should be documented in the method’s **Javadoc**.
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14143#discussion_r1206483576
Re: RFR: 8306431: File.listRoots method description should be re-examined [v7]
On Mon, 22 May 2023 07:27:00 GMT, Nagata-Haruhito wrote: >> I fixed File.listRoots description. >> * remove "the insertion or ejection of removable media" >> * change "available" to "existing" >> >> Please review this change. > > Nagata-Haruhito has updated the pull request incrementally with one > additional commit since the last revision: > > Move SecurityException paragraph Would you please review this PR ? - PR Comment: https://git.openjdk.org/jdk/pull/13526#issuecomment-1564029175
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Fri, 26 May 2023 06:47:29 GMT, Hannes Greule wrote:
>> src/java.base/share/classes/java/util/UUID.java line 286:
>>
>>> 284: long lsb = 0;
>>> 285: for (int i = start; i < start + 8; i++) {
>>> 286: msb = (msb << 8) | (data[i] & 0xff);
>>
>> Can we use VarHandle/ByteBuffer to read 64 bits at a time?
>
> `jdk.internal.util.ByteArray` has VarHandle-based methods ready for that
Yes, I have that optimization in the pipeline, and wanted to do so separately.
I can still fold it here, let me see.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206431718
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Fri, 26 May 2023 00:50:04 GMT, Brett Okken wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.421 ± 0.047 ops/us
>> UUIDRandomBench.max thrpt 15 6.658 ± 0.092 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.099 ± 0.022 ops/us
>> UUIDRandomBench.max thrpt 15 3.555 ± 0.062 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> src/java.base/share/classes/java/util/UUID.java line 255:
>
>> 253: // initializations, and thus false sharing between
>> reader threads.
>> 254: random.nextBytes(buf);
>> 255: for (int c = 0; c < BUF_SIZE; c += UUID_CHUNK) {
>
> I think this could be faster by using a ByteBuffer (or VarHandle) to process
> as longs.
>
> https://mail.openjdk.org/pipermail/core-libs-dev/2023-March/101249.html
Yup, but that would probably fold into discussion here:
https://github.com/openjdk/jdk/pull/14135#discussion_r1206097261
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206434408
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Fri, 26 May 2023 00:13:57 GMT, Andrei Pangin wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.421 ± 0.047 ops/us
>> UUIDRandomBench.max thrpt 15 6.658 ± 0.092 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.099 ± 0.022 ops/us
>> UUIDRandomBench.max thrpt 15 3.555 ± 0.062 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> src/java.base/share/classes/java/util/UUID.java line 224:
>
>> 222: // Try to pull the UUID from the current buffer at
>> current position.
>> 223: if (stamp != 0) {
>> 224: int p = (int)VH_POS.getAndAdd(this, UUID_CHUNK);
>
> An atomic update together with an optimistic lock looks non-idiomatic use of
> StampedLock to me. Won't a simple CAS loop be simpler? E.g. in pseudocode:
>
> while ((p = this.pos) + 16) < buf.length) {
> long msb = getLong(buf, p);
> long lsb = getLong(buf, p + 8);
> if (cas(this.pos, p, p + 16)) {
> return new UUID(msb, lsb);
> }
> }
>
> // refill buffer under lock
Nope, I don't think you cannot do this, because there is a race on
concurrently-updating `buf`. The StampedLock (RW lock), protects the
buffer-reading threads from seeing the `buf` undergoing the initialization by
buffer-writing threads. Atomic pos only arbitrates the concurrent
buffer-threads. That atomic does not help to resolve the buf races.
> src/java.base/share/classes/java/util/UUID.java line 260:
>
>> 258: buf[c + 8] &= 0x3f; /* clear variant*/
>> 259: buf[c + 8] |= (byte) 0x80; /* set to IETF
>> variant */
>> 260: }
>
> I'm not sure I understand the point about initialization. Why not just
> setting the required version bits right in UUID constructor without updating
> the array? This will be faster and simpler IMO.
Right, we can do things straight on locals, without ever touching the heap,
let's see...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206430447
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206433588
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk > on AIX , we run into various "warnings as errors". > Some of those are in shared codebase and could be addressed by small > adjustments. > A lot of those changes are in hotspot, some might be somewhere else in the > OpenJDK C/C++ code. JoKern65 has updated the pull request incrementally with one additional commit since the last revision: forgotton _ - Changes: - all: https://git.openjdk.org/jdk/pull/14146/files - new: https://git.openjdk.org/jdk/pull/14146/files/2699fdce..da38fb2d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14146=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14146=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14146.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14146/head:pull/14146 PR: https://git.openjdk.org/jdk/pull/14146
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code
On Fri, 26 May 2023 07:12:07 GMT, Matthias Baesken wrote: >> This is IBMs declaration of statfs >> `extern int statfs(char *, struct statfs *);` >> So the compiler will not accept a `const char*` >> Indeed I do not know if this ever worked, but my change makes it not worse. > > Here is the documentation of statfs on AIX > https://www.ibm.com/docs/en/aix/7.2?topic=s-statfs-fstatfs-statfs64-fstatfs64-ustat-subroutine > (showing the IBM declaration Joachim told us) . > Also, pre-existing, the cast seems really suspicious. The type of `chars` is > `jchar*`, which is a sequence of 16bit characters. Does this actually work? > If so, how? Probably a jchar to char conversion would be needed for the array elements, maybe like this here (or is there a better utility function in the codebase) ? See jCharArrayToCKCharArray https://github.com/openjdk/jdk/blob/199b1bf5009120efd1fd37a1ddabc0c6fb84f62c/src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_util.c#L644 I am not aware of an AIX statfs for wchars but maybe I miss something. But it is a separate issue of Java_GetXSpace_getSpace0 anyway and should be handled in another bug. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1206370114
Re: RFR: 8308842: Consolidate exceptions thrown from Class-File API [v2]
> Class-File API actually throws wide variety of exceptions based on the actual > situation. Complete error handling code must cover > `IndexOutOfBoundsException`, `IllegalStateException` and > `IllegalArgumentException`. > > Based on previous discussions we decided to consolidate on > `IllegalArgumentException` with possible sub-classes. > > It allows easy common error handling in majority of use case, however it > allows also to distinguish source of the error when needed (for example > `javap` needs to know if the exception comes from constant poll or not). > > Newly introduced `ConstantPoolException` extends `IllegalArgumentException` > to indicate the source of the problem is in constant pool. > > Please review. > > Thanks, > Adam Adam Sotona has updated the pull request incrementally with one additional commit since the last revision: fixed javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/14143/files - new: https://git.openjdk.org/jdk/pull/14143/files/d2ec1c99..8bc444df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14143=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14143=00-01 Stats: 6 lines in 1 file changed: 1 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/14143.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14143/head:pull/14143 PR: https://git.openjdk.org/jdk/pull/14143
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote: > When using the clang compiler to build OpenJDk on Linux, we encounter various > "warnings as errors". > They can be fixed with small changes. According to our docs, [clang is a supported compiler for Linux](https://github.com/openjdk/jdk/blob/master/doc/building.md#native-compiler-toolchain-requirements). Here's an example how warning exclusion is implemented today: https://github.com/openjdk/jdk/blob/master/make/modules/java.desktop/lib/Awt2dLibraries.gmk#L827 - PR Comment: https://git.openjdk.org/jdk/pull/14033#issuecomment-1563956337
Re: RFR: 8308804: Improve UUID.randomUUID performance with bulk/scalable PRNG access
On Thu, 25 May 2023 23:54:14 GMT, Andrei Pangin wrote:
>> UUID is the very important class that is used to track identities of objects
>> in large scale systems. On some of our systems, `UUID.randomUUID` takes >1%
>> of total CPU time, and is frequently a scalability bottleneck due to
>> `SecureRandom` synchronization.
>>
>> The major issue with UUID code itself is that it reads from the single
>> `SecureRandom` instance by 16 bytes. So the heavily contended `SecureRandom`
>> is bashed with very small requests. This also has a chilling effect on other
>> users of `SecureRandom`, when there is a heavy UUID generation traffic.
>>
>> We can improve this by doing the bulk reads from the backing SecureRandom
>> and possibly striping the reads across many instances of it.
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### AArch64 (m6g.4xlarge, Graviton, 16 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 3.545 ± 0.058 ops/us
>> UUIDRandomBench.max thrpt 15 1.832 ± 0.059 ops/us ; negative scaling
>>
>> # After
>> UUIDRandomBench.single thrpt 15 4.421 ± 0.047 ops/us
>> UUIDRandomBench.max thrpt 15 6.658 ± 0.092 ops/us ; positive
>> scaling, ~1.5x
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before
>> UUIDRandomBench.single thrpt 15 2.710 ± 0.038 ops/us
>> UUIDRandomBench.max thrpt 15 1.880 ± 0.029 ops/us ; negative
>> scaling
>>
>> # After
>> BenchmarkMode Cnt Score Error Units
>> UUIDRandomBench.single thrpt 15 3.099 ± 0.022 ops/us
>> UUIDRandomBench.max thrpt 15 3.555 ± 0.062 ops/us ; positive
>> scaling, ~1.2x
>>
>>
>> Note that there is still a scalability bottleneck in current default random
>> (`NativePRNG`), because it synchronizes over a singleton instance for SHA1
>> mixer, then the engine itself, etc. -- it is quite a whack-a-mole to figure
>> out the synchronization story there. The scalability fix in current default
>> `SecureRandom` would be much more intrusive and risky, since it would change
>> a core crypto class with unknown bug fanout.
>>
>> Using the bulk reads even when the underlying PRNG is heavily synchronized
>> is still a win. A more scalable PRNG would benefit from this as well. This
>> PR adds a system property to select the PRNG implementation, and there we
>> can clearly see the benefit with more scalable PRNG sources:
>>
>>
>> Benchmark Mode Cnt Score Error Units
>>
>> ### x86_64 (c6.8xlarge, Xeon, 18 cores)
>>
>> # Before, hacked `new SecureRandom()` to
>> `SecureRandom.getInstance("SHA1PRNG")`
>> UUIDRandomBench.single thrpt ...
>
> src/java.base/share/classes/java/util/UUID.java line 286:
>
>> 284: long lsb = 0;
>> 285: for (int i = start; i < start + 8; i++) {
>> 286: msb = (msb << 8) | (data[i] & 0xff);
>
> Can we use VarHandle/ByteBuffer to read 64 bits at a time?
`jdk.internal.util.ByteArray` has VarHandle-based methods ready for that
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14135#discussion_r1206305263
Re: [External] : Re: Classes used in method body are loaded lazily or eagerly depending on method return type
On 25/05/2023 7:21 pm, Raffaello Giulietti wrote:
Yes, ChildClass.class is removed from the filesystem.
And here's, the relevant info when running with -Xlog:class+init,
showing that verification succeeds for both TestLoading$ObjectReturner
and TestLoading$BaseClassReturner:
loading: TestLoading$ObjectReturner...
[0.039s][info][class,init] Start class verification for:
TestLoading$ObjectReturner
[0.039s][info][class,init] End class verification for:
TestLoading$ObjectReturner
[0.039s][info][class,init] 500 Initializing
'TestLoading$ObjectReturner'(no method) (0x000801001800)
loading: TestLoading$BaseClassReturner...
[0.039s][info][class,init] Start class verification for:
TestLoading$BaseClassReturner
[0.039s][info][class,init] End class verification for:
TestLoading$BaseClassReturner
[0.039s][info][class,init] 501 Initializing
'TestLoading$BaseClassReturner'(no method) (0x000801001a08)
Can you enable -xlog:verification and class+load too please.
Thanks,
David
-
On 2023-05-25 04:57, David Holmes wrote:
On 25/05/2023 12:34 am, Raffaello Giulietti wrote:
As mentioned in my previous email, if you move the member class
ChildClass out of TestLoading (out of the nest), and make it a
top-level class like
public class ChildClass extends TestLoading.BaseClass {
}
and change
URL classFileB =
TestLoading.class.getResource(TestLoading.class.getSimpleName() +
"$ChildClass.class");
to
URL classFileB =
TestLoading.class.getResource(ChildClass.class.getSimpleName() +
".class");
rebuild everything and run, nothing is thrown.
deleting: /ChildClass.class
loading: TestLoading$ObjectReturner...
loading: TestLoading$BaseClassReturner...
I can't see any substantial difference, except that the nest rooted
at TestLoading lacks a member in the original setup and lacks nothing
in this setup.
What's an explanation for this difference?
Are you sure it actually deletes the file? What do you see when you
enable class+load/init and verification logging?
David
-
Greetings
Raffaello
On 2023-05-24 00:35, Remi Forax wrote:
- Original Message -
From: "David Holmes"
To: "Raffaello Giulietti" ,
"core-libs-dev"
Sent: Wednesday, May 24, 2023 12:23:24 AM
Subject: Re: Classes used in method body are loaded lazily or
eagerly depending on method return type
On 24/05/2023 12:50 am, Raffaello Giulietti wrote:
I think the problem here is that you are deleting a class in a nest.
During the verification of BaseClassReturner.getObject(), access
control
(JVMS, 5.4.4, second half) determines that the nest is broken, as
ChildClass is not present anymore.
Not sure access control gets involved at this stage of the
verification
process. But in any case turning on logging does not show anything
related to nestmates happening between BaseClass and ChildClass. It
seems to just be the resolution of the return type during verification
of the method, that causes the loading of ChildClass and the
subsequent
CNFE if it has been removed.
If you move ChildClass out of TestLoading so that it becomes a
top-level
class extending TestLoading.BaseClass, and if you adapt the line that
initializes the local var classFileB to refer to the new location,
the
code will not throw, despite ChildClass being deleted.
My simplified test shows it still throws when verifying
BaseClassReturner.
Nestmate checking is done lazily, so if you do not call a
method/access a field of a nested class, the VM should not trigger a
class loading.
Moreover, if you test with Java 8 (nestmates were introduced in Java
11), it failed too.
That's another clue that the error is not related to nestmates
checking.
Cheers,
David
regards,
Rémi
Greetings
Raffaello
On 2023-05-23 13:20, Сергей Цыпанов wrote:
Hello,
originally this question was asked here:
https://urldefense.com/v3/__https://stackoverflow.com/q/76260269/12473843__;!!ACWV5N9M2RV99hQ!Mb5nhj7EbuftWzF7s4GX9auUZZlyPyCUnLs64c4mkmSGJm4pw0CNgRzQR5wOYuApyE_kHSAnVxGyTM9PHz5StCppGw$
,
the code sample for reproduction will be put below or can be
found via
the link
The issue is about eager/lazy loading of a class depending on method
return type.
If one runs the code below with Java 11-19 it will fail with
NoClassDefFoundError (this is expected as delete class file for
ChildClass):
java.lang.NoClassDefFoundError: org/example/TestLoading$ChildClass
at java.base/java.lang.Class.forName0(Native Method)
at java.base/java.lang.Class.forName(Class.java:390)
at java.base/java.lang.Class.forName(Class.java:381)
at org.example.TestLoading.loadMyClass(TestLoading.java:29)
at org.example.TestLoading.main(TestLoading.java:23)
Caused by: java.lang.ClassNotFoundException:
org.example.TestLoading$ChildClass
at
java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
at
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v20]
On Fri, 26 May 2023 06:07:05 GMT, Joe Darcy wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improving error recovery in presence of less important syntactic errors in
>> top-level methods and fields.
>>
>> Author: Jan Lahoda
>
> test/jdk/tools/launcher/InstanceMainTest.java line 31:
>
>> 29: * @run main InstanceMainTest
>> 30: */
>> 31: public class InstanceMainTest extends TestHelper {
>
> By my reading of the spec, "main" methods can be defined in record classes
> and enum classes as well as normal classes.
> Are there tests for record and enum main method operation?
You can also have a `main` method inside an interface. For enum classes and
interfaces, the main has to be static.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1206285923
Re: RFR: JDK-8306112 Implementation of JEP 445: Unnamed Classes and Instance Main Methods (Preview) [v20]
On Thu, 25 May 2023 14:32:44 GMT, Jim Laskey wrote:
>> Add flexible main methods and anonymous main classes to the Java language.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improving error recovery in presence of less important syntactic errors in
> top-level methods and fields.
>
> Author: Jan Lahoda
test/jdk/tools/launcher/InstanceMainTest.java line 31:
> 29: * @run main InstanceMainTest
> 30: */
> 31: public class InstanceMainTest extends TestHelper {
By my reading of the spec, "main" methods can be defined in record classes and
enum classes as well as normal classes.
Are there tests for record and enum main method operation?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13689#discussion_r1206277064
