Re: RFR: 8287496: Alternative virtual thread implementation that maps to OS thread [v2]

[Mandy Chung]

On Tue, 31 May 2022 15:39:39 GMT, Alan Bateman  wrote:

>> This patch adds an alternative virtual thread implementation where each 
>> virtual thread is backed by an OS thread. It doesn't scale but it can be 
>> used by ports that don't have continuations support in the VM. Aside from 
>> scalability, the lack of continuations support means:
>> 
>> 1. JVM TI is not supported when running with --enable-preview (the JVM TI 
>> spec allows for this) 
>> 2. jshell --enable-preview can't be used (as jshell uses the debugger APIs 
>> and so needs JVM TI)
>> 
>> The VM option "VMContinuations" is added as an experimental option so it can 
>> be used by tests. A number of tests are changed to re-run with 
>> -XX:-VMContinuations. A new jtreg property is added so that tests that need 
>> the underlying VM support to be present can use "@requires vm.continuations" 
>> in the test description. A follow-up change would be to add "@requires 
>> vm.continuations" to the ~70 serviceability/jvmti/vthread that run with 
>> preview features enabled.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Allowing linking without intrinsic stub, contributed-by: rehn

Looks okay to me.

src/java.management/share/classes/sun/management/ThreadImpl.java line 586:

> 584: 
> 585: /**
> 586:  * Returns the ThreadInfo objects from the given array that 
> coresspond to platform

typo: coresspond -> correspond

src/java.management/share/classes/sun/management/Util.java line 92:

> 90: 
> 91: /**
> 92:  * Returns true if the given Thread is a virutal thread.

typo: virutal -> virtual

this typo appears in other comments too.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8939

Re: RFR: 8287135: Calculation of jmm_GetMemoryUsage is wrong

[Mandy Chung]

On Mon, 23 May 2022 07:28:41 GMT, Yi Yang  wrote:

> It seems that calculation of 
> MemoryMXBean.getNonHeapMemoryUsage(jmm_GetMemoryUsage) is wrong.
> 
> Currently, 
> `NonHeapUsage=CodeCache+Metaspace(ClassTypeSpace+NonClassTypeSpace)+CompressedClassSpace(ClassTypeSpace)`
> 
> ==> CodeHeap 'non-nmethods' 1532544 (Used)
> ==> CodeHeap 'profiled nmethods' 0
> ==> CodeHeap 'non-profiled nmethods' 13952
> ==> Metaspace 506696
> ==> Compressed Class Space 43312
> init = 7667712(7488K) used = 2096504(2047K) committed = 8454144(8256K) max = 
> -1(-1K)
> 
> In this way, getNonHeapMemoryUsage is larger than it ought to be, it should 
> be `NonHeapUsage = CodeCache + Metaspace`.

Memory pool API does not support "nested" memory pools (or sub-memory pools).  
JDK 18 memory pools are the following:

HEAP  G1 Eden Space
HEAP  G1 Old Gen
HEAP  G1 Survivor Space
NON_HEAP  CodeHeap 'non-nmethods'
NON_HEAP  Metaspace
NON_HEAP  CodeHeap 'profiled nmethods'
NON_HEAP  Compressed Class Space
NON_HEAP  CodeHeap 'non-profiled nmethods'


If `Metaspace` memory pool contains the compressed class space, then I agree 
with David that the current non-heap memory pools need to be examined. The meta 
space should be separated into multiple non-heap memory pools without overlap.

-

PR: https://git.openjdk.java.net/jdk/pull/8831

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v5]

[Mandy Chung]

On Thu, 28 Apr 2022 18:24:33 GMT, Andrey Turbanov  wrote:

>> Joe Darcy has updated the pull request with a new target base due to a merge 
>> or a rebase. The incremental webrev excludes the unrelated changes brought 
>> in by the merge/rebase. The pull request contains seven additional commits 
>> since the last revision:
>> 
>>  - Update copyright years.
>>  - Merge branch 'master' into JDK-8285676
>>  - Respond to more review feedback.
>>  - Respond to more review feedback.
>>  - Respond to review feedback.
>>  - Merge branch 'master' into JDK-8285676
>>  - JDK-8285676: Add missing @param tags for type parameters on classes and 
>> interfaces
>
> src/java.base/share/classes/java/lang/ref/PhantomReference.java line 48:
> 
>> 46:  * The {@link #refersTo(Object) refersTo} method can be used to test
>> 47:  * whether some object is the referent of a phantom reference.
>> 48:  * @param the type of the referent
> 
> Shouldn't there be a space after `@param` ?

Good catch.  Sorry I missed it. This occurs in all `java/lang/ref` files.

-

PR: https://git.openjdk.java.net/jdk/pull/8410

Re: RFR: JDK-8285676: Add missing @param tags for type parameters on classes and interfaces [v4]

[Mandy Chung]

On Thu, 28 Apr 2022 16:58:40 GMT, Joe Darcy  wrote:

>> To enable more complete doclint checking (courtesy @jonathan-gibbons), 
>> please review this PR to add type-level @param tags where they are missing.
>> 
>> To the maintainers of java.util.concurrent, those changes could be separated 
>> out in another bug if that would ease maintenance of that code.
>> 
>> Making these library fixes is a blocker for correcting and expanding the 
>> doclint checks (JDK-8285496).
>> 
>> I'll update copyright years before pushing.
>
> Joe Darcy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Respond to more review feedback.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8410

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mandy Chung]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

I reviewed most of the source files in `java.base` except 
`java.util.concurrent`.  I also reviewed `java.logging`, `java.management` and 
`jdk.management` modules.   The changes are easy to follow and clean.  Looks 
good to me.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mandy Chung]

On Wed, 27 Apr 2022 18:13:15 GMT, Paul Sandoz  wrote:

>> src/java.base/share/classes/java/lang/PinnedThreadPrinter.java line 58:
>> 
>>> 56: 
>>> 57: // maps a class to the hashes of stack traces pinned by that code 
>>> in that class
>>> 58: private static final Map, Hashes> classToHashes = new 
>>> WeakHashMap<>();
>> 
>> Can you use `ClassValue` in this case?
>
> I was wondering that too, but held off commenting since it's not super 
> performance critical given `classToHashes` is synchronized on. However, it 
> does make the code a little clearer.

It's not critical and no problem if this is cleaned up as a follow-up.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mandy Chung]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

src/java.management/share/classes/sun/management/ThreadImpl.java line 447:

> 445: if (threads != null) {
> 446: long[] tids = Stream.of(threads)
> 447: .filter(t -> !(t instanceof 
> jdk.internal.misc.CarrierThread))

Returning an array of thread IDs of just the platform threads is good.   The 
javadoc needs to be updated to say "Returns an array of thread identifiers for 
the platform threads"

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mandy Chung]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

src/java.base/share/classes/jdk/internal/misc/UnsafeConstants.java line 122:

> 120:  */
> 121: 
> 122: public static final int SCOPED_CACHE_SHIFT;

I can't find this constant being used.   If added for future, maybe keep 
`UnsafeConstants` class and this static field package-private for now.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Mandy Chung]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

src/java.base/share/classes/java/lang/PinnedThreadPrinter.java line 58:

> 56: 
> 57: // maps a class to the hashes of stack traces pinned by that code in 
> that class
> 58: private static final Map, Hashes> classToHashes = new 
> WeakHashMap<>();

Can you use `ClassValue` in this case?

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]

[Mandy Chung]

On Tue, 19 Apr 2022 10:49:14 GMT, Alan Bateman  wrote:

>> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:
>> 
>>> 59: private final Condition notEmpty;
>>> 60: 
>>> 61: void signal() { notEmpty.signalAll(); }
>> 
>> `signal()`, `await()` and `await(long)` methods are only used by 
>> `ReferenceQueue`.  Good to make them private.
>
> They are overridden so can't be private.

That's right, I missed it.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]

[Mandy Chung]

On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh

src/java.base/share/classes/java/lang/System.java line 2173:

> 2171: 
> 2172: // start Finalizer and Reference Handler threads
> 2173: SharedSecrets.getJavaLangRefAccess().startThreads();

I think it would avoid any confusion if `VM.initLevel(1)` is the last step in 
`initPhase1`, i.e. call after this line.   Previously, the finalizer starts 
very early and it has to wait until initLevel is set to level 1 which 
guarantees that `JavaLangAccess` is available.  With this change, 
`JavaLangAccess` is already set.

src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:

> 59: private final Condition notEmpty;
> 60: 
> 61: void signal() { notEmpty.signalAll(); }

`signal()`, `await()` and `await(long)` methods are only used by 
`ReferenceQueue`.  Good to make them private.

src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
 line 138:

> 136:  *
> 137:  * @param  outputFile the path to the file to create
> 138:  * @param  format the format to use (TEXT_PLAIN or JSON)

It's useful to link to `ThreadDumpFormat#TEXT_PLAN` and `ThreadDumpFormat#JSON`

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]

[Mandy Chung]

On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh

src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 206:

> 204: throws IllegalArgumentException, InterruptedException {
> 205: if (timeout < 0) throw new IllegalArgumentException("Negative 
> timeout value");
> 206: if (timeout == 0) return remove();

Nit: use the same formatting as in `NativeReferenceQueue::remove` and in this 
file.


if (timeout < 0)
throw new IllegalArgumentException("Negative timeout value");
if (timeout == 0)
return remove();

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]

[Mandy Chung]

On Sun, 17 Apr 2022 16:03:30 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh

src/java.management/share/classes/java/lang/management/ThreadMXBean.java line 
655:

> 653:  * synchronization control.  It might be an expensive operation.
> 654:  * Its behavior with cycles involving virtual threads is not defined
> 655:  * in this release.

What does the current implementation return if the virtual threads are involved 
in a deadlock?The class spec says "ThreadMXBean does not support monitoring 
or management of virtual threads" while this javadoc says it's undefined.I 
wonder if it's helpful to include `@implNote` if appropriate.

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8283254: Remove redundant class jdk/internal/agent/spi/AgentProvider

[Mandy Chung]

On Tue, 22 Mar 2022 21:19:20 GMT, Kevin Walls  wrote:

> There are no uses of jdk/internal/agent/spi/AgentProvider, since the SNMP 
> agent was removed ( 8071367 ): this class should be removed.  It is not a 
> public interface.
> 
> Remove 
> src/jdk.management.agent/share/classes/jdk/internal/agent/spi/AgentProvider.java
> Remove import from 
> src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java
> Remove uses from src/jdk.management.agent/share/classes/module-info.java
> 
> Testing with the test/jdk/sun/management tests looks good.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7912

Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v3]

[Mandy Chung]

On Mon, 21 Mar 2022 20:19:03 GMT, Kevin Walls  wrote:

>> Removing permission checks which, in the presence of a Security Manager, 
>> would check for a RuntimePermission "className.subclass".  This was to 
>> prevent subclassing these classes, but is no longer necessary with strong 
>> encapsulation from modules.
>
> Kevin Walls has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove redundant constructors

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7827

Re: RFR: 8283092: JMX subclass permission check redundant with strong encapsulation [v2]

[Mandy Chung]

On Fri, 18 Mar 2022 17:49:05 GMT, Kevin Walls  wrote:

>> Removing permission checks which, in the presence of a Security Manager, 
>> would check for a RuntimePermission "className.subclass".  This was to 
>> prevent subclassing these classes, but is no longer necessary with strong 
>> encapsulation from modules.
>
> Kevin Walls has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Test update

src/java.management/share/classes/sun/management/spi/PlatformMBeanProvider.java 
line 210:

> 208: }
> 209: 
> 210: private PlatformMBeanProvider(Void unused) {

This `PlatformMBeanProvider(Void)` constructor is no longer needed.   Same for 
`AgentProvider(Void)` constructor.

-

PR: https://git.openjdk.java.net/jdk/pull/7827

Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]

[Mandy Chung]

On Mon, 10 Jan 2022 11:17:12 GMT, Kevin Walls  wrote:

>> Remove the use of Security Manager from jstatd.
>> Add use of an ObjectInputFilter to restrict RMI.
>> 
>> Also we can undo the property-setting Launcher.gmk change from: 8279007: 
>> jstatd fails to start because SecurityManager is disabled
>> ..as that is no longer needed.
>> 
>> Docs/man page update to follow (JDK-8278619).
>
> Kevin Walls has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Wildcard in object filter to permit proxies, in case other activity in this 
> JVM changes the nameing/numbering of proxy classes.

If `sun.jvmstat.monitor.remote.RemoteVm` is the only proxy interface, 
`com.sun.proxy.jdk.proxy*` should adequately cover the proxy classes created 
for `RemoteVm`.

-

PR: https://git.openjdk.java.net/jdk/pull/6919

Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed [v2]

[Mandy Chung]

On Mon, 10 Jan 2022 11:17:12 GMT, Kevin Walls  wrote:

>> Remove the use of Security Manager from jstatd.
>> Add use of an ObjectInputFilter to restrict RMI.
>> 
>> Also we can undo the property-setting Launcher.gmk change from: 8279007: 
>> jstatd fails to start because SecurityManager is disabled
>> ..as that is no longer needed.
>> 
>> Docs/man page update to follow (JDK-8278619).
>
> Kevin Walls has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Wildcard in object filter to permit proxies, in case other activity in this 
> JVM changes the nameing/numbering of proxy classes.

Are all the proxy interfaces public?The package in which a proxy class is 
created may be different depending if whether any proxy interface is in a 
non-exported and non-open package.   `com.sun.proxy.jdk.proxy*` is the package 
for non-exported proxy classes.  The proxy classes may be public in an 
unconditionally exported package [1] and its package name is `jdk.proxy*`.

[1] 
https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/reflect/Proxy.html#membership

-

PR: https://git.openjdk.java.net/jdk/pull/6919

Re: RFR: 8272317: jstatd has dependency on Security Manager which needs to be removed

[Mandy Chung]

On Wed, 22 Dec 2021 18:14:43 GMT, Kevin Walls  wrote:

> Remove the use of Security Manager from jstatd.
> Add use of an ObjectInputFilter to restrict RMI.
> 
> Also we can undo the property-setting Launcher.gmk change from: 8279007: 
> jstatd fails to start because SecurityManager is disabled
> ..as that is no longer needed.
> 
> Docs/man page update to follow (JDK-8278619).

src/jdk.jstatd/share/classes/sun/tools/jstatd/Jstatd.java line 51:

> 49: private static RemoteHost remoteHost;
> 50: 
> 51: private static final String rmiFilterPattern = 
> "sun.jvmstat.monitor.remote.RemoteVm;com.sun.proxy.jdk.proxy1.$Proxy1;com.sun.proxy.jdk.proxy1.$Proxy2;java.lang.reflect.Proxy;java.rmi.server.RemoteObjectInvocationHandler;java.rmi.server.RemoteObject;!*";

The class name of the dynamic proxy is generated at runtime and can be 
different.   As Bernd commented, the proxy classes cannot/should not be listed 
in the filter pattern.

