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

[Jon Masamitsu]

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