<div dir="ltr">It sounds to me that I don't need to change the webrev I sent you. Is that right? --Jungwoo</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Oct 4, 2013 at 6:20 AM, Mikael Gerdin <span dir="ltr"><<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Jon,<div class="im"><br>
<br>
On 10/02/2013 04:34 PM, Jon Masamitsu wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Mikael,<br>
<br>
I heard back from Ramki on the setting of the scanning options and it<br>
likely is not what I thought it was. He commented that your suggestion<br>
about SO_SystemClasses or SO_AllClasses being the reasonable<br>
possibilities sounded right. But I'm a bit hesitant to change to using<br>
a set_root_scanning_option() (discarding the persistent state in<br>
_root_scanning_options) without having a very clear understanding<br>
about how we got to the current code.<br>
<br>
For Jungwoo's change I'd like to continue to use the add / remove<br>
paradigm and investigate the set_root_scanning_option()<br>
separately.<br>
</blockquote>
<br></div>
Fair enough. Let's keep the add/remove code for now.<br>
Hopefully we'll have the time to clean this up at some point in the future. I suspect that a lot of this complexity can be removed completely now that perm gen is gone. CMS desperately needs any decrease in complexity it can get :)<span class="HOEnZb"><font color="#888888"><br>
<br>
/Mikael</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Jon<br>
<br>
On 10/1/2013 10:13 PM, Jon Masamitsu wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 10/1/2013 6:51 AM, Mikael Gerdin wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 09/30/2013 08:29 PM, Jon Masamitsu wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 9/30/13 8:33 AM, Mikael Gerdin wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Jungwoo,<br>
<br>
On 09/25/2013 12:36 AM, Jungwoo Ha wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Mikael,<br>
<br>
I updated the patch as you mentioned.<br>
There are no "set"_root_scanning_option, so I added that.<br>
<a href="http://cr.openjdk.java.net/~rasbold/8024954/webrev/" target="_blank">http://cr.openjdk.java.net/~<u></u>rasbold/8024954/webrev/</a><br>
</blockquote>
<br>
Why do you check CMSClassUnloadingEnabled before checking<br>
should_unload_classes()?<br>
This will cause the code for<br>
ExplicitGCInvokesConcurrentAnd<u></u>UnloadsClasses to break, since it<br>
doesn't depend on CMSClassUnloading being set.<br>
<br>
I think you could just move<br>
"set_root_scanning_option(<u></u>SharedHeap::SO_SystemClasses);<u></u>" into the<br>
existing should_unload_classes() check.<br>
<br>
Upon closer inspection I think that<br>
"set_root_scanning_option(SO_<u></u>AllClasses|SO_Strings|SO_<u></u>CodeCache);"<br>
in the case of !should_unload_classes() is incorrect since we should<br>
be unloading strings every cycle regardless of the class unloading<br>
state.<br>
Maybe it would be better to keep part of your original suggestion and<br>
doing:<br>
<br>
</blockquote>
<br>
Mikael,<br>
<br>
Could you explain your suggestion of using set_root_scanning_option()?<br>
_root_scanning_options is part of CMSCollector and there's only one<br>
CMSCollector<br>
(right?) so whatever is in _root_scanning_options persists from<br>
collection to collection.<br>
I don't know that there are some bits set in _root_scanning_options<br>
that<br>
should<br>
be preserved but it's possible so resetting _root_scanning_options with<br>
set_root_scanning_option() could be losing bits.<br>
</blockquote>
<br>
Jon,<br>
<br>
I'm actually pretty confused by the code which adds and removes<br>
SO_Strings and SO_CodeCache from the CMSCollector's<br>
root_scanning_options.<br>
I don't understand why they should ever be roots for CMS, the only<br>
ScanOptions which make sense to me are SO_SystemClasses or<br>
SO_AllClasses, depending on wether we are going to unload classes<br>
this particular CMS cycle.<br>
<br>
Since I believe that these are the only two reasonable values I think<br>
that the add/remove logic is confusing to readers of the code if<br>
there aren't any other values that need to be preserved.<br>
<br>
I think that this ScanOption mess is somehow related to perm gen<br>
verification which has (obviously) been removed from 8, and the fact<br>
that interned strings were in the perm gen before 7, so if we were<br>
not going to sweep perm gen we would just treat the StringTable as<br>
roots.<br>
</blockquote>
<br>
>From looking at the SO_CodeCache ScanningOption this may be related to<br>
a class unloading bug that was fixed some time during jdk7 development.<br>
C2 keeps some profiling information at call sites which include<br>
pointers to<br>
methods. The profiling information was keeping classes alive. The<br>
desired behavior was that the pointers to methods in the profiling<br>
information would act as weak reference. As I recall the fix for all the<br>
collectors except CMS was straight forward but the CMS fix was<br>
rather complicated and may have used the ScanningOption. I'll<br>
ping Ramki on this to see if he remembers anything about it.<br>
<br>
Jon<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
As soon as (should_unload_classes() != CMSClassUnloadingEnabled) (for<br>
any reason) we will run into the bug we are trying to fix here.<br>
<br>
/Mikael<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Jon<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
if (should_unload_classes()) {<br>
set_root_scanning_option(SO_<u></u>SystemClasses);<br>
set_verifying(should_verify);<br>
return;<br>
} else {<br>
remove_root_scanning_option(<u></u>SO_SystemClasses);<br>
add_root_scanning_option(SO_<u></u>AllClasses);<br>
}<br>
<br>
<br>
The whole chunk of code below<br>
assert(!should_unload_classes(<u></u>)) seems suspect to me, it adds the<br>
strings to the root set if verification is enabled which seems to<br>
suggest that if verification is enabled CMS will never unlink interned<br>
strings. I've been digging around a bit in the history and I think the<br>
code is related to perm gen verification, which does not exist in 8<br>
any more, so we may be able to get rid of it. I want to understand the<br>
reason for adding and removing SO_Strings and SO_CodeCache, because<br>
doing that does not make sense to me in the 8 codebase.<br>
<br>
/Mikael<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Jungwoo<br>
<br>
<br>
On Thu, Sep 19, 2013 at 4:48 AM, Mikael Gerdin<br>
<<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.com</a><br>
<mailto:<a href="mailto:mikael.gerdin@oracle.com" target="_blank">mikael.gerdin@oracle.<u></u>com</a>>> wrote:<br>
<br>
Hi Jungwoo,<br>
<br>
<br>
On 2013-09-17 01:14, Jungwoo Ha wrote:<br>
<br>
Hi,<br>
<br>
We found that hotspot crashes when<br>
CMSClassUnloadingEnabled is<br>
true, and<br>
CMSClassUnloadingMaxInterval > 0.<br>
This is on hotspot24 and u40.<br>
This is easily reproducible with DaCapo tradesoap<br>
benchmark with<br>
heapsize around 200MB.<br>
<br>
The reason for the crash is that CMS sets the root set when<br>
CMSClassUnloadingEnabled is on during the constructor phase<br>
assuming<br>
that every CMS cycle will unload the class.<br>
However, when CMSClassUnloadingMaxInterval > 0, CMS may not<br>
unload<br>
classes ended up crashing.<br>
I think this is apparently a bug, and I attach the fix.<br>
Please take a look at the attached patch.<br>
My changes are resetting the root scanning option on every<br>
CMS<br>
cycle in<br>
setup_cms_unloading_and___<u></u>verification_state() if<br>
CMSCLassUnloadingEnabled<br>
is on.<br>
<br>
Please take a look and let us know how to proceed.<br>
<br>
<br>
Jon filed a bug for this:<br>
<a href="https://bugs.openjdk.java.net/__browse/JDK-8024954" target="_blank">https://bugs.openjdk.java.net/<u></u>__browse/JDK-8024954</a><br>
<<a href="https://bugs.openjdk.java.net/browse/JDK-8024954" target="_blank">https://bugs.openjdk.java.<u></u>net/browse/JDK-8024954</a>><br>
<br>
I think the code change fixes the bug but I'd like it better<br>
if you<br>
changed the code to always set the requested ScanOption values in<br>
setup_cms_unloading_and___<u></u>verification_state<br>
<br>
something like:<br>
if (should_unload_classes()) {<br>
set_root_scanning_option(SO___<u></u>SystemClasses);<br>
} else {<br>
set_root_scanning_option(SO___<u></u>AllClasses|SO_Strings|SO___<u></u>CodeCache);<br>
}<br>
<br>
I also believe that your check for CMSClassUnloadingEnabled can<br>
cause a bug similar to the one you are trying to fix if<br>
ExplicitGCInvokesConcurrentAnd<u></u>__UnloadsClasses would be enabled.<br>
should_unload_classes() should be sufficient to determine which<br>
ScanOptions to set for a particular cms cycle.<br>
<br>
Please send a patch/webrev based on the latest jdk8 sources<br>
<a href="http://hg.openjdk.java.net/__hsx/hotspot-gc/hotspot" target="_blank">http://hg.openjdk.java.net/__<u></u>hsx/hotspot-gc/hotspot</a><br>
<<a href="http://hg.openjdk.java.net/hsx/hotspot-gc/hotspot" target="_blank">http://hg.openjdk.java.net/<u></u>hsx/hotspot-gc/hotspot</a>><br>
Since the function is also messing around with the verification<br>
flags, make sure to run some tests on your changes with<br>
verification<br>
enabled.<br>
<br>
/Mikael<br>
<br>
<br>
Thanks,<br>
Jungwoo<br>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br></div>