-

PR: https://git.openjdk.java.net/jdk/pull/6919

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal

[Mandy Chung]

On Fri, 19 Nov 2021 18:15:46 GMT, Brent Christian  wrote:

>> src/java.base/share/classes/java/lang/Object.java line 481:
>> 
>>> 479:  * system resources or to perform other cleanup.
>>> 480:  * 
>>> 481:  * When running in a Java virtual machine in which finalization 
>>> has been
>> 
>> Disabling finalization is not part of the Java SE spec.   This paragraph 
>> describes the implementation of HotSpot VM and I think it does not belong to 
>> this javadoc.
>
> The coupling between this change and 
> [8276422](https://bugs.openjdk.java.net/browse/JDK-8276422) ("Add 
> command-line option to disable finalization") is probably tighter than would 
> be ideal.  That [CSR](https://bugs.openjdk.java.net/browse/JDK-8276773) 
> allows for finalization to be disabled in the SE Platform Spec and the JLS.

Thanks for pointing out that the CSR for JDK-8276422 ("Add command-line option 
to disable finalization") includes the change of the SE Platform Spec and JLS 
to allow an implementation for finalization to be disabled.   This makes senses 
now.

-

PR: https://git.openjdk.java.net/jdk/pull/6465

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal

[Mandy Chung]

On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian  wrote:

> Here are the code changes for the "Deprecate finalizers in the standard Java 
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code 
> review.
> 
> This change makes the indicated deprecations, and updates the API spec for 
> JEP 421. It also updates the relevant @SuppressWarning annotations.
> 
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6465

Re: RFR: JDK-8276447 Deprecate finalization-related methods for removal

[Mandy Chung]

On Thu, 18 Nov 2021 21:51:30 GMT, Brent Christian  wrote:

> Here are the code changes for the "Deprecate finalizers in the standard Java 
> API" portion of JEP 421 ("Deprecate Finalization for Removal") for code 
> review.
> 
> This change makes the indicated deprecations, and updates the API spec for 
> JEP 421. It also updates the relevant @SuppressWarning annotations.
> 
> The CSR has been approved.
> An automated test build+test run passes cleanly (FWIW :D ).

src/java.base/share/classes/java/lang/Object.java line 481:

> 479:  * system resources or to perform other cleanup.
> 480:  * 
> 481:  * When running in a Java virtual machine in which finalization 
> has been

Disabling finalization is not part of the Java SE spec.   This paragraph 
describes the implementation of HotSpot VM and I think it does not belong to 
this javadoc.

-

PR: https://git.openjdk.java.net/jdk/pull/6465

Re: RFR: 8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with the fix for JDK-6824466 [v3]

[Mandy Chung]

On Thu, 18 Nov 2021 04:03:06 GMT, Leonid Mesnik  wrote:

>> The nsk.share.jdi.TestClass1 is used via reflection. The reflective call 
>> creates MethodHandle and one more reference to TestClass1.  So the number of 
>> expected references should be incremented.  Thanks to @plummercj and 
>> @mlchung for the investigation.
>> This fix also prints references to inspected class.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   force GC to clean weak references

Looks okay to me.  Chris should re-review as I'm not close to the jdi tests.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6402

Re: RFR: 8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with the fix for JDK-6824466 [v2]

[Mandy Chung]

On Tue, 16 Nov 2021 02:45:13 GMT, Leonid Mesnik  wrote:

>> The nsk.share.jdi.TestClass1 is used via reflection. The reflective call 
>> creates MethodHandle and one more reference to TestClass1.  So the number of 
>> expected references should be incremented.  Thanks to @plummercj and 
>> @mlchung for the investigation.
>> This fix also prints references to inspected class.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updated

I believe the Constructor object is already GC'ed.   This weak entry should be 
GC'ed in the subsequent GC cycles.   Can you experiment to call `System::gc` in 
the debuggee and see what the reference count remains?

-

PR: https://git.openjdk.java.net/jdk/pull/6402

Re: RFR: 8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with the fix for JDK-6824466 [v2]

[Mandy Chung]

On Tue, 16 Nov 2021 02:45:13 GMT, Leonid Mesnik  wrote:

>> The nsk.share.jdi.TestClass1 is used via reflection. The reflective call 
>> creates MethodHandle and one more reference to TestClass1.  So the number of 
>> expected references should be incremented.  Thanks to @plummercj and 
>> @mlchung for the investigation.
>> This fix also prints references to inspected class.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updated

I think the test calls `Class::newInstance` to create an instance of 
`TestClass1`.

The old implementation has one reference from `Constructor::clazz` field to 
`TestClass1`.  The new implementation should have 2 new references: 
`MethodHandle::member` whose `MemberName::clazz` field references it and 
`MethodHandle::type` as the signature is `()TestClass1`.   I would expect 2 
additional references but not 1. 

@lmesnik does the debugger show MemberName::clazz?

-

PR: https://git.openjdk.java.net/jdk/pull/6402

Re: RFR: 8265796: vmTestbase/nsk/jdi/ObjectReference/referringObjects/referringObjects002/referringObjects002.java fails when running with the fix for JDK-6824466 [v2]

[Mandy Chung]

On Tue, 16 Nov 2021 02:45:13 GMT, Leonid Mesnik  wrote:

>> The nsk.share.jdi.TestClass1 is used via reflection. The reflective call 
>> creates MethodHandle and one more reference to TestClass1.  So the number of 
>> expected references should be incremented.  Thanks to @plummercj and 
>> @mlchung for the investigation.
>> This fix also prints references to inspected class.
>
> Leonid Mesnik has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   updated

@lmesnik I also have the same question as David asks.   If the reference is 
held via the MethodType, is TestClass1 part of the signature of the reflective 
call?

-

PR: https://git.openjdk.java.net/jdk/pull/6402

Re: RFR: JDK-8272065: jcmd cannot rely on the old core reflection implementation which will be changed after JEP 416

[Mandy Chung]

On Thu, 4 Nov 2021 13:25:14 GMT, Thomas Stuefe  wrote:

> `VM.metaspace`, `VM.classloaders` and `VM.class_hierarchy` all print out 
> reflection invocation targets for delegating reflection class loaders. Post 
> JEP 416 we don't use DelegatingClassLoaders anymore.
> 
> This patch removes the display of reflection targets from these commands as 
> well as associated helper code and tests.
> 
> I don't have enough time atm to reimplement this feature using method 
> handles. But at least we can remove the old code, and prepare the way for 
> more code removal.
> 
> The patch does not touch vmClasses, `reflect_ConstructorAccessor` and 
> `reflect_MethodAccessor` are both still there.
> 
> Tests: GHAs, manually testing the commands.

Looks good to me.   Thanks for following this up.The new implementation 
does not spin any new class loader and so I don't think jcmd needs to extend 
its support for the new implementation using method handles.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6257

Re: RFR: 8275735: [linux] Remove deprecated Metrics api (kernel memory limit)

[Mandy Chung]

On Thu, 28 Oct 2021 13:03:56 GMT, Severin Gehwolf  wrote:

> Please review this change to remove some API which no longer works as 
> expected as recent OCI runtimes start to drop support for `--kernel-memory` 
> switch. See the bug for references. This part of the API is not present in 
> hotspot code.
> 
> Testing: Container tests (cgroup v1) on Linux x86_64 (all pass)

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/6156

Re: RFR: 8275384: Change nested classes in jdk.jconsole to static nested classes

[Mandy Chung]

On Sat, 16 Oct 2021 20:32:43 GMT, Andrey Turbanov  wrote:

> Non-static classes hold a link to their parent classes, which can be avoided.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5974

Re: RFR: 8275322: Change nested classes in java.management to static nested classes

[Mandy Chung]

On Fri, 15 Oct 2021 06:43:13 GMT, Andrey Turbanov  wrote:

> Non-static classes hold a link to their parent classes, which can be avoided.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5962

Re: Internal Thread CPU times in JDK 17

[Mandy Chung]

java.lang.management, com.sun.management API and JVM TI only report Java 
threads.   The internal GC threads are not Java threads and are 
filtered.   There is no supported API to get the safepoint time and 
GC/compiler thread CPU time.


I also think that the internal thread CPU time/usage are useful to 
monitor.  It's worth considering exposing those statistics through 
com.sun.management API and/or via JFR.


Mandy

On 9/27/21 9:30 PM, Dylan Wragge wrote:

Hey serviceability devs,

I'm currently looking to migrate our company's codebases to JDK 17 and 
am running into an issue where sun.management.HotspotThreadMBean and 
HotspotRuntimeMBean are no longer accessible from client code. We have 
a few internal metrics that use this MBean in order to provide 
counters on safepoint time and internal thread (GC/Compiler/etc) CPU 
time/usage.


Safepoint time is nicely exposed as JFR events, which can be consumed 
easily with the streaming JFR API. Internal thread CPU time is proving 
difficult to access however. Other APIs I've tried seem to hide the 
existence of the GC threads altogether (which we are most interested 
in). So far I've tried JFR's ThreadCPUTime event and JVMTI's 
GetAllThreads/GetThreadInfo, both of which don't seem to include any 
of the internal GC threads. The MBean also does not seem to be 
registered by default, so seems totally inaccessible. Although even if 
it were, JEP 396 would stop me accessing any of its methods.


Is there a supported method in JDK 17 to get this information? To me 
this seems like a reasonable use case. In the past, we've found these 
metrics extremely useful when evaluating and tuning new GCs. I managed 
to get something working by scraping /proc but it feels like there 
should be a better (and more portable) way.


- Dylan

IMC Logo 

F 



t 



I 



in 



*imc.com* 


The information in this e-mail is intended only for the person or 
entity to which it is addressed.


It may contain confidential and /or privileged material. If you are 
not the intended recipient, please notify us immediately and delete it 
from your system. Any other use or disclosure by you, including 
through automated tools operating on your systems is prohibited.

Re: RFR: 8274522: java/lang/management/ManagementFactory/MXBeanException.java test fails with Shenandoah

[Mandy Chung]

On Wed, 29 Sep 2021 17:46:46 GMT, Aleksey Shipilev  wrote:

> Fails like this:
> 
> 
> $ CONF=linux-x86_64-server-fastdebug make run-test 
> TEST=java/lang/management/ManagementFactory/MXBeanException.java 
> TEST_VM_OPTS="-XX:+UseShenandoahGC"
> 
> ...
> 
> java.lang.NullPointerException: Cannot invoke 
> "java.lang.management.MemoryPoolMXBean.setUsageThreshold(long)" because 
> "MXBeanException.pool" is null
> 
> 
> This test tries to find the pool that is `!p.isUsageThresholdSupported()`, 
> and for Shenandoah there is no such pool. Likewise with ZGC, which is already 
> filtered.
> 
> Additional testing:
>  - [x] Affected test is now skipped for Shenandoah
>  - [x] Affected test is still skipped for Z
>  - [x] Affected test is still passing for G1

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5757

Re: RFR: 8274523: java/lang/management/MemoryMXBean/MemoryTest.java test should handle Shenandoah

[Mandy Chung]

On Wed, 29 Sep 2021 17:56:00 GMT, Aleksey Shipilev  wrote:

> Currently it fails with:
> 
> 
> $ CONF=linux-x86_64-server-fastdebug make run-test 
> TEST=java/lang/management/MemoryMXBean/MemoryTest.java
> 
> STDERR:
> java.lang.RuntimeException: TEST FAILED: Number of heap pools = 1 but 
> expected <= 3 and >= 3
> 
> 
> Z already handles it with a special configuration, Shenandoah should do the 
> same. 
> 
> Additional testing:
>  - [x] Affected test now works for Shenandoah
>  - [x] Affected test still works for Z
>  - [x] Affected test still works for G1

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/5758

Re: RFR: 8266936: Add a finalization JFR event [v14]

[Mandy Chung]

On Sat, 25 Sep 2021 14:22:28 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   symbols-unix

I reviewed the change in java.base that looks good.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4731

Re: RFR: 8266936: Add a finalization JFR event [v12]

[Mandy Chung]

On Fri, 24 Sep 2021 09:27:02 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   no precompiled headers and mtServiceability nmt classification

Hi Markus,

It's a little surprised to see Finalizer.c to depend JMM interface which is 
used by `java.management` and `jdk.management` modules only.   It's more 
appropriate for it to be a JVM_* entry point for Finalizer to report completion 
of the finalization instead.I understand that you want to make 
FinalizerService to be a conditional feature which is a good idea.  Such JVM 
entry can be made as a nop if not INCLUDE_SERVICES.

-

PR: https://git.openjdk.java.net/jdk/pull/4731

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]

[Mandy Chung]

On Mon, 12 Jul 2021 05:21:34 GMT, Yi Yang  wrote:

> I'm not familiar with the review process of core-lib group. Is it sufficient 
> for merging with one approval? Should I have more reviews for this? 樂

It depends on the change.

For this patch, it's good to get another reviewer to look through it.

-

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} for java.base [v11]

[Mandy Chung]

On Thu, 8 Jul 2021 03:12:24 GMT, Yi Yang  wrote:

>> After JDK-8265518(#3615), it's possible to replace all variants of 
>> checkIndex by 
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in 
>> the whole JDK codebase.
>
> Yi Yang has refreshed the contents of this pull request, and previous commits 
> have been removed. The incremental views will show differences compared to 
> the previous content of the PR. The pull request contains one new commit 
> since the last revision:
> 
>   restore JSR166; restore java.desktop

Looks good.  Thanks for separating the other files from this patch.   I also 
did a test run on tier1-3 that looks clean.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]

[Mandy Chung]

On Wed, 7 Jul 2021 16:22:25 GMT, Mandy Chung  wrote:

>> Hi Mandy, thanks for reviewing this. 
>> 
>>> I suggest to separate the client changes (both src and test) in a separate 
>>> PR for the client review.
>> 
>> Does "client changes" means changes involving src/java.desktop and 
>> test/java/awt?
>> 
>>> src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java 
>>> needs to be updated in JSR 166 upstream repo. Better to file a separate 
>>> issue for this change to ensure that gets fixed in the upstream project.
>> 
>> Can you please paste a link for that? I'm not sure where I can find JSR 166 
>> upstream repo..
>> 
>>> Nit: The above formatting (line 70-97) is inconsistent with the formatting 
>>> in line 110-124. It'd be good to use the same formatting.
>> 
>> Restored.
>
>> Does "client changes" means changes involving src/java.desktop and 
>> test/java/awt?
> 
> src/java.desktop, test/java/awt, and test/javax/imageio

> > src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java 
> > needs to be updated in JSR 166 upstream repo. Better to file a separate 
> > issue for this change to ensure that gets fixed in the upstream project.
> 
> Can you please paste a link for that? I'm not sure where I can find JSR 166 
> upstream repo..

What I meant is to file a JBS issue for this change and revert the change in 
this patch.   That can be fixed when the next JSR 166 changes are imported to 
JDK. 

I wasn't sure if this is the right repo:  
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/

-

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]

