RFR (S) 8236901: 8232759 missed a test case
Ioi Lam
ioi.lam at oracle.com
Thu Jan 16 22:50:27 UTC 2020
Looks good to me too. This method in WhiteBox probably should be
declared as private. No need for a new webrev.
public native int countAliveClasses0(String name);
Thanks
- Ioi
On 1/16/20 1:40 PM, mikhailo.seledtsov at oracle.com wrote:
> 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