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

[Stefan Johansson]

On 2016-05-27 17:17, 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.
Ok, you decide I don't have a strong opinion. I just saw that the other 
call to set_should_concurrent_collect(false) was made from 
CMSCollector::collect_in_background(...), so I thought keeping this call 
in CMSCollector (instead of ConcurrentMarkSweepGeneration) might make 
sense.
>
>>
>> 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.
>
I think using CMSWaitDuration is good, we need to control that the 
concurrent collection doesn't start. What I would like to avoid is 
having the test sleep for 20s, when we should be able to check that the 
reset was successful right after the Full GC is finished. But you are 
probably correct, we should also make sure that 
_should_concurrent_collect wasn't set to false because an actual 
concurrent cycle was started.

Thanks,
Stefan

> 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/20160530/3088e4f1/attachment.htm>