[Mandy Chung]

On Wed, 7 Jul 2021 02:10:10 GMT, Yi Yang  wrote:

> Does "client changes" means changes involving src/java.desktop and 
> test/java/awt?

src/java.desktop, test/java/awt, and test/javax/imageio

-

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v8]

[Mandy Chung]

On Wed, 23 Jun 2021 03:54:55 GMT, Yi Yang  wrote:

>> After JDK-8265518(#3615), it's possible to replace all variants of 
>> checkIndex by 
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in 
>> the whole JDK codebase.
>
> Yi Yang has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   tests rely on IOOBE exception message

I suggest to separate the client changes (both src and test) in a separate PR 
for the client review.

I review the rest of the files, which looks okay to me.   I will take another 
pass carefully.

src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java 
needs to be updated in JSR 166 upstream repo.Better to file a separate 
issue for this change to ensure that gets fixed in the upstream project.

test/jdk/java/lang/StringBuffer/Exceptions.java line 73:

> 71: new StringBuffer();
> 72: }
> 73: });

Nit: The above formatting (line 70-97) is inconsistent with the formatting in 
line 110-124.   It'd be good to use the same formatting.

test/jdk/java/lang/StringBuilder/Exceptions.java line 73:

> 71: new StringBuilder();
> 72: }
> 73: });

Nit: it'd be good to make the formatting of all calls to tryCatch method 
consistent.

-

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible

[Mandy Chung]

On Mon, 5 Jul 2021 06:29:58 GMT, David Holmes  wrote:

>> @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest 
>> version? Thanks.
>
> @kelthuzadx  I did not see any response to my query about the change in 
> initialization (not load) order. I also remain concerned about introducing 
> cross dependencies between core classes (e.g. String now uses Precondition, 
> so does that impact Precondition using String?) But these are things for the 
> core-libs folk to be concerned about, or not.
> 
> Cheers,
> David

> @dholmes-ora @AlanBateman @PaulSandoz do you have any comments on the latest 
> version? Thanks.

Alan and Paul are currently on vacation.  I'm reviewing it.

-

PR: https://git.openjdk.java.net/jdk/pull/4507

Re: RFR: 8263203: jconsole Online User Guide has wrong URL [v2]

[Mandy Chung]

On Mon, 7 Jun 2021 21:46:52 GMT, Chris Plummer  wrote:

>> The URL for the users guide, referenced from both the About dialog and from 
>> the "Help -> Online Users Guide" menu item, has not functioned for a few 
>> releases due the restructuring of the JDK online docs. This PR fixes the 
>> URL. Note I also filed JDK-8268351 to get rid of the URL, but for 17 I 
>> prefer to just fix the URL.
>> 
>> BTW, the change to MANAGE_HOTSPOT_MBEANS_IN_COLON_ is just my editor 
>> automatically getting rid of an extra space at the end of the line.
>
> Chris Plummer has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Fix type from last commit
>  - Use URL for users guide instead of command reference

Looks good.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4399

Re: RFR: 8263203: jconsole Online User Guide has wrong URL

[Mandy Chung]

Try "using_jconsole"

Mandy

On 6/7/21 2:09 PM, Chris Plummer wrote:

On Mon, 7 Jun 2021 19:36:23 GMT, Chris Plummer  wrote:


The URL for the users guide, referenced from both the About dialog and from the "Help 
-> Online Users Guide" menu item, has not functioned for a few releases due the 
restructuring of the JDK online docs. This PR fixes the URL. Note I also filed JDK-8268351 
to get rid of the URL, but for 17 I prefer to just fix the URL.

BTW, the change to MANAGE_HOTSPOT_MBEANS_IN_COLON_ is just my editor 
automatically getting rid of an extra space at the end of the line.

...although I'm uncertain how to get to .../management/using-jconsole.html using the 
"lookup" syntax as was done for getting to jconsole.html. Will need to ask 
someone on the docs teame.

-

PR: https://git.openjdk.java.net/jdk/pull/4399

Re: RFR: 8263203: jconsole Online User Guide has wrong URL

[Mandy Chung]

On Mon, 7 Jun 2021 19:36:23 GMT, Chris Plummer  wrote:

> The URL for the users guide, referenced from both the About dialog and from 
> the "Help -> Online Users Guide" menu item, has not functioned for a few 
> releases due the restructuring of the JDK online docs. This PR fixes the URL. 
> Note I also filed JDK-8268351 to get rid of the URL, but for 17 I prefer to 
> just fix the URL.
> 
> BTW, the change to MANAGE_HOTSPOT_MBEANS_IN_COLON_ is just my editor 
> automatically getting rid of an extra space at the end of the line.

Hi Chris, 

It has been a while but I think this may intend to link to the User Guide (not 
the tool reference)
   https://docs.oracle.com/en/java/javase/16/management/using-jconsole.html

-

PR: https://git.openjdk.java.net/jdk/pull/4399

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Mandy Chung]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  wrote:

