<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 06/07/2016 11:48 PM, Stefan
Johansson wrote:<br>
</div>
<blockquote cite="mid:5757BFAE.60007@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
Hi Jon,<br>
<br>
<div class="moz-cite-prefix">On 2016-06-08 04:06, Jon Masamitsu
wrote:<br>
</div>
<blockquote
cite="mid:da9677a6-4023-6ce8-4504-f15b984568c5@oracle.com"
type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Please find the new versions of the webrevs<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8026752/webrev.01/">http://cr.openjdk.java.net/~jmasa/8026752/webrev.01/</a><br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8026752/top_webrev.01/">http://cr.openjdk.java.net/~jmasa/8026752/top_webrev.01/</a><br>
<br>
</blockquote>
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.<br>
</blockquote>
<br>
Thanks for the review.<br>
<br>
If you don't mind, I would rather not add the check before the
System.gc(). I<br>
can think of different scenarios (unlikely perhaps) where the
should-concurrent-collect<br>
flag would not be set for legitimate reasons and would prefer to
avoid the<br>
assertion failure in those cases.<br>
<br>
Jon<br>
<br>
<blockquote cite="mid:5757BFAE.60007@oracle.com" type="cite">
Thanks,<br>
Stefan<br>
<br>
<blockquote
cite="mid:da9677a6-4023-6ce8-4504-f15b984568c5@oracle.com"
type="cite"> There is a delta (but I'd recommend just looking at
the complete<br>
webrevs).<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejmasa/8026752/webrev_delta.00_01/">http://cr.openjdk.java.net/~jmasa/8026752/webrev_delta.00_01/</a><br>
<br>
Thanks.<br>
<br>
Jon<br>
<br>
<div class="moz-cite-prefix">On 5/27/2016 8:17 AM, 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>
<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>
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>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>