Review quest for JDK-7067973: test/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java hanging intermittently

[Leonid Mesnik]

On 11/28/2013 04:33 PM, Staffan Larsen wrote:
>
> On 28 nov 2013, at 11:16, Leonid Mesnik <leonid.mesnik at oracle.com 
> <mailto:leonid.mesnik at oracle.com>> wrote:
>
>> Eric, Mandy
>>
>> Sorry that I looping on very late step. It is not a review just 
>> suggestion.
>> We have whitebox API in Hotspot which includes fullGC() method. It 
>> could be used to reliably provoke full GC.
>> See example in 
>> http://hg.openjdk.java.net/jdk8/tl/hotspot/file/c9f439732b18/test/runtime/interned/SanityTest.java
>>
>> Should such approach works for you?
>
> It’s not necessary. System.gc is implemented as a full GC for all of 
> our GC implementations. I don’t think there is a problem in relying on 
> that fact.
>
>>
>> Also please note that your new variant of test fails if any of GC is 
>> set explicitly. It is incompatible with GC setting.
>> We set GC's and GC-related options during Promotion/Nightly/PIT in 
>> Hotspot/SVC. For us is better if test just works
>> with any GC set externally.
>
> This is broken (as has been discussed many times). Tests *need* to be 
> able to provide their own flags without someone overriding them and 
> still expecting the test to work.
>
I agree that this is not good however it is how all works now. There are 
several proposals about jtreg improvement but they are not implemented yet.

Leonid
> /Staffan
>
>>
>> Do you need to run it with all GC each time?
>>
>> Leonid
>> On 11/28/2013 09:21 AM, Eric Wang wrote:
>>> Hi Mandy,
>>>
>>> Yes, I have tested and all settings are passed, as you mentioned the 
>>> test hangs with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent and 
>>> default heap size as no GC happens on Old Gen. That is why to add 
>>> -Xmx2m and big object to make sure GC happens.
>>>
>>> I didn't realized the -Xconcgc is same as -XX:+UseConcMarkSweepGC, i 
>>> have updated the webrev:
>>> http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/ 
>>> <http://cr.openjdk.java.net/%7Eewang/JDK-7067973/webrev.00/>
>>>
>>> Thanks,
>>> Eric
>>> On 2013/11/27 10:17, Mandy Chung wrote:
>>>> Hi Eric,
>>>>
>>>> I'll defer this to the serviceability team to sponsor it and also 
>>>> get one more review.
>>>>
>>>> I don't think you need all 7 @runs.  -Xconcgc is equivalent to 
>>>> setting -XX:+UseConcMarkSweepGC. For G1 and CMS, you should use 
>>>> -XX:+ExplicitGCInvokesConcurrent so that System.gc will force a GC 
>>>> in foreground that you can count the GC reliably. The test wants to 
>>>> get notified for each System.gc and if there is any GC caused by 
>>>> allocation failure, the test would fail due to the unexpected GC 
>>>> count.  It seems that you may run into this issue setting -Xmx2m.
>>>>
>>>> Have you got the test passed in all settings?   I'm seeing that the 
>>>> test hangs with -XX:+UseG1GC -XX:+ExplicitGCInvokesConcurrent 
>>>> without -Xmx2m.  Looks like there is no GC in the old gen - I'm not 
>>>> familiar with G1 if it allocates the big object in the old gen. 
>>>> Jarolsav - can you help Eric diagnose this issue?  I recalled you 
>>>> ran into something like that before - maybe Staffan?
>>>>
>>>> thanks
>>>> Mandy
>>>>
>>>> On 11/25/2013 8:53 PM, Eric Wang wrote:
>>>>> Hi Mandy,
>>>>>
>>>>> 1. for L34-40, executing tests with 7 settings is trying to cover 
>>>>> more cases (normal cases and special cases), especially last 3 
>>>>> settings, as found that the test hung if using vm option 
>>>>> "-XX:+ExplicitGCInvokesConcurrent" with one of 3 options 
>>>>> -XX:+UseG1GC, -XX:+UseConcMarkSweepGC or -Xconcgc
>>>>>
>>>>> 2. for L61, that is right, the test has been updated. please review.
>>>>> http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Eewang/JDK-7067973/webrev.00/>
>>>>>
>>>>> Thanks,
>>>>> Eric
>>>>> On 2013/11/26 8:37, Mandy Chung wrote:
>>>>>> Hi Eric,
>>>>>>
>>>>>> On 11/24/2013 7:41 PM, Eric Wang wrote:
>>>>>>> Hi Mandy & All,
>>>>>>>
>>>>>>> Sorry for late!
>>>>>>> The webrev below is just finished based on the comments from 
>>>>>>> peers, please help to review.
>>>>>>> http://cr.openjdk.java.net/~ewang/JDK-7067973/webrev.00/ 
>>>>>>> <http://cr.openjdk.java.net/%7Eewang/JDK-7067973/webrev.00/>
>>>>>>>
>>>>>>
>>>>>> Thanks for the patch that looks okay.  Some comments:
>>>>>>
>>>>>> L34-40: can you explain why you want to run all 7 settings?  I 
>>>>>> would expect one for each collector.
>>>>>> L61: I think the static checker variable is meant to be a local 
>>>>>> variable (and looks like "pools" and "managers" don't need to be 
>>>>>> static variable).
>>>>>>
>>>>>> Mandy
>>>>>>
>>>>>>> Thanks,
>>>>>>> Eric
>>>>>>> On 2013/11/15 10:55, Mandy Chung wrote:
>>>>>>>> Hi Eric,
>>>>>>>>
>>>>>>>> On 11/14/2013 6:16 PM, Eric Wang wrote:
>>>>>>>>> Hi Everyone,
>>>>>>>>>
>>>>>>>>> I'm working on the bug 
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-7067973.
>>>>>>>>>
>>>>>>>>> It is a test bug as the test doesn't guarantee memory 
>>>>>>>>> allocated from the Old Gen, if the used memory is zero and 
>>>>>>>>> doesn't cross the threshold, no notification is sent, so both 
>>>>>>>>> the main thread and Checker thread are blocked to wait for the 
>>>>>>>>> GC notification.
>>>>>>>>>
>>>>>>>>> so the suggested fix is similar as the fix 
>>>>>>>>> ResetPeakMemoryUsage.java 
>>>>>>>>> <http://hg.openjdk.java.net/jdk8/tl/jdk/rev/a0896634ab46> to 
>>>>>>>>> create big object to make sure the old gen usage crosses the 
>>>>>>>>> threshold and run test with different GC vmoptions.
>>>>>>>>
>>>>>>>> What are you looking for specifically?  I have provided the 
>>>>>>>> above information.  I need to see the webrev to provide further 
>>>>>>>> feedback.
>>>>>>>>
>>>>>>>> Mandy
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>> -- 
>> Leonid Mesnik
>> JVM SQE
>


-- 
Leonid Mesnik
JVM SQE

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20131128/f3d7c354/attachment-0001.html