RFR (S) 8236901: 8232759 missed a test case
David Holmes
david.holmes at oracle.com
Thu Jan 16 01:05:01 UTC 2020
Hi Coleen,
On 16/01/2020 9:41 am, coleen.phillimore at oracle.com wrote:
>
> 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.
Glad you got to the bottom of that one!
> 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
Fix looks fine, as does use of WB. This seems a little odd in the test
+ if (wb.countAliveClasses("test.DefineClass") ==
expectedCount) break;
+ }
+ checkClasses(expectedCount, true);
as we check the same condition we used to break the loop; but it's not a
major concern.
Thanks,
David
-----
> 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