> Please review this implementation of [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> The code change is divided into 3 commits. Please review them one by one.
> 
> 1. 
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>  The essential change for this JEP, including the `@Deprecate` annotations 
> and spec change. It also update the default value of the 
> `java.security.manager` system property to "disallow", and necessary test 
> change following this update.
> 2. 
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>  Manual changes to several files so that the next commit can be generated 
> programatically.
> 3. 
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>  Automatic changes to other source files to avoid javac warnings on 
> deprecation for removal
> 
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
> generated programmatically, see the comment below for more details. If you 
> are only interested in a portion of the 3rd commit and would like to review 
> it as a separate file, please comment here and I'll generate an individual 
> webrev.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> any file to minimize unnecessary merge conflict.
> 
> Furthermore, since the default value of `java.security.manager` system 
> property is now "disallow", most of the tests calling 
> `System.setSecurityManager()` need to launched with 
> `-Djava.security.manager=allow`. This is covered in a different PR at 
> https://github.com/openjdk/jdk/pull/4071.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/4073

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[Mandy Chung]

On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang  wrote:

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> With JEP 411 and the default value of `-Djava.security.manager` becoming 
> `disallow`, tests calling `System.setSecurityManager()`  need 
> `-Djava.security.manager=allow` when launched. This PR covers such changes 
> for tier1 to tier3 (except for the JCK tests).
> 
> To make it easier to focus your review on the tests in your area, this PR is 
> divided into multiple commits for different areas (~~serviceability~~, 
> ~~hotspot-compiler~~, i18n, rmi, javadoc, swing, 2d, security, 
> hotspot-runtime, nio, xml, beans, ~~core-libs~~, ~~net~~, compiler, 
> ~~hotspot-gc~~). Mostly the rule is the same as how Skara adds labels, but 
> there are several small tweaks:
> 
> 1. When a file is covered by multiple labels, only one is chosen. I make my 
> best to avoid putting too many tests into `core-libs`. If a file is covered 
> by `core-libs` and another label, I categorized it into the other label.
> 2. If a file is in `core-libs` but contains `/xml/` in its name, it's in the 
> `xml` commit.
> 3. If a file is in `core-libs` but contains `/rmi/` in its name, it's in the 
> `rmi` commit.
> 4. One file not covered by any label -- 
> `test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java` -- is in 
> the `swing` commit.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> all files to minimize unnecessary merge conflict.
> 
> Please note that this PR can be integrated before the source changes for JEP 
> 411, as the possible values of this system property was already defined long 
> time ago in JDK 9.
> 
> Most of the change in this PR is a simple adding of 
> `-Djava.security.manager=allow` to the `@run main/othervm` line. Sometimes it 
> was not `othervm` and we add one. Sometimes there's no `@run` at all and we 
> add the line.
> 
> There are several tests that launch another Java process that needs to call 
> the `System.setSecurityManager()` method, and the system property is added to 
> `ProcessBuilder`, `ProcessTools`, or the java command line (if the test is a 
> shell test).
> 
> 3 langtools tests are added into problem list due to 
> [JDK-8265611](https://bugs.openjdk.java.net/browse/JDK-8265611).
> 
> 2 SQL tests are moved because they need different options on the `@run` line 
> but they are inside a directory that has a `TEST.properties`:
> 
> rename test/jdk/java/sql/{testng/test/sql/othervm => 
> permissionTests}/DriverManagerPermissionsTests.java (93%)
> rename test/jdk/javax/sql/{testng/test/rowset/spi => 
> permissionTests}/SyncFactoryPermissionsTests.java (95%)
>  ```
> 
> The source change for JEP 411 is at https://github.com/openjdk/jdk/pull/4073.

I reviewed non-client areas.  Looks okay.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4071

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass

[Mandy Chung]

Maybe you were thinking 
`jdk.internal.reflect.ReflectUtil::isVMAnonymousClass` that is now 
removed by this patch [1].


[1] 
https://github.com/openjdk/jdk/pull/3974/files#diff-1af3026a3b4942af3ebe6a4df02f8952fb9d51bf93336a2f7f93ce175d047574


On 5/13/21 12:03 PM, Brian Goetz wrote:
Thanks for checking.  I thought I remembered something like this 
somewhere, but it may be that you already fixed such tests when you 
did hidden classes?  In any case, seems like we're all good now.


Cheers,
-Brian

On 5/13/2021 2:50 PM, Mandy Chung wrote:
I did a search on java.base and the tests on `String::indexOf` and 
`String::contains` of a slash and don't spot any such test.  The JDK 
has no use of VM anonymous class.   If the test is trying to 
determine if it's lambda proxy class, it should be converted to call 
`Class::isHidden` but testing of the class name containing a slash is 
still valid (I haven't found any of such test though).


Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name 
to see if it contains a slash, especially tests, but which don’t say 
“anonymous”.  Did you do a search for these idioms too, which are now dead 
tests?

Sent from my iPad


On May 11, 2021, at 8:59 AM, Harold Seigel  wrote:

Please review this large change to remove Unsafe::defineAnonymousClass().  The 
change removes dAC relevant code and changes a lot of tests.  Many of the 
changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
the tests were modified to use hidden classes, others were deleted because 
either similar hidden classes tests already exist or they tested dAC specific 
functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and 
Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

-

Commit messages:
- 8243287: Removal of Unsafe::defineAnonymousClass

Changes:https://git.openjdk.java.net/jdk/pull/3974/files
Webrev:https://webrevs.openjdk.java.net/?repo=jdk=3974=00
  Issue:https://bugs.openjdk.java.net/browse/JDK-8243287
  Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod
  Patch:https://git.openjdk.java.net/jdk/pull/3974.diff
  Fetch: git fetchhttps://git.openjdk.java.net/jdk  pull/3974/head:pull/3974

PR:https://git.openjdk.java.net/jdk/pull/3974

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass

[Mandy Chung]

I did a search on java.base and the tests on `String::indexOf` and 
`String::contains` of a slash and don't spot any such test.  The JDK has 
no use of VM anonymous class.   If the test is trying to determine if 
it's lambda proxy class, it should be converted to call 
`Class::isHidden` but testing of the class name containing a slash is 
still valid (I haven't found any of such test though).


Mandy

On 5/11/21 6:20 AM, Brian Goetz wrote:

There may be some JDK code that checks for anon classes by comparing the name 
to see if it contains a slash, especially tests, but which don’t say 
“anonymous”.  Did you do a search for these idioms too, which are now dead 
tests?

Sent from my iPad


On May 11, 2021, at 8:59 AM, Harold Seigel  wrote:

Please review this large change to remove Unsafe::defineAnonymousClass().  The 
change removes dAC relevant code and changes a lot of tests.  Many of the 
changed tests need renaming.  I hope to do this in a follow up RFE.  Some of 
the tests were modified to use hidden classes, others were deleted because 
either similar hidden classes tests already exist or they tested dAC specific 
functionality, such as host classes.

This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and 
Mach5 tiers 3-7 on Linux x64.

Thanks, Harold

-

Commit messages:
- 8243287: Removal of Unsafe::defineAnonymousClass

Changes: https://git.openjdk.java.net/jdk/pull/3974/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3974=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8243287
  Stats: 3516 lines in 116 files changed: 69 ins; 3181 del; 266 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3974.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3974/head:pull/3974

PR: https://git.openjdk.java.net/jdk/pull/3974

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v4]

[Mandy Chung]

On Wed, 12 May 2021 16:10:24 GMT, Harold Seigel  wrote:

>> Please review this large change to remove Unsafe::defineAnonymousClass().  
>> The change removes dAC relevant code and changes a lot of tests.  Many of 
>> the changed tests need renaming.  I hope to do this in a follow up RFE.  
>> Some of the tests were modified to use hidden classes, others were deleted 
>> because either similar hidden classes tests already exist or they tested dAC 
>> specific functionality, such as host classes.
>> 
>> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
>> and Mach5 tiers 3-7 on Linux x64.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   test changes and small fixes

Overall looks good to me.

src/hotspot/share/classfile/classLoaderData.cpp line 299:

> 297: }
> 298: 
> 299: // Weak hidden classes have their own ClassLoaderData that is marked to 
> keep alive

`s/Weak/Non-strong/`

test/hotspot/jtreg/runtime/HiddenClasses/StressHiddenClasses.java line 39:

> 37: import jdk.test.lib.compiler.InMemoryJavaCompiler;
> 38: 
> 39: public class StressHiddenClasses {

Since `StressClassLoadingTest` is revised to test hidden class, this test is a 
subset of it.
I think this can be removed as JDK-8265694 suggests.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3974

Re: RFR: 8243287: Removal of Unsafe::defineAnonymousClass [v3]

[Mandy Chung]

On Tue, 11 May 2021 14:13:49 GMT, Harold Seigel  wrote:

>> Please review this large change to remove Unsafe::defineAnonymousClass().  
>> The change removes dAC relevant code and changes a lot of tests.  Many of 
>> the changed tests need renaming.  I hope to do this in a follow up RFE.  
>> Some of the tests were modified to use hidden classes, others were deleted 
>> because either similar hidden classes tests already exist or they tested dAC 
>> specific functionality, such as host classes.
>> 
>> This change was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, 
>> and Mach5 tiers 3-7 on Linux x64.
>> 
>> Thanks, Harold
>
> Harold Seigel has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix GetModuleTest.java

I reviewed java.base and took a pass on the test changes.  Here are some 
comments:
 
test/hotspot/jtreg/runtime/HiddenClasses/TestHiddenClassUnloading.java has this 
comment:


+// This is based on test 
compiler/classUnloading/anonymousClass/TestAnonymousClassUnloading.java


This comment can be removed as this test will be removed.   A few tests under  
test/hotspot/jtreg/runtime/HiddenClasses also have similar comment that should 
be removed.

test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/func/castToGrandparent/Test.java
test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/func/classNameInStackTrace/Test.java
- I think these tests are already well covered by 
test/hotspot/jtreg/runtime/HiddenClasses/CastToParentTest.java
and test/hotspot/jtreg/runtime/HiddenClasses/HiddenClassStack.java 
- I suggest to copy the description from the anonloader tests to these hidden 
class tests

test/hotspot/jtreg/vmTestbase/vm/mlvm/anonloader/share/StressClassLoadingTest.java
   - test/hotspot/jtreg/runtime/HiddenClasses/StressClassLoadingTest.java is a 
subset of this test.  Should we remove 
test/hotspot/jtreg/runtime/HiddenClasses/StressClassLoadingTest.java?
   
test/jdk/java/lang/invoke/VMAnonymousClass.java
- FYI.  I have added a new test to verify hidden class (see JDK-8266925)

-

PR: https://git.openjdk.java.net/jdk/pull/3974

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Mandy Chung]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

I reviewed the module-loader-map and non-hotspot non-AOT tests.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3398

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v7]

[Mandy Chung]

On Fri, 2 Apr 2021 18:00:57 GMT, Daniel Fuchs  wrote:

>> This RFE proposes to extend the MXBean framework to define a mapping to 
>> records.
>> 
>> The MXBean framework already defines a mapping of `CompositeType` to plain 
>> java objects. Records are a natural representation of CompositeTypes. A 
>> record can be easily reconstructed from a `CompositeData` through the record 
>> canonical constructor. A clear advantage of records over plain java objects 
>> is that the canonical constructor will not need to be annotated in order to 
>> map composite data property names to constructor parameter names.
>> 
>> With this RFE, here is an example comparing coding a composite type 
>> `NamedNumber` that consists of an `int` and a `String`, using records and 
>> using a plain java class. In both case, the `CompositeType` looks like this:
>> 
>> CompositeType(
>> "NamedNumber",  // typeName
>> "NamedNumber",  // description
>> new String[] {"number", "name"},// itemNames
>> new String[] {"number", "name"},// itemDescriptions
>> new OpenType[] {SimpleType.INTEGER,
>> SimpleType.STRING}  // itemTypes
>> );
>> 
>> The plain Java class needs a public constructor annotated with 
>> `@ConstructorParameters` annotation:
>> 
>> public class NamedNumber {
>> public int getNumber() {return number;}
>> public String getName() {return name;}
>> @ConstructorParameters({"number", "name"})
>> public NamedNumber(int number, String name) {
>> this.number = number;
>> this.name = name;
>> }
>> private final int number;
>> private final String name;
>> }
>> 
>> And the equivalent with a record class: 
>> 
>> public record NamedNumber(int number, String name) {}
>
> Daniel Fuchs 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 13 additional 
> commits since the last revision:
> 
>  - Merge
>  - minor style issue
>  - minor style issue
>  - Integrated review feedback
>  - Improved test
>  - Moved mapping for records just before Mapping for MXBean interface; 
> Improved test.
>  - Merge branch 'master' into mxbeans-8264124
>  - Minor tweaks. Improved test.
>  - Merge branch 'master' into mxbeans-8264124
>  - Integrated review feedback. Updated the section for Mapping to records.
>  - ... and 3 more: 
> https://git.openjdk.java.net/jdk/compare/522a6472...257a6ed8

This looks okay to me but I'm not that familiar with the existing 
implementation.  The test case covers the important cases.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v6]

[Mandy Chung]

On Wed, 31 Mar 2021 21:02:51 GMT, Daniel Fuchs  wrote:

>> This RFE proposes to extend the MXBean framework to define a mapping to 
>> records.
>> 
>> The MXBean framework already defines a mapping of `CompositeType` to plain 
>> java objects. Records are a natural representation of CompositeTypes. A 
>> record can be easily reconstructed from a `CompositeData` through the record 
>> canonical constructor. A clear advantage of records over plain java objects 
>> is that the canonical constructor will not need to be annotated in order to 
>> map composite data property names to constructor parameter names.
>> 
>> With this RFE, here is an example comparing coding a composite type 
>> `NamedNumber` that consists of an `int` and a `String`, using records and 
>> using a plain java class. In both case, the `CompositeType` looks like this:
>> 
>> CompositeType(
>> "NamedNumber",  // typeName
>> "NamedNumber",  // description
>> new String[] {"number", "name"},// itemNames
>> new String[] {"number", "name"},// itemDescriptions
>> new OpenType[] {SimpleType.INTEGER,
>> SimpleType.STRING}  // itemTypes
>> );
>> 
>> The plain Java class needs a public constructor annotated with 
>> `@ConstructorParameters` annotation:
>> 
>> public class NamedNumber {
>> public int getNumber() {return number;}
>> public String getName() {return name;}
>> @ConstructorParameters({"number", "name"})
>> public NamedNumber(int number, String name) {
>> this.number = number;
>> this.name = name;
>> }
>> private final int number;
>> private final String name;
>> }
>> 
>> And the equivalent with a record class: 
>> 
>> public record NamedNumber(int number, String name) {}
>
> Daniel Fuchs has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - minor style issue
>  - minor style issue

Thanks for making the change.  The spec change looks good to me.

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Mandy Chung]

On Wed, 31 Mar 2021 15:38:22 GMT, Daniel Fuchs  wrote:

>> Link added. With respect to adding a section for records, it's a bit 
>> difficult to separate that from the "Mapping for other types" without a lot 
>> of duplication. I can add a row in the summary table, and then add a new 
>> section "Mapping for records" just before "Mapping for other types", that 
>> just gives a brief overview and refers to the mapping for other types below. 
>> 
>> Something like this - what do you think?
>> 
>> Mappings for Records
>> 
>> A {@linkplain Record record} R whose {@linkplain
>>   Class#getRecordComponents() components} are
>>   all convertible to open types, is itself convertible to a
>>   {@link CompositeType} as follows.
>>   The type name of this {@code CompositeType}
>>   is determined by the same type name rules
>>   defined by the Mapping for other types
>>   below. Its getters are the accessors for the {@linkplain
>>   RecordComponent record components}, and the record is reconstructed
>>   using its canonical constructor, without needing any annotation.
>> 
>> A record may also expose additional non-canonical constructors, which
>>   can be used to reconstruct the record if the composite data does
>>   not exactly contain all the components expected by the
>>   canonical constructor. However, in order to be taken into account
>>   by the MXBean framework, such non-canonical constructors
>>   need to be annotated with either the {@link ConstructorParameters
>>   javax.management.ConstructorParameters} or
>>  {@code @java.beans.ConstructorProperties} annotation.
>> 
>> The complete rules for the mapping are detailed as part
>>   of the Mapping for other types
>>   below.
>> 
>> Mappings for other types
>> 
>> Given a record, or a Java class or interface J that does not 
>> match the other
>>   rules in the table above, the MXBean framework will attempt to map
>>   it to a {@link CompositeType} as follows.  []
>
> Hi Mandy, I have updated the PR with a revised version of the text above. Is 
> that more in line what you had in mind?
> 
> best regards,
> -- daniel

This looks better.  I like a separate mapping section for records since the 
mapping rules are straight-forward.   The complexity comes if we want to 
support a record to implement `CompositeDataView` that allows a type to support 
more flexible conversion to `CompositeData` which is why you add records in the 
"Mappings for other types".   I am wondering if we really want to support 
records to implement `CompositeDataView` but we may want to avoid such a 
special case.

I suggest add subsections in "Mappings for other types":   There are two 
mapping rules: (1) _opentype(J)_ maps `J` to `CompositeType` (2) _opendata(J)_ 
maps `J` to `CompositeData`.   _opentype(J)_ for record class does not need to 
refer to other types.   The common cases for _opendata(J)_ for record class is 
using the canonical constructors.   The other less common cases like using 
non-canonical constructors and `CompositeDataView` can state that it follows 
the same rules as specified for other types.   "Mappings for other types" does 
not need any change for records.   

"Reconstructing an instance of Java type J from a CompositeData" section should 
be renamed to "Reconstructing an instance of Java type or record class J from a 
CompositeData"

Here is a minor tweaking on "Mappings for Records"

A record is convertible to a CompositeType if and only if all its components 
are 
convertible to open types. Otherwise, it is not convertible.

A record class is converted to a CompositeType with one item for every record 
component as follows.
- The type name of this CompositeType is determined by the type name rules 
detailed below. 
- Each record component of type T, the item in the CompositeType has the same 
name 
  as the record component and of type opentype(T).

The mapping from an instance of a record class to a CompositeData corresponding 
to 
the CompositeType is the same as specified as for other types (add a link).

A record is reconstructed from a CompositeData using its canonical constructor. 
The canonical constructor doesn't require the presence of 
@javax.management.ConstructorParameters 
or @java.beans.ConstructorProperties annotations. If these annotations are 
present on
the canonical constructor, they will be ignored.

If the CompositeData from which the record is reconstructed doesn't contain all 
the record components, 
the MXBean framework will attempt to reconstruct the record in the same way as 
specified for 
other types (add a link).

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Mandy Chung]

On Fri, 26 Mar 2021 15:15:48 GMT, Daniel Fuchs  wrote:

>> This RFE proposes to extend the MXBean framework to define a mapping to 
>> records.
>> 
>> The MXBean framework already defines a mapping of `CompositeType` to plain 
>> java objects. Records are a natural representation of CompositeTypes. A 
>> record can be easily reconstructed from a `CompositeData` through the record 
>> canonical constructor. A clear advantage of records over plain java objects 
>> is that the canonical constructor will not need to be annotated in order to 
>> map composite data property names to constructor parameter names.
>> 
>> With this RFE, here is an example comparing coding a composite type 
>> `NamedNumber` that consists of an `int` and a `String`, using records and 
>> using a plain java class. In both case, the `CompositeType` looks like this:
>> 
>> CompositeType(
>> "NamedNumber",  // typeName
>> "NamedNumber",  // description
>> new String[] {"number", "name"},// itemNames
>> new String[] {"number", "name"},// itemDescriptions
>> new OpenType[] {SimpleType.INTEGER,
>> SimpleType.STRING}  // itemTypes
>> );
>> 
>> The plain Java class needs a public constructor annotated with 
>> `@ConstructorParameters` annotation:
>> 
>> public class NamedNumber {
>> public int getNumber() {return number;}
>> public String getName() {return name;}
>> @ConstructorParameters({"number", "name"})
>> public NamedNumber(int number, String name) {
>> this.number = number;
>> this.name = name;
>> }
>> private final int number;
>> private final String name;
>> }
>> 
>> And the equivalent with a record class: 
>> 
>> public record NamedNumber(int number, String name) {}
>
> Daniel Fuchs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed typo. Moved example with record after example with from method to 
> respect the logical order of precedence.

src/java.management/share/classes/javax/management/MXBean.java line 757:

> 755: 
> 756: If the class is a {@link Record}, its getters are the
> 757:   accessors for the record components. Otherwise, the

It may be good to add a link like {@linkplain RecordComponent record components}

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8264124: Update MXBean specification and implementation to extend mapping of CompositeType to records [v2]

[Mandy Chung]

On Sat, 27 Mar 2021 01:40:17 GMT, Mandy Chung  wrote:

>> Daniel Fuchs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed typo. Moved example with record after example with from method to 
>> respect the logical order of precedence.
>
> src/java.management/share/classes/javax/management/MXBean.java line 757:
> 
>> 755: 
>> 756: If the class is a {@link Record}, its getters are the
>> 757:   accessors for the record components. Otherwise, the
> 
> It may be good to add a link like {@linkplain RecordComponent record 
> components}

You add record in "Mappings for other types".  I think it deserves a separate 
section "Mappings for record classes" (maybe after primitive types).   It's 
useful to add a row for record in the summary table above "Mappings for 
primitive types"

-

PR: https://git.openjdk.java.net/jdk/pull/3201

Re: RFR: 8260448: Simplify ManagementFactory$PlatformMBeanFinder

[Mandy Chung]

On Tue, 26 Jan 2021 19:34:35 GMT, Claes Redestad  wrote:

> Simplify and desugar the static initialization of PlatformMBeanFinder. 
> 
> While arguably making the code easier to comprehend, this saves 5-6ms on 
> startup in various applications such as the spring-petclinic.

Looks fine.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2244

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v11]

[Mandy Chung]

On Tue, 22 Dec 2020 22:11:19 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   1. Prperly reverted fixes in 3 tests; 2. Got rid of  term in 
> tests summary comment

Thanks for the change.  Looks okay.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v10]

[Mandy Chung]

On Tue, 22 Dec 2020 05:47:19 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   @summary of some negative tests is corrected

test/jdk/java/lang/instrument/RetransformAgent.java line 26:

> 24: /**
> 25:  * @test
> 26:  * @bug 6274264 6274241 5070281 8165276

The copyright header and `@bug` change should be reverted.

test/jdk/java/lang/instrument/SimpleAgent.java line 2:

> 1: /*
> 2:  * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights 
> reserved.

The copyright header change should be reverted.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v9]

[Mandy Chung]

On Fri, 18 Dec 2020 12:00:48 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   1. Suppress premain method check only if agent class is in unnamed module; 
> 2. minor tests cleanup

The change looks okay with minor comments on the tests.Nit: it will be good 
to update `@summary` in the InheritAgent tests that now fail to load the 
agent.  

W.r.t. the premain/agentmain method declared in the agent class, the 
specification clearly states that in the package summary:

> The agent class must implement a public static premain method similar in 
> principle to the main application entry point.

> The agent class must implement a public static agentmain method.

test/jdk/java/lang/instrument/PremainClass/NonPublicAgent.java line 27:

> 25:  * @test
> 26:  * @bug 8165276
> 27:  * @summary Test that agent with non-public premain method is rejected to 
> load

the comment is incorrect.  This tests a non-public agent class with a public 
premain method.   The agent is not rejected.

test/jdk/java/lang/instrument/RetransformAgent.java line 41:

> 39: import asmlib.*;
> 40: 
> 41: public class RetransformAgent {

Making RetransformAgent as public is not necessary.   This fix does not change 
this test and so better to revert it.

test/jdk/java/lang/instrument/SimpleAgent.java line 26:

> 24: import java.lang.instrument.Instrumentation;
> 25: 
> 26: public class SimpleAgent {

There is no other change in this test except making SImpleAgent as public which 
is not necessary.   So better to revert it.

-

Marked as reviewed by mchung (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v8]

[Mandy Chung]

On Thu, 17 Dec 2020 04:59:17 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   added to NegativeAgentRunner exit value check to be non-zero

@sspitsyn  Thanks for the update.   I think it's good to add the unnamed module 
check before calling `setAccessible` that becomes clear that this check only 
intends for unnamed module.   Maybe make it clear in the comment something like 
this: 

// if the java agent class is in an unnamed module, the java agent class 
can be non-public.
// suppress access check upon the invocation of the premain/agentmain method

Since the agent class can be non-public, it means that it's not necessary to 
modify the test agent class to public as you did in this patch.

I don't think `@library /test` is needed, isn't it?  The test library classes 
are under test/lib.

src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java line 
475:

> 473: 
> 474: // reject non-public premain or agentmain method
> 475: if ((m.getModifiers() & java.lang.reflect.Modifier.PUBLIC) == 0) 
> {

An alternative way: `if (Modifier.isPublic(m.getModifiers()))`

Similar can be applied for checking if the javaagentClass is public.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]

[Mandy Chung]

On Wed, 16 Dec 2020 08:18:49 GMT, Alan Bateman  wrote:

> So PR1694 is really just about aligning the spec so that the premain method 
> is public. However, it does not fix the overall accessibility issue.

OK I see that JDK-5070281 changes to allow the agent class to be non-public as 
the main class that declares `public static void main`.   So `setAccessible` 
should only be called if the agent class is in an unnamed module.  

If the agent class is in a named module and not accessible to java.instrument 
module (if the java agent is packaged in a modular JAR file), it should fail 
with IAE - can you confirm?I wanted to understand the current behavior.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v6]

[Mandy Chung]

On Wed, 16 Dec 2020 07:40:18 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fixed trailing spaces

test/jdk/java/lang/instrument/AgentJarBuilder.java line 36:

> 34: import jdk.test.lib.util.JarUtils;
> 35: 
> 36: public class AgentJarBuilder {

Can you use the test library `test/lib/jdk/test/lib/util/JavaAgentBuilder.java`?

test/jdk/java/lang/instrument/PremainClass/InheritAgent0010.java line 32:

> 30:  * @library /test/lib
> 31:  * @library /test
> 32:  * @modules jdk.jartool/sun.tools.jar

I don't see `sun.tools.jar` is used.  Is this qualified exports needed?

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v6]

[Mandy Chung]

On Wed, 16 Dec 2020 07:40:18 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   fixed trailing spaces

test/jdk/java/lang/instrument/NegativeAgentRunner.java line 42:

> 40: agentClassName);
> 41: OutputAnalyzer output = new OutputAnalyzer(pb.start());
> 42: output.shouldContain(excepClassName);

This should also check if the exit value is non-zero.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]

[Mandy Chung]

On 12/15/20 5:50 PM, David Holmes wrote:

On 16/12/2020 11:35 am, Serguei Spitsyn wrote:
On Wed, 16 Dec 2020 00:50:04 GMT, David Holmes  
wrote:

The agent class doesn't have to be public it just has to be accessible.

The premain method should be queried for public modifier rather than 
just relying on a failed invocation request.


David, thank you for catching this. I'm probably missing something here.
If the agent class is not public then the `m.canAccess(null)` check 
is not passed and IAE is thrown with the message:
   `Exception in thread "main" java.lang.IllegalAccessException: 
method NonPublicAgent.premain must be declared public`


But the `NonPublicAgent.premain` is declared public as below:
 public static void premain(String agentArgs, Instrumentation 
inst) {

 System.out.println("premain: NonPublicAgent was loaded");
 }
It seems, the IAE is thrown because the agent class is not public.
Does it mean the `m.canAccess(null)` check is not fully correct?


canAccess will check both the type accessibility and the method 
accessibility.


The premain class is not required to be public 


(I think you meant "exported").


The premain method must be public as this issue about.   I expect the 
agent module must export the package of the Premain-Class (or 
Agent-Class) premain method (qualifiedly or unconditionally) for 
java.instrument to access.  The spec does not state clearly if the agent 
class is public but as it requires the premain method public, it is 
sensible to say it's accessible (otherwise, premain method does not have 
to be public), i.e. public premain method in a public (accessible) 
class).    So in the modular world, the public method in a public class 
in a package exported at least to java.instrument.


Are you expecting differently?

The spec should clarify.  FWIW.  I just realized that 
https://bugs.openjdk.java.net/browse/JDK-6932391 is not resolved.



so it may not be right to checks its accessibility as that may fail 
even though the premain method is public and should be invoked. In 
that regard I think the setAccessible call has to remain to deal with 
this situation. So the process would be:


- load premain class via appLoader
- lookup premain method using getDeclaredMethod()
- check premain method is public and reject if not
- call setAccessible(true) in case premain class is not accessible
- do invoke

David


Mandy

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v4]

[Mandy Chung]

On 12/14/20 2:47 PM, Serguei Spitsyn wrote:

On Mon, 14 Dec 2020 02:39:30 GMT, David Holmes  wrote:


Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

   added 8165276 to @bug list of impacted tests

Changes requested by dholmes (Reviewer).

Alan, David and Many, thank you for the comments!
I'll prepare an update according to the recent requests from you.
One question is if I need to clone this PR to the JDK 16 fork:
   https://github.com/openjdk/jdk16
It depends on our chances to finalize this before RDP2.
An alternate approach would be to continue reviewing this PR until all comment 
are resolved and then clone it to jdk16.


Since this already passes RDP1, this can be retarget to JDK 17 and 
continue with this PR to openjdk/jdk master.  I see no urgency to fix 
this in JDK 16.


Mandy

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs [v2]

[Mandy Chung]

On Thu, 10 Dec 2020 00:37:14 GMT, Serguei Spitsyn  wrote:

>> This change have been already reviewed by Mandy, Sundar, Alan and David.
>> Please, see the jdk 15 review thread:
>>   
>> http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-June/031998.html
>> 
>> Now, the PR approval is needed.
>> The push was postponed because the CSR was not approved at that time (it is 
>> now):
>>https://bugs.openjdk.java.net/browse/JDK-8248189
>> Investigation of existing popular java agents was requested by Joe.
>> --
>> 
>> The java.lang.instrument spec:
>>   
>> https://docs.oracle.com/en/java/javase/15/docs/api/java.instrument/java/lang/instrument/package-summary.html
>> 
>> Summary:
>>   The java.lang.instrument spec clearly states:
>> "The agent class must implement a public static premain method
>>  similar in principle to the main application entry point."
>>   Current implementation of sun/instrument/InstrumentationImpl.java
>>   allows the premain method be non-public which violates the spec.
>>   This fix aligns the implementation with the spec.
>> 
>> Testing:
>>   A mach5 run of jdk_instrument tests is in progress.
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   removed lookup for inherited premain/agentmain methods

Please update @bug in the tests to include this bug ID.

All InheritAgentXXX tests are updated to have the InheritAgentXXX class public. 
 However, I think you need to go through them individually for this behavioral 
change.   The existing comments may not be applicable and please update them 
where appropriate.For example InheritAgent0100 previously verifies the 
invocation of the premain in its superclass.  Does the modified test ensure 
that this fails to load?  Per the comment in the new InheritAgent0100Super 
class, it expects the superclass' premain should be called.

src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java line 
499:

> 497: // reject non-public premain or agentmain method
> 498: if (!m.canAccess(null)) {
> 499: String msg = "method " + classname + "." +  methodname + " 
> must be declared public";

The comments in line 429-443 need to be updated.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Re: RFR: 8165276: Spec states to invoke the premain method in an agent class if it's public but implementation differs

[Mandy Chung]

On Wed, 9 Dec 2020 06:40:25 GMT, Serguei Spitsyn  wrote:

>> src/java.instrument/share/classes/sun/instrument/InstrumentationImpl.java 
>> line 501:
>> 
>>> 499: String msg = "method " + classname + "." +  methodname + " 
>>> must be declared public";
>>> 500: throw new IllegalAccessException(msg);
>>> 501: }
>> 
>> If canAccess fails then it means that javaAgentClass is not public or the 
>> premain method is not public. The invoke will fail but I agree eat explicit 
>> canAccess check means we get finer control on the exception message.
>> 
>> (I can't help feeling that we should do a bit more cleanup here and not use 
>> getDeclaredMethod or be concerned with inheritance. This is because the 
>> Premain-Class is meant to specify the agent class. Also can't have a public 
>> static premain in super class and a non-public static premain in a 
>> sub-class).
>
> My gut feeling is that it should be possible to get rid of the 
> getDeclaredMethod calls with the reasoning you provided above. The canAccess 
> check is not really needed in such a case as the getMethod returns public 
> methods only (is my understanding correct?). This would simplify the 
> implementation. Two disadvantages of this approach are:
> - less fine control on the exception message: NoSuchMethodException instead 
> of IllegalAccessException for non-public premain methods
> - some compatibility risk is involved (most of agents define premain method 
> correctly, so the only java agents that define premain methods with unusual 
> tricks are impacted)

If the java agent class declares a non-public premain method but  its 
superclass declares a public premain method, what should be the expected 
behavior?   The proposed fix will choose the java agent class's non-public 
premain and therefore it will fail to load the agent class with IAE.

If we get rid of the `getDeclaredMethod` call and find premain using 
`getMethod`, it will choose the public premain method defined directly in the 
java agent class or indirectly in its superclass.

As Alan stated, `Premain-Class` attribute is meant to specify the agent class.  
Also specified in the package spec of `java.lang.instrument', the agent class 
must implement a public static premain method similar in principle to the main 
application entry point.  Also under "Manifest Attributes" section,
   Premain-Class
When an agent is specified at JVM launch time this attribute specifies 
the agent class. That is, the class containing the premain method. 
 It expects that there is a public premain method declared in the agent class, 
not in its superclass.

So I think it's best to fix this as well in this PR by dropping line 469-495 
(i.e. keep `getDeclaredMethod` case and not search for inherited premain).   
Throw if the premain method is not found or it is not accessible.   This needs 
to update the CSR for behavioral change.

-

PR: https://git.openjdk.java.net/jdk/pull/1694

Integrated: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Thu, 3 Dec 2020 22:54:54 GMT, Mandy Chung  wrote:

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

This pull request has now been integrated.

Changeset: 972bc3b4
Author:Mandy Chung 
URL:   https://git.openjdk.java.net/jdk/commit/972bc3b4
Stats: 60 lines in 12 files changed: 7 ins; 11 del; 42 mod

8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

Reviewed-by: sspitsyn, shade, dfuchs, alanb, kbarrett

-

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo` [v2]

[Mandy Chung]

> This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
> avoid keeping a referent strongly reachable that could cause unnecessary 
> delay in collecting such object.   I only made change in some but not all 
> classes in core libraries when working with Kim on `Reference::refersTo`.
> The remaining uses are left for the component owners to convert at 
> appropriate time.

Mandy Chung has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 15 commits:

 - revise matchsKey to check equality if the reference is not cleared
 - fix typo
 - update copyright end year and fixup grammatical nit
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - 8256167: Convert JDK use of  to
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - minor cleanup
 - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into 
refersTo
 - improve refersTo0 descriptions
 - ... and 5 more: https://git.openjdk.java.net/jdk/compare/d8ac76fa...72947eb3

-

Changes: https://git.openjdk.java.net/jdk/pull/1609/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1609=01
  Stats: 60 lines in 12 files changed: 7 ins; 11 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1609.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 20:14:13 GMT, Peter Levart  wrote:

>> Good point on checking k != null. A cleaner check:
>> 
>> // then check for equality if the referent is not cleared
>> Object k = e.get();
>> return k != null || key.equals(k);
>
> You meant: `return k != null && key.equals(k);` right?

oops...yes.

-

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 20:05:23 GMT, Kevin Rushforth  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 437:
> 
>> 435: int index = indexFor(h, tab.length);
>> 436: Entry e = tab[index];
>> 437: while (e != null && !(e.hash == h && matchesKey(e, k))
> 
> This doesn't compile, which is why the checks from the GitHub actions build 
> failed.

I caught that and it's in my local repo that I haven't pushed the commit.  
Sorry for not syncing my branch for review.

-

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 19:47:17 GMT, Peter Levart  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.management/share/classes/com/sun/jmx/mbeanserver/WeakIdentityHashMap.java
>  line 127:
> 
>> 125: T got = get();
>> 126: return got != null && wr.refersTo(got);
>> 127: }
> 
> Here you could pre-screen the get() with:
> 
> if (this.refersTo(null) != wr.refersTo(null)) return false;
> 
> In case one of the two WeakReference(s) is cleared and the other is not, they 
> are not equal and you don't call this.get() in such case.

`expunge` is called at `get`, `put`, and `remove`.   While there is a chance 
that a referent might be cleared,  I will leave this as is and this patch 
simply replaces to use `refersTo`.

-

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 18:12:36 GMT, Kim Barrett  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 293:
> 
>> 291: // then checks for equality
>> 292: Object k = e.get();
>> 293: return key == k || key.equals(k);
> 
> I think `key == k` is already covered by refersTo. But k could be null; 
> checking for that here might be useful, to skip the call to equals in that 
> case.

Good point on checking k != null. A cleaner check:

// then check for equality if the referent is not cleared
Object k = e.get();
return k != null || key.equals(k);

-

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

On Fri, 4 Dec 2020 09:19:23 GMT, Aleksey Shipilev  wrote:

>> This patch replaces some uses of `Reference::get` to `Reference::refersTo` 
>> to avoid keeping a referent strongly reachable that could cause unnecessary 
>> delay in collecting such object.   I only made change in some but not all 
>> classes in core libraries when working with Kim on `Reference::refersTo`.
>> The remaining uses are left for the component owners to convert at 
>> appropriate time.
>
> src/java.base/share/classes/java/util/WeakHashMap.java line 291:
> 
>> 289: if (e.refersTo(key)) return true;
>> 290: 
>> 291: // then checks for equality
> 
> Obnoxiously minor nit: plurality is inconsistent. `check if the given 
> entry...` above, and `then check[s] for equality`. Should be `...then check 
> for equality`?

OK.  Fixed this grammatical nit.

-

PR: https://git.openjdk.java.net/jdk/pull/1609

RFR: 8256167: Convert JDK use of `Reference::get` to `Reference::refersTo`

[Mandy Chung]

This patch replaces some uses of `Reference::get` to `Reference::refersTo` to 
avoid keeping a referent strongly reachable that could cause unnecessary delay 
in collecting such object.   I only made change in some but not all classes in 
core libraries when working with Kim on `Reference::refersTo`.The remaining 
uses are left for the component owners to convert at appropriate time.

-

Commit messages:
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - 8256167: Convert JDK use of  to
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - Merge branch 'master' of https://github.com/openjdk/jdk into refersTo
 - minor cleanup
 - Merge branch 'refersto' of https://github.com/kimbarrett/openjdk-jdk into 
refersTo
 - improve refersTo0 descriptions
 - basic functional test
 - change referent access
 - expand test
 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/f0b11940...b1e645b1

Changes: https://git.openjdk.java.net/jdk/pull/1609/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1609=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256167
  Stats: 52 lines in 12 files changed: 7 ins; 10 del; 35 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1609.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1609/head:pull/1609

PR: https://git.openjdk.java.net/jdk/pull/1609

Re: RFR: 8253280: Use class name as class loading lock

[Mandy Chung]

On Thu, 10 Sep 2020 05:25:43 GMT, Robert LU 
 wrote:

> When many thread try to load same class, the thread will stuck on 
> `ClassLoader.loadClass`.
> At current jdk, the stacktrace by example program is:
> "Thread-1" prio=5 Id=13 BLOCKED on java.lang.String@724af044 owned by 
> "Thread-0" Id=12
> at 
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646)
> -  blocked on java.lang.String@724af044 val="java.lang.String"
> at 
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:634)
> at 
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:182)
> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:519)
> at app//Main2.test(Main2.java:19)
> at app//Main2$$Lambda$37/0x0001000c2a20.run(Unknown Source)
> at java.base/java.lang.Thread.run(Thread.java:831)
> There is no way to get which class stuck the thread.
> 
> **After this patch, the stacktrace will be**:
> "Thread-2" prio=5 Id=13 BLOCKED on java.lang.String@724af044 owned by 
> "Thread-3" Id=14
>   at 
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:612)
>   -  blocked on java.lang.String@724af044 val="java.lang.String"
>   at 
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:600)
>   at 
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
>   at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
>   at app//Main2.test(Main2.java:18)
>   at app//Main2$$Lambda$38/0x000100097440.run(Unknown Source)
>   at java.base/java.lang.Thread.run(Thread.java:832)
> That is, user will know which class stuck the thread, in this example, the 
> class is `java.lang.String`. It's helpful for troubleshooting.
> 
> The example program:
> Before patch:
> 
> Main.java
> 
> // Main.java
> import java.io.PrintStream;
> import java.lang.management.*;
> 
> public final class Main {
> private synchronized static void test1() {
> while (true) {
> try {
> Thread.sleep(1000);
> } catch (InterruptedException e) {
> e.printStackTrace();
> }
> }
> }
> 
> private static void test() {
> while (true) {
> try {
> Main.class.getClassLoader().loadClass("java.lang.String");
> } catch (ClassNotFoundException e) {
> }
> }
> }
> 
> public static void main(String[] args) throws InterruptedException, 
> NoSuchFieldException, IllegalAccessException {
> new Thread(Main::test).start();
> new Thread(Main::test).start();
> new Thread(Main::test).start();
> new Thread(Main::test).start();
> new Thread(Main::test).start();
> 
> while (true) {
> Thread.sleep(1000);
> ThreadMXBean bean = ManagementFactory.getThreadMXBean();
> ThreadInfo[] infos = bean.dumpAllThreads(true, true);
> for (ThreadInfo info : infos) {
> System.out.println(printThreadInfo(info));
> }
> }
> }
> 
> private static String printThreadInfo(ThreadInfo threadInfo) {
> StringBuilder sb = new StringBuilder(""" + threadInfo.getThreadName() 
> + """ +
> (threadInfo.isDaemon() ? " daemon" : "") +
> " prio=" + threadInfo.getPriority() +
> " Id=" + threadInfo.getThreadId() + " " +
> threadInfo.getThreadState());
> if (threadInfo.getLockName() != null) {
> sb.append(" on " + threadInfo.getLockName());
> }
> if (threadInfo.getLockOwnerName() != null) {
> sb.append(" owned by "" + threadInfo.getLockOwnerName() +
> "" Id=" + threadInfo.getLockOwnerId());
> }
> if (threadInfo.isSuspended()) {
> sb.append(" (suspended)");
> }
> if (threadInfo.isInNative()) {
> sb.append(" (in native)");
> }
> sb.append('\n');
> int i = 0;
> StackTraceElement[] stackTrace = threadInfo.getStackTrace();
> for (; i < stackTrace.length; i++) {
> StackTraceElement ste = stackTrace[i];
> sb.append("\tat " + ste.toString());
> sb.append('\n');
> if (i == 0 && threadInfo.getLockInfo() != null) {
> Thread.State ts = threadInfo.getThreadState();
> switch (ts) {
> case BLOCKED:
> sb.append("\t-  blocked on " + 
> printLockInfo(threadInfo.getLockInfo()));
> sb.append('\n');
> break;
> case WAITING:
> sb.append("\t-  waiting on " + 
> printLockInfo(threadInfo.getLockInfo()));
>

Re: RFR: JDK-8255262: Remove use of legacy custom @spec tag

[Mandy Chung]

On Thu, 22 Oct 2020 17:16:23 GMT, Jonathan Gibbons  wrote:

> The change is (just) to remove legacy usages of a JDK-private custom tag.

Marked as reviewed by mchung (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/814

Re: RFR: 8246774: Record Classes (final) implementation

[Mandy Chung]

On Thu, 24 Sep 2020 00:23:13 GMT, Mandy Chung  wrote:

>> @vicente-romero-oracle I noticed that we can also remove the preview args 
>> from the record serialization tests and
>> ObjectMethodsTest. I opened a PR against the branch in your fork. You should 
>> be able to just merge in the changes. See
>> https://github.com/vicente-romero-oracle/jdk/pull/1
>
> What is the policy of `@since` release value when a preview API becomes 
> final.I would expect `@since` should be
> updated from 14 to 16 because 16 is the Java SE release these APIs are added??

Thanks Alex.

@vicente-romero-oracle  `@since` needs to be changed.

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation

[Mandy Chung]

On Tue, 22 Sep 2020 09:49:12 GMT, Chris Hegarty  wrote:

>> note: I have removed from the original patch the code related to 
>> javax.lang.model, I will publish them in a separate PR
>
> @vicente-romero-oracle I noticed that we can also remove the preview args 
> from the record serialization tests and
> ObjectMethodsTest. I opened a PR against the branch in your fork. You should 
> be able to just merge in the changes. See
> https://github.com/vicente-romero-oracle/jdk/pull/1

What is the policy of `@since` release value when a preview API becomes final.  
  I would expect `@since` should be
updated from 14 to 16 because 16 is the Java SE release these APIs are added??

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs

[Mandy Chung]

On 6/27/20 12:23 AM, Alan Bateman wrote:

On 27/06/2020 01:40, serguei.spit...@oracle.com wrote:


The implementation has this order of lookup:

    // The agent class must have a premain or agentmain method that
    // has 1 or 2 arguments. We check in the following order:
    //
    // 1) declared with a signature of (String, Instrumentation)
    // 2) declared with a signature of (String)
    // 3) inherited with a signature of (String, Instrumentation)
    // 4) inherited with a signature of (String)

The declared methods are gotten with the getDeclaredMethod and 
inherited with the getMethod.
This works for both 1-arg and 2-arg premain methods, so I'm not sure 
what is inconsistent.

Or you have a concern there can be a non-nice NoSuchMethodException?

In fact, I don't understand why there is a need to use the 
getDeclaredMethod.
As I see, the getMethod should return a declared method first, and 
only if it is absent then it checks for a inherited one.
The JPLIS agent used getMethod when it was originally created in JDK 5 
so it would only find public methods. I haven't studied the 
intervening history too closely but I assume JDK-6289149 (in JDK 7) 
created the inconsistency between the spec and implementation when it 
explored the scenario of premain declared in a super class with 
different arity and/or modifiers to the premain in the sub-class. I 
assume the tests that you've been forced to change are related to this 
same issue.




Thanks for digging up the history.

So given where we are, and given the statement "The JVM first attempts 
to invoke the following method on the agent class" in the spec then I 
guess it's okay to keep the getDeclaredMethod to deal with "whacky" 
case where a super class of the agent class has a public premain method.




I also think it's okay to get a different exception message in this case.

Serguie - I reviewed this version.  It looks okay.

New wevrev version is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.2/


Mandy

Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs

[Mandy Chung]

On 6/24/20 12:26 PM, serguei.spit...@oracle.com wrote:

On 6/24/20 05:25, David Holmes wrote:


Ah! The test class SimpleAgent is what is not public. That seems a 
bug in the test.


There are many such tests.
We can break some of the existing agents by rejecting non-public agent 
classes.
I'm inclined to continue using the setAccessible and just add an extra 
check for non-public premain/agentmain methods.


There is only one non-public SimpleAgent which is shared by 
j.l.instrument tests.

  test/jdk/java/lang/instrument/SimpleAgent.java

test/hotspot/jtreg/runtime/cds/appcds/jvmti/dumpingWithAgent implements 
the agent properly (a public class and a public static void premain method).


As the popular Java agents are conforming the spec (publicly accessible 
premain method), the compatibility risk is low.


Unless such a  java agent exists and finds a strong compelling reason to 
argue that its premain method must be allowed non-public, I do not see 
the argument to change the spec to allow non-public agent classes.


A bad test case is not a representative existing java agent.

Mandy

Re: 15 RFR(XS): 8165276: Spec states that invoke the premain method in an agent class if it's public but implementation differs

[Mandy Chung]

Hi Serguei,

I'm glad that you have a patch for this.

On 6/23/20 7:05 PM, serguei.spit...@oracle.com wrote:

Please, review a fix for:
  https://bugs.openjdk.java.net/browse/JDK-8165276


CSR draft (one CSR reviewer is needed before finalizing it):
  https://bugs.openjdk.java.net/browse/JDK-8248189



The compatibility risk should be low (rather than minimal).

It says "All known Java agents define the premain method as public".  
It'd be useful to add a comment in the JBS issue to list the Java agents 
you have checked.




Webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/instr-setAccessible.1/




Looks okay.  Can you add a test to verify this fix?

Mandy

Re: RFR: [15,docs] JDK-8248061,bad reference in @throws in HotSpotDiagnosticMXBean

[Mandy Chung]

+1

Mandy

On 6/22/20 4:55 PM, Jonathan Gibbons wrote:


Please review a small change to fix an unresolved reference in 
`@throws IOException`.


The problem is that the method signature uses a fully-qualified name 
for `java.io.IOException` instead of importing it. meaning that the 
`@throws` cannot resolve the name. Although this could be fixed by 
using a fully-qualified name in `@throws` as well, a better, more 
conventional solution is to import that name and use the simple name 
in both places.


-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8248061

Patch:


$ hg diff -R open open/src/jdk.management
diff -r 9cfa0137612f 
src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
--- 
a/src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java 
Mon Jun 22 13:37:41 2020 -0700
+++ 
b/src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java 
Mon Jun 22 16:07:38 2020 -0700

@@ -25,6 +25,7 @@

 package com.sun.management;

+import java.io.IOException;
 import java.lang.management.PlatformManagedObject;

 /**
@@ -72,7 +73,7 @@
  * method denies write access to the named file
  * or the caller does not have ManagmentPermission("control").
  */
- public void dumpHeap(String outputFile, boolean live) throws 
java.io.IOException;
+ public void dumpHeap(String outputFile, boolean live) throws 
IOException;


 /**
  * Returns a list of {@code VMOption} objects for all diagnostic 
options.

Re: RFR: JDK-8247784,Bad link causes invalid documentation

[Mandy Chung]

+1.  Thanks for fixing this.

Mandy


On 6/18/20 8:16 PM, Jonathan Gibbons wrote:


resend, with correct subject line

On 6/18/20 8:12 PM, Jonathan Gibbons wrote:


Please review some changes to fix typos in some recent doc updates.

In two places, ${docRoot} is used instead of {@docRoot}

-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8247784

Patch:

diff -r c5904de55565 src/jdk.jdi/share/classes/com/sun/jdi/Type.java
--- a/src/jdk.jdi/share/classes/com/sun/jdi/Type.java Thu Jun 18 
17:32:57 2020 -0700
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/Type.java Thu Jun 18 
20:05:42 2020 -0700

@@ -152,7 +152,7 @@
  * Returns the name of this type. The result is of the same form as
  * the name returned by {@link Class#getName()}.
  * The returned name may not be a
- * href="${docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name.
+ * href="{@docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name.

  *
  * @return the name of this type
  */
diff -r c5904de55565 
src/jdk.jdi/share/classes/com/sun/jdi/event/ClassUnloadEvent.java
--- 
a/src/jdk.jdi/share/classes/com/sun/jdi/event/ClassUnloadEvent.java 
Thu Jun 18 17:32:57 2020 -0700
+++ 
b/src/jdk.jdi/share/classes/com/sun/jdi/event/ClassUnloadEvent.java 
Thu Jun 18 20:05:42 2020 -0700

@@ -44,7 +44,7 @@
 /**
  * Returns the {@linkplain com.sun.jdi.Type#name() name of the 
class}

  * that has been unloaded. The returned string may not be a
- * href="${docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name.
+ * href="{@docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name.

  *
  * @see Class#getName()
  */

Re: RFR: [15,docs] JDK-8247894,Invalid @see in java.management

[Mandy Chung]

+1

Mandy

On 6/18/20 4:00 PM, Jonathan Gibbons wrote:


Please review a trivial fix for an invalid @see tag in 
java/lang/management/package.html.
The presumed intent of the original is not a supported variant. The 
fix is to remove the

`{@linkplain ...}` wrapper.

-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8247894

Patch inline:

diff -r cf0df75c75c1 
src/java.management/share/classes/java/lang/management/package.html
--- 
a/src/java.management/share/classes/java/lang/management/package.html 
Thu Jun 18 14:07:49 2020 -0700
+++ 
b/src/java.management/share/classes/java/lang/management/package.html 
Thu Jun 18 15:51:09 2020 -0700

@@ -234,7 +234,7 @@

  The java.lang.management API is thread-safe.

-@see <mailto:-@see>{@linkplain javax.management JMX Specification}
+@see javax.management JMX Specification

 @author Mandy Chung
 @since 1.5

Re: RFR: 8242328: Update mentions of ThreadMBean to ThreadMXBean

[Mandy Chung]

On 6/12/20 12:27 AM, David Holmes wrote:


No. There is no MXBeanServer.

This attempts to shed some light on everything:

https://docs.oracle.com/en/java/javase/14/docs/api/java.management/javax/management/MXBean.html 



"An MXBean is a kind of MBean. An MXBean object can be registered 
directly in the MBean Server, or it can be used as an argument to 
StandardMBean and the resultant MBean registered in the MBean Server."


MXBean was added in Java SE 6 to support the management bean API changes 
and interoperability.


For a remote client to access a MBean, all classes referenced by a MBean 
interface must be observable in the remote client.  If a MBean is 
updated to reference type X, a remote client attempts to access this 
MBean may fail if X is not present.


MXBean is a type of open MBean which means that a remote client accesses 
a MXBean using the predefined set of open types.   For example, if 
ThreadMXBean were modified to access a new type say StackFrame in JDK N, 
a remote client can continue to run on an older JDK while it's able to 
monitor a JVM running on a newer release N using the open types 
(javax.management.openmbean.* types).


Mandy

Re: RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

Thanks for confirming it.

Mandy

On 5/28/20 3:31 PM, Harold Seigel wrote:


Hi Mandy,

The entries in the PermittedSubclasses attribute are constant pool 
ConstantClass_info entries.  These names get validated by the VM in 
this code in ClassFileParser::parse_constant_pool():


  for (index = 1; index < length; index++) {
    const jbyte tag = cp->tag_at(index).value();
    switch (tag) {
  case JVM_CONSTANT_UnresolvedClass: {
    const Symbol* const class_name = cp->klass_name_at(index);
    // check the name, even if _cp_patches will overwrite it
*verify_legal_class_name(class_name, CHECK);*
    break;
  }

Thanks, Harold


On 5/28/2020 5:12 PM, Mandy Chung wrote:
I read the JVMS but it isn't clear to me that the VM will validate 
the names in `PermittedSubclasses`attribute are valid class 
descriptors.   I see ConstantPool::is_klass_or_reference check but 
can't find where it validates the name is a valid class descriptor - 
can you point me there?   (otherwise, maybe define it to be unspecified?)

Re: RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

Hi Harold,

On 5/27/20 1:35 PM, Harold Seigel wrote:


Incremental webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.incr.2/


full webrev: 
http://cr.openjdk.java.net/~hseigel/sealedClasses.8225056.2/webrev/



Class.java

4406 ReflectionData rd = reflectionData();
4407 ClassDesc[] tmp = rd.permittedSubclasses;
4408 if (tmp != null) {
4409 return tmp;
4410 }
4411
4412 if (isArray() || isPrimitive()) {
4413 rd.permittedSubclasses = new ClassDesc[0];
4414 return rd.permittedSubclasses;
4415 }


This causes an array class or primitive type to create a 
ReflectionData.   It should first check if this is non-sealed class and 
returns a constant empty array.


In fact, ReflectionData caches the derived names and reflected members 
for performance and also they may be invalidated when the class is 
redefined.   It might be okay to add ReflectionData::permittedSubclasses 
while `PermittedSubclasses` attribute can't be redefined and getting 
this attribute is not performance sensitive.   For example, the result 
of `getNestMembers` is not cached in ReflectionData.  It may be better 
not to add it in ReflectionData for modifiable and performance-sensitive 
data.



4421 tmp = new ClassDesc[subclassNames.length];
4422 int i = 0;
4423 for (String subclassName : subclassNames) {
4424 try {
4425 tmp[i++] = ClassDesc.of(subclassName.replace('/', '.'));
4426 } catch (IllegalArgumentException iae) {
4427 throw new InternalError("Invalid type in permitted subclasses 
information: " + subclassName, iae);

4428 }
4429 }

Nit: rename tmp to some other name e.g. descs

I read the JVMS but it isn't clear to me that the VM will validate the 
names in `PermittedSubclasses`attribute are valid class descriptors.   I 
see ConstantPool::is_klass_or_reference check but can't find where it 
validates the name is a valid class descriptor - can you point me 
there?   (otherwise, maybe define it to be unspecified?)



W.r.t. the APIs. I don't want to delay this review.  I see that you 
renamed the method to new API style as I brought up.  OTOH,  I expect 
more discussion is needed. Below is a recent comment from John on this 
topic [1]


One comment, really for the future, regarding the shape of the Java 
API here: It uses Optional and omits the "get" prefix on accessors. 
This is the New Style, as opposed to the Classic Style using null (for 
"empty" results) and a "get" prefix ("getComponentType") to get a 
related type. We may choose to to use the New Style for new reflection 
API points, and if so let's not choose N different New Styles, but one 
New Style. Personally I like removing "get"; I accept Optional instead 
of null; and I also suggest that arrays (if any) be replaced by 
(immutable) Lists in the New Style


There are a few existing Class APIs that use the new API style:
Class arrayClass();
Optional describeConstable();
String descriptorString();

This will set up a precedence of the new API style in this class. Should 
this new permittedSubclasses method return a List instead of an array?  
It's okay with me if you prefer to revert back to the old API style and 
follow this up after integration.


4442 * Returns true if this {@linkplain Class} is sealed.
4443 *
 * @return returns true if this class is sealed


NIt: {@code true} instead of true -  consistent with the style this 
class uses (in most methods)


test/jdk/java/lang/reflect/sealed_classes/SealedClassesReflectionTest.java

Nit: s/sealed_classes/sealedClasses/
- the test directory/file naming convention use camel case or java 
variable name convention.


Thanks
[1] https://github.com/openjdk/valhalla/pull/53#issuecomment-633116043

Re: RFR: JDK-8225056 VM support for sealed classes

[Mandy Chung]

Hi Vicente,

On 5/20/20 8:40 AM, Vicente Romero wrote:

Hi David,


src/java.base/share/classes/java/lang/Class.java

There needs to be a CSR request for these changes.


yes there is one already: 
https://bugs.openjdk.java.net/browse/JDK-8244556


Adding to David's comment w.r.t. @throws IAE:

The Class::getXXX APIs returns `Class` or `Class[]` if the result is 
about type(s).  This new `getPermittedSubclasses` returns 
`ClassDesc[]`.   I wonder if this should be renamed to 
`permittedSubclasses` to follow the convention of the new APIs added to 
support descriptors e.g. `describeConstable`


Nit: {@linkplain Class} should be {@code Class}

Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/20/20 1:06 PM, Chris Plummer wrote:

On 4/18/20 3:28 PM, Mandy Chung wrote:

Hi Chris, Serguei,

On 4/18/20 12:18 AM, Chris Plummer wrote:


Yes. I'd like to see all this as part of the Class/Classloading spec 
in some sort of section that gives an overview of all these topics, 
but mostly clarifies what an initiating loader is, and the (non) 
relationship to hidden classes. 


We should refer initiating loader and class loading from JVMS 5.3.   
JVM TI needs the clarification w.r.t. GetClassLoaderClasses that does 
not include hidden classes because the initiating class loader cannot 
find it.


GetLoadedClasses is about class creation.   While it seems tempted to 
put the descriptions in some sort of overview section, I found the 
clarification is specific for each function and hence inlining them 
in each function helps the readers directly see the difference.


I made a few changes that should ease your concern of duplication:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes.01/ 



- The class creation description added in GetLoadedClasses.  Not in 
JDWP, JDI and Instrumentation.
- The description in GetClassLoaderClasses, 
Instrumentation::getInitiatedClasses, 
ClassLoaderReference::visibleClasses is revised to take out the 
details.   Add links from JDWP and JDI to GetClassLoaderClasses.
- The details about hidden classes can be found from 
`Class::isHidden` and      `Lookup::defineHiddenClass`.


Mandy

Hi Mandy,

This looks much better. Thank you for the consolidating and the cross 
references. I think that's a big improvement.


One minor thing I noticed is a few "JNI-style signature" references. 
In LocalVariable.java you renamed "JNI signature" to "JNI-style 
signature", but in JDI Mirror.java you renamed "JNI-style signature" 
to "type signature". 


You meant Type::signature.   Type::signature may return the type 
signature of a hidden class whereas LocalVariable::signature and 
TypeComponent::signature return the field/method descriptor where no 
hidden class will appear in a resolved field/variable/method.


And then in JDI TypeComponent.java you left the "JNI-style signature" 
reference unchanged. Why the inconsistency?


I made LocalVariable::signature consistent with TypeComponent::signature 
whereas Type::signature returns the string of the form described in 
Class::descriptorString.


I tried to keep existing reference to use "JNI-style signature". Since I 
touched on these files, I can change them to "type signature" which 
applies to these cases but I will leave the link to the JNI spec as it is.



In VirtualMachine.java:

 145  * @see Class or interface creation specified in
 146  * href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
GetLoadedClasses


How about:

 145  * @a description of how Class and interface creation can be 
triggered is specified in
 146  * href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">JVM TI 
GetLoadedClasses




 * @see href="{@docRoot}/../specs/jvmti/jvmti.html#GetLoadedClasses">
 * JVM TI GetLoadedClasses regarding how class and interface 
creation can be triggered


I read the following incorrectly a few times before I finally 
understood the meaning:


44 /**
  45  * Returns the {@linkplain com.sun.jdi.Type#name() name of 
the class}

  46  * that has been unloaded. The returned string may not be a
  47  * href="${docRoot}/java.base/java/lang/ClassLoader.html#binary-name">binary 
name.

  48  *
  49  * @see Class#getName()
  50  */
  51 public String className();
  52
  53 /**
  54  * Returns the {@linkplain com.sun.jdi.Type#signature() type 
signature of the class}
  55  * that has been unloaded.  The returned string may not be a 
type descriptor

  56  * specified in JVMS {@jvms 4.3.2}.
  57  *
  58  * @see java.lang.invoke.TypeDescriptor#descriptorString()
  59  */
  60 public String classSignature();

"may not be" through me off. I read it as "is not allowed not be". 
Your intent was "might not be", which might be a better choice of words.




I make it clearer as:

 * Returns the {@linkplain com.sun.jdi.Type#signature() type 
signature of the class}

 * that has been unloaded.  The result is of the same
 * form as the string returned by {@link Class#descriptorString()}.
 * If this class can be described nominally, the returned string is a
 * type descriptor conforming to JVMS {@jvms 4.3.2}; otherwise, the 
returned string

 * is not a type descriptor.

Similar clarification is also made in Type::signature:

 * Returns the type signature for this type.  The result is of the same
 * form as the string returned by {@link Class#descriptorString()}.
 * The returned string is a type descriptor conforming to JVMS 
{@jvms 4.3.2}
 * if this type can be described nominally.  Otherwise, the 
returned string

 * is not a type descriptor.

Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

Hi Chris, Serguei,

On 4/18/20 12:18 AM, Chris Plummer wrote:


Yes. I'd like to see all this as part of the Class/Classloading spec 
in some sort of section that gives an overview of all these topics, 
but mostly clarifies what an initiating loader is, and the (non) 
relationship to hidden classes. 


We should refer initiating loader and class loading from JVMS 5.3. JVM 
TI needs the clarification w.r.t.  GetClassLoaderClasses that does not 
include hidden classes because the initiating class loader cannot find it.


GetLoadedClasses is about class creation.   While it seems tempted to 
put the descriptions in some sort of overview section, I found the 
clarification is specific for each function and hence inlining them in 
each function helps the readers directly see the difference.


I made a few changes that should ease your concern of duplication:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes.01/

- The class creation description added in GetLoadedClasses.  Not in 
JDWP, JDI and Instrumentation.
- The description in GetClassLoaderClasses, 
Instrumentation::getInitiatedClasses, 
ClassLoaderReference::visibleClasses is revised to take out the 
details.   Add links from JDWP and JDI to GetClassLoaderClasses.
- The details about hidden classes can be found from `Class::isHidden` 
and      `Lookup::defineHiddenClass`.


Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/18/20 12:47 AM, Chris Plummer wrote:


It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.
Since it looks like this reference was present before your changes, I 
guess it's ok to leave it, but like JDWP maybe a bug should be filed 
to clean it up.




Right, they are existing reference.  I'll let Serguei to file a JBS 
issue to follow up.


Regarding your JNI spec reference, it say "The JNI uses the Java VM’s 
representation of type signatures" and then has a table called "Java 
VM Type Signatures". No where in the JNI spec do you see "JNI 
Signature" or "JNI Type Signature". It seems we should always just use 
"Type Signature"?




I agree that the svc specs should be examined and use the terminologies 
consistently.


Even the JVMTI spec is not consistent. It has a couple of references 
to JNI Type Signature as links to the JNI spec, but also says "type 
signatures" in a couple of places, including for 
GetLocalVariableTable() which refers the the JVMS: "The local 
variable's type signature, encoded as a modified UTF-8 string. The 
signature format is the same as that defined in The Java™ Virtual 
Machine Specification, Chapter 4.3.2. "


Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/17/20 3:51 PM, Chris Plummer wrote:

Hi Mandy,

Thanks for updating the svc specs. Some comments below:


In the JDWP spec update, you changed "JNI signature" to "type 
signature" in one place, but left it as "JNI signature" everywhere 
else. Should they all be changed?




JDWP signature is changed because there is no JNI signature representing 
a hidden class.    I leave other references to JNI signature as is since 
those only apply for normal classes as I wanted to keep this change 
specifically due to hidden classes.  I think it's good to file a JBS 
issue to follow up on JDWP and JDI to determine if the spec should be 
upgraded to reference the new TypeDescriptor API.




In the JDWP spec for ClassLoaderReference.VisibleClasses:

 "That is, this class loader has been recorded as an initiating 
loader of the returned classes." -> "That is, all classes for which 
this class loader has been recorded as an initiating loader."


This seems like too much detail to be put here. Basically the term 
"initiating ClassLoader" has turned into a short essay. Is it possible 
that all this detail could be put elsewhere and referenced? 


Any suggestion?   We attempted to place those description in JVM TI 
Class section or ClassLoad event.   However, that's not ideal place 
since that's needed by JDWP, JDI and Instrumentation.   I found inlining 
this description is not ideal but it provides adequate clarification.


Aren't there other places in other specs where a similar clarification 
of "initiating ClassLoader" is needed (I see now that 
ClassLoaderClasses in the JVMTI spec, 
ClassLoaderReference,visibleClasses in the JDI spec, and 
Instrumentation.getInitiatedClasses are all dealing with this, but not 
all in the exact same way).




I took the conservative side and make sure the clarification is in place 
for all APIs.  I'm open to any suggestion for example having JDWP and 
JDI to link to JVM TI spec if you think appropriate.




In the JVMTI spec for GetLoadedClasses:

This suffers in a way similar to ClassLoaderReference.VisibleClasses 
in the JDWP spec, although not as badly. A simple concept ends up with 
a complex description, and it seems that description should really be 
in a more centralized place.  I would also suggest a bit of cleanup of 
these lines:


6866 An array class is created directly by Java virtual 
machine.  An array class
6867 creation can be triggered by using class loaders or by 
invoking methods in certain

6868 Java SE Platform API such as reflection.

"Created by [the] Java virtual machine" (add "the")
Change "An array class creation" to "The creation" since your are 
repeating "An array class" from the previous sentence.



In the JVMTI spec ClassLoaderClasses section:

"That is, initiating_loader has been recorded as an initiating loader 
of the returned classes." -> "That is, all classes for which 
initiating_loader has been recorded as an initiating loader."



In the JVMTI spec GetClassSignature section:

"If the class indicated by klass is ..." -> "If the class ..." (you 
just finished the previous sentence with "class indicated by Klass").


"the returned name is [the] JNI type signature" (add "the"). Also, is 
"JNI type signature" formally defined somewhere? This relates to my 
JDWP spec comment above.




It's a link to 
https://download.java.net/java/early_access/jdk15/docs/specs/jni/types.html#type-signatures. 
This is how the current JVM TI spec defines.


" where N is the binary name encoded in internal form indicated by the 
class file". Is "binary name encoded in internal form" explained 
somewhere? 


JVMS 4.2.1

https://docs.oracle.com/javase/specs/jvms/se14/html/jvms-4.html#jvms-4.2.1


Also, can you add an example of a returned hidden class signature?


OK



In the JVMTI spec ClassLoad section:

"representation using [a] class loader" (add "a")

"By invoking Lookup::defineHiddenClass, that creates ..."  -> "By 
invoking Lookup::defineHiddenClass to create ..."


"certain Java SE Platform API" -> Should be "APIs"


In JDI ClassLoaderReference.definedClasses()

"loaded at least to the point of preparation and types ..." -> "loaded 
at least to the point of preparation, and types ..." (Note, this not a 
new issue with your edits)



In Instrumentation.getAllLoadedClasses:

The reference to `class` did not format properly.



Serguei caught that one too.  I fixed it in my local repo.

"by invoking Lookup::defineHiddenClass that creates" -> "by invoking 
Lookup::defineHiddenClass, which creates"


"An array class is created directly by Java virtual machine. An array 
class creation can be triggered ..." ->"An array class is created 
directly by the Java virtual machine. Array class creation can be 
triggered ..."



In Instrumentation.getInitiatedClasses:

"That is, loader has been recorded as an initiating loader of these 
classes." -> "That is, all classes for which loader has been recorded 
as an initiating loader."


thanks,

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/16/20 11:42 AM, serguei.spit...@oracle.com wrote:

Hi Mandy,

I have a couple of minor comments on the Serviceability spec update.

http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java.udiff.html

 Replace: "The returned name is the same form as ..." => "The returned 
name is of the same form as ..."



Example is:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/Type.java.udiff.html

  + * Returns the name of this type. The result is of the same form as


http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java.udiff.html
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java.udiff.html

Both files above have this: "a `class` file representation".
It feels like something is wrong with the `class` part.
Should it say `class file` or maybe use some other formatting?

Otherwise, the Serviceability spec update looks great.
I'll look at the non-Serviceability spec changes too.


Thanks for catching these typo/format issue.  I have fixed them in my 
local repo:


diff --git 
a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java 
b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
--- 
a/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java
+++ 
b/src/java.instrument/share/classes/java/lang/instrument/Instrumentation.java

@@ -391,7 +391,7 @@
  * 
  * A class or interface creation can be triggered by one of the 
following:

  * 
- * by loading and deriving a class from a `class` file 
representation
+ * by loading and deriving a class from a {@code class} file 
representation

  * using class loader (see JVMS {@jvms 5.3}).
  * by invoking {@link 
java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], boolean, 
java.lang.invoke.MethodHandles.Lookup.ClassOption...)

  * Lookup::defineHiddenClass} that creates a {@link Class#isHidden
diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java 
b/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java

--- a/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/ReferenceType.java
@@ -85,7 +85,7 @@
 {
 /**
  * Returns the name of this {@code ReferenceType} object.
- * The returned name is the same form as the name returned by
+ * The returned name is of the same form as the name returned by
  * {@link Class#getName()}.
  *
  * @return a string containing the type name.
diff --git a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java 
b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java

--- a/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/VirtualMachine.java
@@ -137,7 +137,7 @@
  * 
  * A class or interface creation can be triggered by one of the 
following:

  * 
- * by loading and deriving a class from a `class` file 
representation
+ * by loading and deriving a class from a {@code class} file 
representation

  * using class loader (see JVMS {@jvms 5.3}).
  * by invoking {@link 
java.lang.invoke.MethodHandles.Lookup#defineHiddenClass(byte[], boolean, 
java.lang.invoke.MethodHandles.Lookup.ClassOption...)

  * Lookup::defineHiddenClass} that creates a {@link Class#isHidden

Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/14/20 11:51 AM, Paul Sandoz wrote:

Looks good to me (not familiar with all the code areas.

Minor suggestion:

MethodHandles.java

1811  * ASCII periods. For the instance of {@link java.lang.Class} 
representing {@code C}:
1812  * 
1813  *  {@link Class#getName()} returns the string {@code GN + "/" + 
},
1814  *  even though this is not a valid binary class or interface 
name.
1815  *  {@link Class#descriptorString()} returns the string
1816  *  {@code "L" + N + ";" + "/" +  },
1817  *  even though this is not a valid type descriptor name.
1818  * 

Add another bullet:

“
even though this is not a valid type descriptor name; and

- therefore {@link Class#describeConstable} returns an empty {@code Optional}.
“

?


OK.   I add this bullet:

- Class.describeConstable() returns an empty optional as C cannot be 
described in nominal form.


The webrev and spec was updated [1] for descriptor string to be of the 
form "Lfoo/Foo.1234;" to mitigate the compatibility risk.  Th


Specdiff with serviceability spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/
Specdiff without svc spec change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-descriptor-string/overview-summary.html

Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-descriptor-string/

Svc spec change webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-svc-spec-changes/

thanks
Mandy
[1] 
https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007155.html





Paul.


On Apr 11, 2020, at 8:13 PM, Mandy Chung  wrote:

Please review the delta patch:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-delta/

Incremental specdiff:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
This is to follow a discussion [1] on Class::descriptorString and 
MethodType::descriptorString for hidden classes.   The proposal is to define 
the descriptor string for a hidden class of this form:
 "L" + N + ";" + "/" + 

The spec of `Lookup::defineHiddenClass`, `Class::descriptorString` and 
`MethodType::descriptorString` are updated to return the descriptor of this 
form for a hidden class.   To support hidden class, 
`java.lang.invoke.TypeDescriptor` spec is revised such that a `TypeDescriptor` 
object can represent an entity that may not be described in nominal form. 
This change affects JVM TI, JDWP and JDI.The spec change includes a couple 
other JVM TI and java.instrument spec clarification w.r.t. hidden classes that 
Serguei has been working on.



Mandy
[1] https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007093.html


As a record, the above patch is applied on the top:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06/

webrev.06 includes the following changes that have been reviewed:
1. rename "weak hidden" and Klass::is_hidden_weak to is_non_strong_hidden
2. remove unused `setImplMethod` method from lambda proxy class
3. include Serguei's patch to fix JDK-8242166: regression in JDI ClassUnload 
events
4. drop the uniqueness guarantee of the suffix of the hidden class's name

On 3/31/20 8:01 PM, Mandy Chung wrote:

Thanks to the review feedbacks.

Updated webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04/
Delta between webrev.03 and webrev.04:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta/

Summary of changes is:
1. Update javac to retain the previous behavior when compiling to target release 
<= 14 where lambda proxy class is not a nestmate
2. Rename Class::isHiddenClass to Class::isHidden
3. Address Joe's feedback on the CSR to document the behavior of the relevant 
methods in java.lang.Class for hidden classes
4. Add test case for unloadable class with nest host error
5. Add test cases for hidden classes with different kinds of class or interface
6. Update dcmd to drop "weak hidden class" and refer it as "hidden class"
7. Small changes in response to Remi, Coleen, Paul's review comments
8. Fix copyright headers
9. Fix @modules in tests

Most of the changes above have also been reviewed as separate patches.

Thanks
Mandy

On 3/26/20 4:57 PM, Mandy Chung wrote:

Please review the implementation of JEP 371: Hidden Classes. The main changes 
are in core-libs and hotspot runtime area.  Small changes are made in javac, VM 
compiler (intrinsification of Class::isHiddenClass), JFR, JDI, and jcmd.  CSR 
[1]has been reviewed and is in the finalized state (see specdiff and javadoc 
below for reference).

Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03

javadoc/specdiff
http://cr.openjdk

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

Please review the delta patch:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06-delta/

Incremental specdiff:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff-inc/overview-summary.html
This is to follow a discussion [1] on Class::descriptorString and 
MethodType::descriptorString for hidden classes.   The proposal is to 
define the descriptor string for a hidden class of this form:

    "L" + N + ";" + "/" + 

The spec of `Lookup::defineHiddenClass`, `Class::descriptorString` and 
`MethodType::descriptorString` are updated to return the descriptor of 
this form for a hidden class.   To support hidden class, 
`java.lang.invoke.TypeDescriptor` spec is revised such that a 
`TypeDescriptor` object can represent an entity that may not be 
described in nominal form.     This change affects JVM TI, JDWP and 
JDI.    The spec change includes a couple other JVM TI and 
java.instrument spec clarification w.r.t. hidden classes that Serguei 
has been working on.




Mandy
[1] 
https://mail.openjdk.java.net/pipermail/valhalla-dev/2020-April/007093.html



As a record, the above patch is applied on the top:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.06/

webrev.06 includes the following changes that have been reviewed:
1. rename "weak hidden" and Klass::is_hidden_weak to is_non_strong_hidden
2. remove unused `setImplMethod` method from lambda proxy class
3. include Serguei's patch to fix JDK-8242166: regression in JDI 
ClassUnload events

4. drop the uniqueness guarantee of the suffix of the hidden class's name

On 3/31/20 8:01 PM, Mandy Chung wrote:

Thanks to the review feedbacks.

Updated webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04/ 


Delta between webrev.03 and webrev.04:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta/ 



Summary of changes is:
1. Update javac to retain the previous behavior when compiling to 
target release <= 14 where lambda proxy class is not a nestmate

2. Rename Class::isHiddenClass to Class::isHidden
3. Address Joe's feedback on the CSR to document the behavior of the 
relevant methods in java.lang.Class for hidden classes

4. Add test case for unloadable class with nest host error
5. Add test cases for hidden classes with different kinds of class or 
interface

6. Update dcmd to drop "weak hidden class" and refer it as "hidden class"
7. Small changes in response to Remi, Coleen, Paul's review comments
8. Fix copyright headers
9. Fix @modules in tests

Most of the changes above have also been reviewed as separate patches.

Thanks
Mandy

On 3/26/20 4:57 PM, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The main 
changes are in core-libs and hotspot runtime area.  Small changes are 
made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed 
and is in the finalized state (see specdiff and javadoc below for 
reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/ 



JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf 



CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359
[2] https://bugs.openjdk.java.net/browse/JDK-8240338
[3] https://bugs.openjdk.java.net/browse/JDK-8230502

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/6/20 9:56 AM, serguei.spit...@oracle.com wrote:


The suggested fix is:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/valhalla-jdi-regression-8242166.1/



This patch looks okay. I'll include in my local patch.

On 4/6/20 11:00 AM, Chris Plummer wrote:


I think that's fine but I don't think it should be done in the context 
of this Vahalla webrev since it has nothing to do with Vahalla. I'd 
suggest filing an RFE and pushing it to jdk/jdk. Easier to track that 
way if there are issues down the road.




I am okay to follow up as a separate RFE.

thanks
Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

Hi Peter,

On 4/4/20 3:58 AM, Peter Levart wrote:


Here I think, you are not quite right. First I need to clarify that we 
are talking about the case where the method reference in above example 
is not converted to lambda by javac, so the proxy class needs to 
invoke the superclass method directly (without the help of lambda 
bridge). I did an experiment and compiled the example with JDK 13 
javac, where the patch for (JDK-8234729) is not applied yet. What I 
get from this compilation is the following metafactory bootstrap 
method invocation:


  0: #35 REF_invokeStatic 
java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;

    Method arguments:
  #42 ()V
  #43 REF_invokeVirtual test/LambdaTest.m:()V
  #42 ()V

The #43 is the implMethod method handle and it is the following:

  #43 = MethodHandle   5:#44  // REF_invokeVirtual 
test/LambdaTest.m:()V

  #44 = Methodref  #2.#45 // test/LambdaTest.m:()V
  #45 = NameAndType    #46:#6 // m:()V
  #46 = Utf8   m
   #2 = Class  #4 // test/LambdaTest
   #4 = Utf8   test/LambdaTest

*BUT* the class that looks up this MH from the constant pool is the 
subclass (test.sub.LambdaTestSub) which is equivalent to the following 
programmatic lookup:


    var mh = MethodHandles.lookup().findVirtual(LambdaTest.class, 
"m", MethodType.methodType(void.class));

    System.out.println(mh.type());

and this results in a method handle of the following type: 
(LambdaTestSub)void


which is correct since the method handle *MUST* check that the passed 
in instance (this) is of type LambdaTestSub or subtype or else the 
"protected" access would be violated.


And since the ref type is REF_invokeVirtual, the 
AbstractValidatingLambdaMetafactory assigns the following to the 
implClass:


    this.implMethodType = implMethod.type();
    this.implInfo = caller.revealDirect(implMethod);
    switch (implInfo.getReferenceKind()) {
    case REF_invokeVirtual:
    case REF_invokeInterface:
    this.implClass = implMethodType.parameterType(0);

...which makes the implClass be LambdaTestSub in this case. Which is 
correct again since we want InnerClassLambdaMetafactory to decide that 
this is the special case for proxy to invoke the method via method 
handle:


    useImplMethodHandle = 
!implClass.getPackageName().equals(implInfo.getDeclaringClass().getPackageName())
    && 
!Modifier.isPublic(implInfo.getModifiers());


If implClass was test.LambdaTest as you said above, this condition 
would evaluate to false, since implInfo is "invokeVirtual 
test.LambdaTest.m:()void" in above case.




My bad.  I mixed up with implClass and implInfo cracked from implMethod 
in your question.


implInfo::getDeclaringClass() returns the declaring class of the 
resolved method, which is the superclass (which is what I have been 
thinking for test.LambdaTest).


implClass is the target type of the method reference. 
AbstractValidatingLambdaMetafactory has clear comment:


    final MethodType implMethodType;  // Type of the implMethod 
MethodHandle "(CC,int)String"
    final Class implClass; // Class for referencing 
the implementation method "class CC"



So everything is OK, but my original question was the following: The 
name of the generated proxy class is derived from the targetClass 
which is the caller's lookup class. In this example the caller is 
LambdaTestSub and this is the same as implClass in this case.


Yes.

Would those two classes always be the same in the case where the 
evaluation of the above `useImplMethodHandle` boolean matters? I mean, 
the decision about whether to base the proxy invocation strategy on 
method handle or direct bytecode invocation is based on one class 
(implClass), but the actual package of the proxy class which 
effectively influences the bytecode invocation rights is taken from 
another class (targetClass).


On one hand the package of the proxy class has to be the same as 
targetClass if the proxy wants to be the nestmate of the targetClass 
(for example to have private access to the members of the nest). But 
OTOH it needs to be the same package also with implClass so that the 
above decision of the proxy invocation strategy is correct. I have a 
feeling that for protected virtual methods, this is true, because the 
type of the 0-argument of such method handle is always the lookup 
class. But am I right?


What do you think of another alternative to invoking the super 
protected method in other package: What if the LMF would decide to 
base the name of the proxy class on the implInfo.getDeclaringClass() 
in such case? It would not have to be a nestmate of any class, just in 

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Mandy Chung]

On 4/4/20 9:16 AM, Peter Levart wrote:

Hi Mandy,

Just another observation of the code in InnerClassLambdaMetafactory...

For the useImplMethodHandle case you generate the protectedImplMethod 
static field to hold the MH and a static setter method:


    mv = cw.visitMethod(ACC_PRIVATE + ACC_STATIC,
    "setImplMethod", DESCR_SET_IMPL_METHOD,
    null, null);

...but then later after you define the class you inject the MH via a 
"staticSetter" method handle:


    MethodHandle mh = 
lookup.findStaticSetter(lookup.lookupClass(), NAME_FIELD_IMPL_METHOD, 
MethodHandle.class);

    mh.invokeExact(implMethod);

So you don't invoke the generated setter method but set the field via 
special kind of method handle (equivalent to putstatic bytecode).

You can remove the setImplMethod method generation code.



Good catch.   Removed the unused method.

Mandy


Regards, Peter