RFR (S) 8236901: 8232759 missed a test case
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Jan 17 11:56:49 UTC 2020
On 1/16/20 5:50 PM, Ioi Lam wrote:
> 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, Yes, I'll make that private.
Coleen
>
> 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