RFR (S) 8236901: 8232759 missed a test case
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Thu Jan 16 01:10:48 UTC 2020
On 1/15/20 8:05 PM, David Holmes wrote:
> 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!
Thanks for the alert.
>
>> 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.
>
Yeah, it's not expensive and 2 gc's work now but it might change in the
future (see comment).
Thanks for the review.
Coleen
> 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