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

[Ioi Lam]

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
>>>>>>>
>>>>>
>>>>
>>>
>>
>