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

[Jon Masamitsu]

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/20160527/f97fedc5/attachment.htm>