Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Mon, 10 Jul 2023 23:53:03 GMT, Chris Plummer wrote: > What testing did you do? Any -Xcomp testing? Just run with -Xcomp on all mach5 platforms, passed > test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java > line 41: > >> 39: /** >> 40: * The test implements different scenarios to get desired JVMTI thread >> states. >> 41: * For each scenario test also checks states after carrier and virtual >> threads suspend/resume > > Suggestion: > > * For each scenario the test also checks states after carrier and virtual > threads suspend/resume Fixed > test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java > line 45: > >> 43: * Special handling is required for WAITING state scenarios: >> 44: * Spurious wakeups may cause unexpected thread state change and this >> causes test failure. >> 45: * To avoid this test thread should be suspended (i.e. carrier and/or >> mounted virtual thread is suspended). > > Suggestion: > > * To avoid this, the test thread should be suspended (i.e. carrier and/or > mounted virtual thread is suspended). Fixed. - PR Comment: https://git.openjdk.org/jdk/pull/14622#issuecomment-1629939250 PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1259052949 PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1259053151
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Thu, 29 Jun 2023 18:40:50 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: > > updated comment Just a couple of comment suggestions. Otherwise looks good. What testing did you do? Any -Xcomp testing? test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java line 41: > 39: /** > 40: * The test implements different scenarios to get desired JVMTI thread > states. > 41: * For each scenario test also checks states after carrier and virtual > threads suspend/resume Suggestion: * For each scenario the test also checks states after carrier and virtual threads suspend/resume test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/GetThreadStateMountedTest.java line 45: > 43: * Special handling is required for WAITING state scenarios: > 44: * Spurious wakeups may cause unexpected thread state change and this > causes test failure. > 45: * To avoid this test thread should be suspended (i.e. carrier and/or > mounted virtual thread is suspended). Suggestion: * To avoid this, the test thread should be suspended (i.e. carrier and/or mounted virtual thread is suspended). - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14622#pullrequestreview-1523188533 PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1259012273 PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1259012879
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Thu, 29 Jun 2023 18:40:50 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: > > updated comment Ping. need one more reviewer - PR Comment: https://git.openjdk.org/jdk/pull/14622#issuecomment-1626406831
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Fri, 30 Jun 2023 05:54:12 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> updated comment
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
> line 112:
>
>> 110: LOG(" ERROR: all expected 'weak' bits are set\n");
>> 111: }
>> 112: }
>
> Nit: I guess, error message at line 110 has to be:
> `ERROR: not all expected 'weak' bits are set`
> Also, it looks like the check 3a is not really needed as the 3b should cover
> it.
> But I leave it up to you, if you think check 3a gives some convenience.
'weak' value is for 'sleeping' testcase. In the case we have 2 valid states:
JVMTI_THREAD_STATE_SLEEPING and JVMTI_THREAD_STATE_PARKED, but only one of them
should be reported.
Test passes expected_weak == JVMTI_THREAD_STATE_SLEEPING |
JVMTI_THREAD_STATE_PARKED.
check 3a verifies that at least 1 bit is set (JVMTI_THREAD_STATE_SLEEPING,
JVMTI_THREAD_STATE_PARKED or both).
check 3b verifies that only one of the bits is set (i.e. we don't have both
JVMTI_THREAD_STATE_SLEEPING and JVMTI_THREAD_STATE_PARKED)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1248265672
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
On Thu, 29 Jun 2023 18:40:50 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:
>
> updated comment
Thank you for the test. It is nice to have it.
It looks good.
Thanks,
Serguei
test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp
line 112:
> 110: LOG(" ERROR: all expected 'weak' bits are set\n");
> 111: }
> 112: }
Nit: I guess, error message at line 110 has to be:
`ERROR: not all expected 'weak' bits are set`
Also, it looks like the check 3a is not really needed as the 3b should cover it.
But I leave it up to you, if you think check 3a gives some convenience.
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14622#pullrequestreview-1506557170
PR Review Comment: https://git.openjdk.org/jdk/pull/14622#discussion_r1247465477
Re: RFR: JDK-8310066: Improve test coverage for JVMTI GetThreadState on carrier and mounted vthread [v4]
> 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: updated comment - Changes: - all: https://git.openjdk.org/jdk/pull/14622/files - new: https://git.openjdk.org/jdk/pull/14622/files/6c517cef..8918411b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14622=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14622=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
