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

[Stefan Johansson]

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,
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/1d6527a4/attachment.htm>