<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 2016-05-27 17:17, Jon Masamitsu
wrote:<br>
</div>
<blockquote
cite="mid:1fa166df-b646-ead9-50ad-672ce7efc267@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Stefan,<br>
<br>
Thanks for looking at this. Please see in-line.<br>
<br>
<div class="moz-cite-prefix">On 5/27/2016 3:47 AM, Stefan
Johansson wrote:<br>
</div>
<blockquote cite="mid:574825A8.80706@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<tt>Hi Jon,</tt><br>
<br>
<div class="moz-cite-prefix">On 2016-05-27 07:46, Jon Masamitsu
wrote:<br>
</div>
<blockquote
cite="mid:cef5236e-31b9-0c05-f859-b58619f0ef88@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8026752">https://bugs.openjdk.java.net/browse/JDK-8026752</a><br>
<br>
If a concurrent CMS collection has been scheduled for
Metaspace<br>
needs, it should be cancelled if a full GC is done.<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8026752/webrev.00/">http://cr.openjdk.java.net/~jmasa/8026752/webrev.00/</a><br>
<br>
Fix was verified with the new test TestMetaspaceCMSCancel.java<br>
Stability testing was done with gc_test_suite.<br>
<br>
</blockquote>
<tt>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(...).</tt><tt><br>
</tt></blockquote>
<br>
<tt>No strong reason other than it is the place that occurred to
me.<br>
That reset_after_compaction() was added to take care of work<br>
that needed to be done after a compaction so it seems a good<br>
place to me. If do_compaction_work() is a more obvious place to<br>
you, I can move it there.<br>
</tt></blockquote>
<tt>Ok, you decide I don't have a strong opinion. I just saw that
the other call to </tt><tt><tt>set_should_concurrent_collect(false)
</tt>was made from CMSCollector::collect_in_background(...), so I
thought keeping this call in CMSCollector (instead of
ConcurrentMarkSweepGeneration) might make sense. </tt><br>
<blockquote
cite="mid:1fa166df-b646-ead9-50ad-672ce7efc267@oracle.com"
type="cite"><tt> <br>
</tt>
<blockquote cite="mid:574825A8.80706@oracle.com" type="cite"><tt>
</tt><tt><br>
</tt><tt>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:</tt><tt><br>
</tt></blockquote>
<blockquote cite="mid:574825A8.80706@oracle.com" type="cite"><tt>
</tt><tt>assertTrue(WB.metaspaceShouldConcurrentCollect());</tt><tt><br>
</tt><tt>System.gc();</tt><tt><br>
</tt><tt>assertFalse(WB.metaspaceShouldConcurrentCollect());</tt><tt><br>
</tt></blockquote>
<br>
<tt>Adding -XX:CMSWaitDuration=5000 to the command line makes it
less likely that<br>
a CMS concurrent collections starts and finishes before the call
to System.gc().<br>
The small MetaspaceSize threshold may have been exceeded during
loading of<br>
the systems classes (i.e., before the test program started
running). Adding the<br>
pause() gives CMS time to start and complete a concurrent
collection if the<br>
concurrent collection is not short-circuited by the fix. That
is, I give the<br>
test a better chance of failing.<br>
<br>
I don't think it is quite enough to check MetaspaceGC
_should_concurrent_collect<br>
because the concurrent collection could have already started.<br>
<br>
I could change the test to check the state of the CMS
collector. It should be<br>
in "Resetting" or "Idling". I'll look into that.<br>
<br>
</tt></blockquote>
<tt>I think using </tt><tt>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.<br>
<br>
Thanks,<br>
Stefan<br>
<br>
</tt>
<blockquote
cite="mid:1fa166df-b646-ead9-50ad-672ce7efc267@oracle.com"
type="cite"><tt> Thanks again.<br>
<br>
Jon<br>
<br>
</tt>
<blockquote cite="mid:574825A8.80706@oracle.com" type="cite"><tt>
</tt><tt><br>
</tt><tt>Doing this you should be able to avoid using the
process builder as well.</tt><tt><br>
</tt><tt><br>
</tt><tt>Thanks,</tt><tt><br>
</tt><tt>Stefan</tt><br>
<blockquote
cite="mid:cef5236e-31b9-0c05-f859-b58619f0ef88@oracle.com"
type="cite"> Thanks.<br>
<br>
Jon<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>