RFR (S) 8236901: 8232759 missed a test case

David Holmes david.holmes at oracle.com
Tue Jan 14 22:11:51 UTC 2020 

Hi Coleen,

On 15/01/2020 5:33 am, coleen.phillimore at oracle.com wrote:
> 
> I added some comments in the test and made the C heap message a logging 
> message, so that the output you see in the JTR file has the header 
> (which the test was trying to do).
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236901.02/webrev

Looks fine.

> This test passes for me.  I don't see why it wouldn't pass reliably 
> unless you have a problem on your machine with creating processes.

This is passing for me now as well. I'm going to do an experiment and 
follow up with you separately if there is a problem.

Thanks,
David
-----


> Also, we could change the test to add a whitebox function to 
> getClasses(jstring class_name) to provide the output that 
> GC.class_histogram prints out, but that would be an enhancement.
> 
> Coleen
> 
> 
> On 1/14/20 11:39 AM, coleen.phillimore at oracle.com wrote:
>>
>>
>> On 1/14/20 1:07 AM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 14/01/2020 3:51 pm, Ioi Lam wrote:
>>>>
>>>>
>>>> On 1/13/20 8:08 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> On 14/01/2020 1:04 pm, coleen.phillimore at oracle.com wrote:
>>>>>>
>>>>>>
>>>>>> On 1/13/20 7:36 PM, David Holmes wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> On 14/01/2020 7:08 am, coleen.phillimore at oracle.com wrote:
>>>>>>>> Summary: Use jcmd GC.class-histogram because it also works for 
>>>>>>>> verifying that the classes are loaded.
>>>>>>>
>>>>>>> I'm looking at GC.class_stats and GC.class_histogram output and 
>>>>>>> the actual test and I'm not sure the test was even working the 
>>>>>>> way it was expected before. If I run the test using 
>>>>>>> GC.class_stats I see:
>>>>>>>
>>>>>>> ----------System.out:(20/1962)----------
>>>>>>> sizeof(DefineClass.class) == 8905
>>>>>>> class test.DefineClass
>>>>>>> WARNING: Ran out of C-heap; undercounted 342 total instances in 
>>>>>>> data below
>>>>>>>  1447    46         0        544           0    6752 12 1351 
>>>>>>> 4048    3616    8088   11704 test.DefineClass
>>>>>>>  1448    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1449    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1450    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1451    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1452    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1453    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1454    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1455    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1456    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>>  1457    46         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488    9024 test.DefineClass
>>>>>>> Should have 2 DefineClass instances and we have: 11
>>>>>>
>>>>>> I think the test wants *at least* 2 DefineClasses in these places, 
>>>>>> where it passes false to getClassStats.
>>>>>
>>>>> Not according to this:
>>>>>
>>>>>      // We expect to have two instances of DefineClass here: the 
>>>>> initial version in which we are
>>>>>      // executing and another version which was loaded into our own 
>>>>> classloader 'MyClassLoader'.
>>>>>      // All the subsequent attempts to reload DefineClass into our 
>>>>> 'MyClassLoader' should have failed.
>>>>>      printClassStats(2, false);
>>>>>
>>>>> However this is a complex test and some of the other variants do 
>>>>> expect 11 instances, so this may just be poorly documented in the 
>>>>> test.
>>>>>
>>
>> I can fix the comment in the test.
>>
>>>>>>>
>>>>>>> then
>>>>>>>
>>>>>>> System.gc()
>>>>>>> WARNING: Ran out of C-heap; undercounted 342 total instances in 
>>>>>>> data below
>>>>>>>  1598    47         0        544           0    6752 12 1351 
>>>>>>> 4048    3616    8088    11704 test.DefineClass
>>>>>>>  1599    47         0        544           0    4072 12 1351 
>>>>>>> 4048    3536    5488     9024 test.DefineClass
>>>>>>> Should have 2 DefineClass instances and we have: 2
>>>>>>>
>>>>>>> so the test "passes". But if I then change the test to use 
>>>>>>> GC.class_histogram as suggested it fails!
>>>>>>>
>>>>>>>
>>>>>>> ----------System.out:(7/321)----------
>>>>>>> sizeof(DefineClass.class) == 8909
>>>>>>> class test.DefineClass
>>>>>>> WARNING: Ran out of C-heap; undercounted 342 total instances in 
>>>>>>> data below
>>>>>>> Should have 2 DefineClass instances and we have: 0
>>>>>>> System.gc()
>>>>>>> WARNING: Ran out of C-heap; undercounted 342 total instances in 
>>>>>>> data below
>>>>>>> Should have 2 DefineClass instances and we have: 0
>>>>>>>
>>>>>>
>>>>>> I don't know why it fails for you.  How did you run it? Did you 
>>>>>> test my patch?
>>>>>
>>>>> I applied your change to my test locally. But I realized why it is 
>>>>> probably failing is the:
>>>>>
>>>>> WARNING: Ran out of C-heap; undercounted 342 total instances
>>>>>
>>>>> as the class we are looking for may be one of the instances that 
>>>>> couldn't be counted and presumably reported. Do you see this C-Heap 
>>>>> warning when you run the test?
>>>>>
>>>> David, we have a bug in the code that prints out the warning:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8235402
>>>
>>> Okay ... so the warning is spurious ... does that mean classes are 
>>> missing from the list or not? If yes then it explains why I saw the 
>>> test fail; if not then we still have a problem with the test.
>>
>> No classes are not missing from the list and this warning shouldn't 
>> prevent any classes from being missing from the list.
>>
>>   // Can happen if k is an archived class that we haven't loaded yet.
>>   if (k->java_mirror_no_keepalive() == NULL) {
>>     return NULL;
>>   }
>>
>> It'll return false if it finds a class that isn't loaded yet from the 
>> shared archive.
>>
>> I have bug JDK-8235402 assigned to me because I want to fix this 
>> ad-hoc hashtable once I get the GC.class_stats code out of the way.
>>
>> I don't know why it fails for you then.  Can you send me your .jtr file?
>>
>> thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>>
>>>>> In conclusion I think your fix for the test is fine, but the test 
>>>>> itself and the underlying jcmd stuff has some potential issues.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Coleen
>>>>>>
>>>>>>> This is on a JDK where GC.class_stats still exists.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>> open webrev at 
>>>>>>>> http://cr.openjdk.java.net/~coleenp/2020/8236901.01/webrev
>>>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8236901
>>>>>>>>
>>>>>>>> Ran tier1 which runs the test on all Oracle platforms.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>
>>>>
>>
>

More information about the hotspot-runtime-dev mailing list