On Tue, 19 Apr 2022 00:09:04 GMT, Mandy Chung <mch...@openjdk.org> wrote:
>> 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 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.
> 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();
okay
> 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.
If there is cycle involving a virtual thread then the thread id of its carrier
will be included in array. I think we can do better and include the thread id
of the mounted thread.
> 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`
I think it might be better if the description is changed to "the format to
use". There is already a link to the ThreadDumpFormat enum in method signature.
-------------
PR: https://git.openjdk.java.net/jdk/pull/8166
