RFR (S) 8236901: 8232759 missed a test case

Tue Jan 14 16:39:55 UTC 2020

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