RFR (S) 8236901: 8232759 missed a test case
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Wed Jan 15 23:41:07 UTC 2020
David,
Thank you for trying this on JDK 14 because you did find a bug in the
GC.class_stats removal. I had inadvertently made GC.class_histogram
list loaded classes even if there were no instances variables of that
class. This isn't what we want for GC.class_histogram.
For this test, GC.class_histogram isn't a good replacement for
GC.class_stats. I've rewritten the test to call a whitebox function,
and fixed GC.class_histogram.
Misha, Ioi and David, can you review this change:
open webrev at http://cr.openjdk.java.net/~coleenp/2020/8236901.03/webrev
Tested with tier1 on all Oracle platforms, where this test runs. Also
tested locally with ZGC and UseParallelGC.
Thanks,
Coleen
On 1/14/20 5:11 PM, David Holmes wrote:
> 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