RFR (S) 8236901: 8232759 missed a test case

[Ioi Lam]

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

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