RFR (S) 8236901: 8232759 missed a test case

[David Holmes]

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

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