<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Sep 8, 2009, at 4:06 PM, <a href="mailto:Y.S.Ramakrishna@Sun.COM">Y.S.Ramakrishna@Sun.COM</a> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Sorry about doing the review in bits and pieces ... I underdstand<br>now that some of my comments below may have been misplaced,<br>given what i see on reading further into the webrev.<br></div></blockquote><div><br></div><div>It's not a problem; the early feedback is helpful, and I hope in turn you don't mind that the target is moving as I discover more bugs.</div><div><br></div><div>(Ideally, I'd like to push a change that will pass JPRT with the feature turned on to the maximum level... That might not be possible, in which case I'd like at least to pass current JRuby regression tests with the feature turned on to the "medium" level.)</div><br><blockquote type="cite"><div>...and after having seen the following hunk:-<br><br>--- old/src/share/vm/memory/sharedHeap.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>2009-09-08 01:01:08.000000000 -0700<br>+++ new/src/share/vm/memory/sharedHeap.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>2009-09-08 01:01:08.000000000 -0700<br>...<br>@@ -157,10 +166,17 @@<br><br> if (!_process_strong_tasks->is_task_claimed(SH_PS_CodeCache_oops_do)) {<br> if (so & SO_CodeCache) {<br>+ assert(collecting_perm_gen, "scanning all of code cache");<br> CodeCache::oops_do(roots);<br>+ } else if (so & (SO_SystemClasses|SO_AllClasses)) {<br>+ if (!collecting_perm_gen) {<br>+ // if we are collecting from class statics, but we are not going<br>+ // to visit all of the CodeCache, collect from the non-perm roots if any:<br>+ CodeCache::scavenge_root_nmethods_oops_do(roots);<br>+ }<br> }<br> // Verify if the code cache contents are in the perm gen<br>- NOT_PRODUCT(CodeCache::oops_do(&assert_is_perm_closure));<br>+ NOT_PRODUCT(CodeCache::asserted_non_scavengable_oops_do(&assert_is_perm_closure));<br> }<br><br>I see that the following comment is misplaced:-<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite"> Would it be worthwhile to make this scavengable list doubly linked<br></blockquote><blockquote type="cite"> so as to allow constant time deletion?<br></blockquote><font class="Apple-style-span" color="#006312"><br></font>... because your intention, in fact, appears to be to treat these<br>scavengable list of nmethods as strong roots (even if they do not<br>have activations on thread stacks) and to wait for a full GC to<br>reclaim or flush them. Could there be situations where these nmethods<br>might hold the sole references to an otherwise unreachable oop?<br>Might that cause a visible heap bloat until such time as a full gc<br>will be called which will reclaim them?<br></div></blockquote><div><br></div><div>Actually, I think you've pointed out a design bug: I want to treat those guys as weak roots (comparable to perm-gen references in the inactive but live nmethods within the code cache).</div><br><blockquote type="cite"><div>Somewhat relatedly, you weakened the following assertion in nmethod.cpp:-<br><br>@@ -1350,7 +1372,10 @@<br> return false;<br> }<br> }<br>- assert(unloading_occurred, "Inconsistency in unloading");<br>+ // If ScavengeRootsInCode is true, an nmethod might be unloaded<br>+ // simply because one of its constant oops has gone dead.<br>+ // No actual classes need to be unloaded in order for this to occur.<br>+ assert(unloading_occurred || ScavengeRootsInCode, "Inconsistency in unloading");<br> make_unloaded(is_alive, obj);<br> return true;<br> }<br><br>Was this necessitated because of the above possibility?</div></blockquote><div><br></div><div>No, I think it is simply an out-of-date assert that is incompatible with the logic of the new feature. (Or a cue that my design is wrong.) It says that the only way an nmethod's embedded weak oops can fail is if class unloading has just occurred. But this is not true if we allow new sources of non-perm oops into the code. If a random app-generated oop makes its way into the code, and the oops goes dead, the code needs to be thrown away. At least, that's my current thinking. A nice-to-have add on would be for the compiler to say "null this constant out when it goes dead" but we don't have a way to say that now.</div><br><blockquote type="cite"><div>(By the way,<br>please make sure to merge yr changes with my recent push of some<br>related changes for 4957990.)<br></div></blockquote><div><br></div>Will do. I see them in hotspot-gc.</div><div><br><blockquote type="cite"><div>rest looks good (although as you hinted, a suitable structuring of<br>the active nmethod "do_once" infrastructure so as to call the related<br>prologue/epilogue in a more structured fashion might be worth thinking<br>about). I'll go look at yr JPRT job later today and see if i have<br>any ideas about the problems you are encountering.<br></div></blockquote></div><br><div>I'm experimenting with abolishing the "do_nmethods" bit altogether, and having closures do the "do_nmethods" thing by default, and with test-and-set marking to ensure once-only processing. I suspect it will be easier to fix bugs from that starting point, than from the webrev you are looking at.</div><div><br></div><div>-- John</div></body></html>