RFR(XS): 8240691: serviceability/sa/ClhsdbCDSJstackPrintAll.java and serviceability/sa/ClhsdbCDSCore.java not work with ZGC

[Yumin Qi]

Thanks. Pushed.

On 3/10/20 11:40 AM, Ioi Lam wrote:
> Looks good to me. Thanks!
>
> - Ioi
>
> On 3/10/20 11:31 AM, Yumin Qi wrote:
>> Hi, Ioi
>>   Thanks for clearing the whole things.
>>   Since it is a trivial fix, I will change the synopsis of the bug at 
>> push using your updated version: ClhsdbCDSJstackPrintAll incorrectly 
>> thinks CDS is in use
>>
>>    Let me know if it is OK for push.
>>
>> Thanks
>> Yumin
>>
>> On 3/10/20 11:11 AM, Ioi Lam wrote:
>>>
>>>
>>> On 3/10/20 10:26 AM, Chris Plummer wrote:
>>>> Ok, that makes sense. I committed a change recently that issues an 
>>>> "echo true" command so all commands that follow are echoed, which 
>>>> is why you now see "verbose true" in the output. So this resulted 
>>>> in it always thinking that shared spaces was enabled, even when 
>>>> not. But this did not cause any difference in behavior I think 
>>>> because of JDK-8240698 (I'm still not real clear on that).
>>>>
>>>> And after fixing this issue and JDK-8240698 we will then start to 
>>>> see issues because ZGC does not support SA, and therefore need a 
>>>> 3rd bug to fix that?
>>>>
>>>
>>> Hi Chris,
>>>
>>>
>>> I think we need to fix things in the following order. The goal is to 
>>> increase test coverage for CDS.
>>>
>>>
>>>
>>> [1] 8240691 - (I renamed the bug to) "ClhsdbCDSJstackPrintAll 
>>> incorrectly thinks CDS is in use")
>>>
>>> After the above is fixed, we still won't run ClhsdbCDSJstackPrintAll 
>>> for jtreg -vmoption:-XX:+UseZGC yet, because this test has 
>>> "@requires vm.cds", which reports false when ZGC is selected.
>>>
>>>
>>>
>>> [2] 8240563 - WB_IsCDSIncludedInVmBuild should support uncompressed 
>>> oops/klasses
>>>
>>> After the above is fixed, we can run a lot of *other* CDS tests with 
>>> CDS+ZGC.
>>>
>>> However, for ClhsdbCDSJstackPrintAll, when jtreg 
>>> -vmoption:-XX:+UseZGC is used
>>>
>>> + the test dumps a CDS archive with ZGC enabled (use compressed oops 
>>> = false)
>>>
>>> + the test tries to use the CDS archive without enabling ZGC (due to 
>>> 8240698), so compressed oops = true. As a result, the VM cannot load 
>>> this CDS archive because the oop compression mode doesn't match.
>>>
>>>
>>>
>>> [3] 8240698 - LingeredApp did not pass getTestJavaOpts() to the 
>>> children process if vmArguments is already specified
>>>
>>> After the above is fixed, ClhsdbCDSJstackPrintAll will dump and run 
>>> with the archive with -XX+UseZGC, so we can test the behavior of SA 
>>> when ZGC is enabled.
>>>
>>>
>>>
>>> But as Chris said, SA doesn't support ZGC yet (JDK-8220624), so we 
>>> probably don't need to fix 8240698 until SA supports ZGC. As a 
>>> precaution, we should disable this test in ProblemList-zgc.txt as a 
>>> part of 8240563.
>>>
>>> Also, I think the behavior of 8240698 may be intentional -- in many 
>>> cases, especially when with "jtreg -vmoption:-Xcomp", you probably 
>>> don't want the LingerApp to run with -Xcomp. So we should have an 
>>> API in LingerApp.startApp() to explicitly allow the passing of 
>>> getTestJavaOpts() to the child process.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>> Chris
>>>>
>>>> On 3/10/20 9:38 AM, Ioi Lam wrote:
>>>>> Hi Chris,
>>>>>
>>>>>
>>>>> The test uses this to check if CDS is on:
>>>>>
>>>>>     List<String> cmds = List.of("flags UseSharedSpaces");
>>>>>     String useSharedSpacesOutput = test.run(theApp.getPid(), cmds,
>>>>>                                                     null, null);
>>>>>     ....
>>>>>     if (!useSharedSpacesOutput.contains("true")) {
>>>>>         // CDS archive is not mapped. Skip the rest of the test.
>>>>>
>>>>> The output looks like this:
>>>>>
>>>>>     hsdb> hsdb> + verbose true
>>>>>     hsdb> + flags UseSharedSpaces
>>>>>     UseSharedSpaces = false command line
>>>>>     hsdb> + quit
>>>>>
>>>>> I think at one point the "if" worked, but the "+ verbose true" 
>>>>> part of the log must have been added recently to cause the "if" to 
>>>>> determine the wrong result.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>>>
>>>>> On 3/9/20 9:33 PM, Chris Plummer wrote:
>>>>>> Hi Yumin,
>>>>>>
>>>>>> While your fix is a more bullet proof way of checking if CDS is 
>>>>>> on, what was broken with the original version (was there a stray 
>>>>>> "false" in the output"), and what does this have to do with ZGC?
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 3/9/20 8:06 PM, Yumin Qi wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>     I made a change after discussed with Ioi, the test should 
>>>>>>> check against UseSharedSpaces instead, it passed with/without 
>>>>>>> change of 8240563 (tested in local). Also passed tier1,2,3.
>>>>>>>
>>>>>>>      URL: http://cr.openjdk.java.net/~minqi/8240691/webrev/
>>>>>>>
>>>>>>>
>>>>>>>     Thanks
>>>>>>>
>>>>>>>      Yumin
>>>>>>>
>>>>>>>
>>>>>>> On 3/6/20 2:26 PM, Yumin Qi wrote:
>>>>>>>> HI,
>>>>>>>>
>>>>>>>>   Can I have your reviews on
>>>>>>>>
>>>>>>>>   Bug: https://bugs.openjdk.java.net/browse/JDK-8240691
>>>>>>>>
>>>>>>>>   webrev: http://cr.openjdk.java.net/~minqi/8240691/webrev/
>>>>>>>>
>>>>>>>>   Summary: With JDK-8232069 
>>>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8232069), CDS works 
>>>>>>>> with UseCompressedOop and UseCompressedClassPointers off. The 
>>>>>>>> code for detecting CDS will report false with ZGC since ZGC 
>>>>>>>> turned off those two flags. The detection code will be changed 
>>>>>>>> in 
>>>>>>>> JDK-8240563(https://bugs.openjdk.java.net/browse/JDK-8240563), 
>>>>>>>> this will cause those two tests fail.
>>>>>>>>
>>>>>>>>
>>>>>>>>    Tests: jtreg to show the tests not selected with ZGC.
>>>>>>>>
>>>>>>>>    Thanks
>>>>>>>>
>>>>>>>>    Yumin
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>