Request for Review (s) 8026752: Cancel MetaspaceGC request for a CMS concurrent collection after GC

[sangheon.kim]

Hi Jon,

Looks good and using WhiteBox for the test seems better.

Thanks,
Sangheon


On 06/08/2016 08:43 AM, Jon Masamitsu wrote:
>
>
> On 06/07/2016 11:48 PM, Stefan Johansson wrote:
>> Hi Jon,
>>
>> On 2016-06-08 04:06, Jon Masamitsu wrote:
>>> Please find the new versions of the webrevs
>>>
>>> http://cr.openjdk.java.net/~jmasa/8026752/webrev.01/
>>> http://cr.openjdk.java.net/~jmasa/8026752/top_webrev.01/
>>>
>> Looks good. I think you could add 
>> Asserts.assertTrue(wb.metaspaceShouldConcurrentCollect()) before the 
>> call to System.gc() to make sure there was something to clear. No 
>> need to see a new webrev if you decide to do that. Reviewed.
>
> Thanks for the review.
>
> If you don't mind, I would rather not add the check before the 
> System.gc().  I
> can think of different scenarios (unlikely perhaps) where the 
> should-concurrent-collect
> flag would not be set for legitimate reasons and would prefer to avoid the
> assertion failure in those cases.
>
> Jon
>
>> Thanks,
>> Stefan
>>
>>> There is a delta (but I'd recommend just looking at the complete
>>> webrevs).
>>>
>>> http://cr.openjdk.java.net/~jmasa/8026752/webrev_delta.00_01/
>>>
>>> Thanks.
>>>
>>> Jon
>>>
>>> On 5/27/2016 8:17 AM, Jon Masamitsu wrote:
>>>> Stefan,
>>>>
>>>> Thanks for looking at this.  Please see in-line.
>>>>
>>>> On 5/27/2016 3:47 AM, Stefan Johansson wrote:
>>>>> Hi Jon,
>>>>>
>>>>> On 2016-05-27 07:46, Jon Masamitsu wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8026752
>>>>>>
>>>>>> If a concurrent CMS collection has been scheduled for Metaspace
>>>>>> needs, it should be cancelled if a full GC is done.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/8026752/webrev.00/
>>>>>>
>>>>>> Fix was verified with the new test TestMetaspaceCMSCancel.java
>>>>>> Stability testing was done with gc_test_suite.
>>>>>>
>>>>> I think the fix is good. Just one question regarding where to do 
>>>>> the call. Is there a reason that you have put 
>>>>> MetaspaceGC::set_should_concurrent_collect(false) in 
>>>>> reset_after_compaction()? To me it would make more sense to have 
>>>>> the call in CMSCollector::do_compaction_work(...).
>>>>
>>>> No strong reason other than it is the place that occurred to me.
>>>> That reset_after_compaction() was added to take care of work
>>>> that needed to be done after a compaction so it seems a good
>>>> place to me.  If do_compaction_work() is a more obvious place to
>>>> you, I can move it there.
>>>>
>>>>>
>>>>> Regarding the test, I think it is a bit unfortunate to have to 
>>>>> sleep for 20s to verify this, especially since there have been 
>>>>> efforts to shorten the test-time. What do you think about adding a 
>>>>> WhiteBox-method for checking the value of 
>>>>> MetaspaceGC::_should_concurrent_collect and have the test be 
>>>>> something like:
>>>>> assertTrue(WB.metaspaceShouldConcurrentCollect());
>>>>> System.gc();
>>>>> assertFalse(WB.metaspaceShouldConcurrentCollect());
>>>>
>>>> Adding -XX:CMSWaitDuration=5000 to the command line makes it less 
>>>> likely that
>>>> a CMS concurrent collections starts and finishes before the call to 
>>>> System.gc().
>>>> The small MetaspaceSize threshold may have been exceeded during 
>>>> loading of
>>>> the systems classes (i.e., before the test program started 
>>>> running).  Adding the
>>>> pause() gives CMS time to start and complete a concurrent 
>>>> collection if the
>>>> concurrent collection is not short-circuited by the fix. That is, I 
>>>> give the
>>>> test a better chance of failing.
>>>>
>>>> I don't think it is quite enough to check MetaspaceGC 
>>>> _should_concurrent_collect
>>>> because the concurrent collection could have already started.
>>>>
>>>> I could change the test to check the state of the CMS collector.  
>>>> It should be
>>>> in "Resetting" or "Idling".  I'll look into that.
>>>>
>>>> Thanks again.
>>>>
>>>> Jon
>>>>
>>>>>
>>>>> Doing this you should be able to avoid using the process builder 
>>>>> as well.
>>>>>
>>>>> Thanks,
>>>>> Stefan
>>>>>> Thanks.
>>>>>>
>>>>>> Jon
>>>>>
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160608/0068c77f/attachment.htm>