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

[Jon Masamitsu]

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/

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/20160607/025044c5/attachment.htm>