On Mon, 11 Sep 2023 22:18:03 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressed second round of review comments on VThreadEventTest.java
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
> line 45:
>
>> 43: * The test uses custom implementation of the CountDownLatch class.
>> 44: * The reason is we want the state of tested thread to be predictable.
>> 45: * With original CountDownLatch it is not clear what thread state is
>> expected.
>
> "original CountDownLatch" -> "java.util.concurrent.CountDownLatch"
Thanks - fixed now.
> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
> line 106:
>
>> 104: ready1.countDown(); // to guaranty state is not State.WAITING
>> after await(mready)
>> 105: try {
>> 106: Thread.sleep(20000); // big timeout to keep unmounted
>> untill interrupted
>
> untill -> until
Thanks - fixed now.
> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/VThreadEventTest.java
> line 132:
>
>> 130: // keep mounted
>> 131: }
>> 132: LockSupport.parkNanos(10 * TIMEOUT_BASE); // will cause extra
>> mount and unmount
>
> I don't see much sense in TIMEOUT_BASE constant (it's used only here and
> multiplied by 10)
> I think it would be clearer to
> Suggestion:
>
> // park for 10ms; causes extra unmount and mount
> LockSupport.parkNanos(10_000_000L);
Thanks - fixed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp
> line 24:
>
>> 22: */
>> 23:
>> 24: #include <cstdlib>
>
> I suppose this include was needed for abort() only and not needed anymore
Thanks - removed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/VThreadEventTest/libVThreadEventTest.cpp
> line 27:
>
>> 25: #include <cstring>
>> 26: #include <jvmti.h>
>> 27: #include <mutex>
>
> not needed
Thanks - removed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322195487
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322198339
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322201761
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322205612
PR Review Comment: https://git.openjdk.org/jdk/pull/15467#discussion_r1322204613
