RFR (S) 8236901: 8232759 missed a test case
mikhailo.seledtsov at oracle.com
mikhailo.seledtsov at oracle.com
Thu Jan 16 21:40:38 UTC 2020
Looks good to me.
Please update copyright year for ||
*test/hotspot/jtreg/runtime/Metaspace/DefineClass.java *before push (no
need for another webrev).
Thank you,
Misha
**
On 1/15/20 5:10 PM, coleen.phillimore at oracle.com wrote:
>
>
> 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