Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Wed, 28 Jun 2023 07:37:00 GMT, Serguei Spitsyn wrote:
> The test is great. I realize it is not easy to make it fully reliable though.
> So, will need another pass.
Good point about spurious wakeups.
Updated the test to handle them.
Now for WAITING scenarios the test suspends test thread and ensures it's
waiting. While the thread is suspended (either carrier or virtual) we can
safely verify thread states
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 117:
>
>> 115: } catch (InterruptedException ex) {
>> 116: // expected, ignore
>> 117: }
>
> Shout this code account for spurious wakeups?
Fixed
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 241:
>
>> 239: private static native void waitInNative();
>> 240: // Signals waitInNative() to exit.
>> 241: private static native void endWait();
>
> Q: Where is this native method invoked?
Good catch. Test thread inNative testcase never exits, it's terminated on the
test exit.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 73:
>
>> 71: jint unexpected = state - actual_full;
>> 72: LOG(" ERROR: some unexpected bits are set (%x): %s\n",
>> 73:unexpected, TranslateState(unexpected));
>
> Nit: lines 65, 73 have wrong indent.
Fixed.
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 176:
>
>> 174: while (!time_to_exit) {
>> 175: sleep_ms(100);
>> 176: }
>
> Is this loop reliable in terms of thread states?
The code is pure native (sleep_ms uses native functions, it does not interact
with JVM), so there is no thread state changes
-
PR Comment: https://git.openjdk.org/jdk/pull/14622#issuecomment-1612423518
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246097094
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246096797
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246098484
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1246098388
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Tue, 27 Jun 2023 20:54:27 GMT, Alex Menkov wrote:
>> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier
>> should return STATE_WAITING)
>> New test tests GetThreadState for different thread states.
>> The test detected a bug in the implementation, new issue is created:
>> JDK-8310584
>> Corresponding testcases are commented now in the test, fix for JDK-8310584
>> will uncomment them
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Added spaces in synchronized
The test is great.
I realize it is not easy to make it fully reliable though.
So, will need another pass.
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 117:
> 115: } catch (InterruptedException ex) {
> 116: // expected, ignore
> 117: }
Shout this code account for spurious wakeups?
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
line 241:
> 239: private static native void waitInNative();
> 240: // Signals waitInNative() to exit.
> 241: private static native void endWait();
Q: Where is this native method invoked?
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 73:
> 71: jint unexpected = state - actual_full;
> 72: LOG(" ERROR: some unexpected bits are set (%x): %s\n",
> 73:unexpected, TranslateState(unexpected));
Nit: lines 65, 73 have wrong indent.
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 176:
> 174: while (!time_to_exit) {
> 175: sleep_ms(100);
> 176: }
Is this loop reliable in terms of thread states?
-
PR Review: https://git.openjdk.org/jdk/pull/14622#pullrequestreview-1502440716
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244797022
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244791733
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244809438
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244799190
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
On Mon, 26 Jun 2023 20:57:44 GMT, Andrey Turbanov wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added spaces in synchronized
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java
> line 85:
>
>> 83: }
>> 84: });
>> 85: synchronized(syncObj) {
>
> Suggestion:
>
> synchronized (syncObj) {
Fixed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1244420236
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v2]
> This is follow-up JDK-8307153/JDK-8309612 (JVMTI GetThreadState on carrier > should return STATE_WAITING) > New test tests GetThreadState for different thread states. > The test detected a bug in the implementation, new issue is created: > JDK-8310584 > Corresponding testcases are commented now in the test, fix for JDK-8310584 > will uncomment them Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: Added spaces in synchronized - Changes: - all: https://git.openjdk.org/jdk/pull/14622/files - new: https://git.openjdk.org/jdk/pull/14622/files/c5f669b7..b5b39bcd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14622&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14622&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14622.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14622/head:pull/14622 PR: https://git.openjdk.org/jdk/pull/14622
