<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 04/24/2013 11:42 PM, Coleen
Phillimore wrote:<br>
</div>
<blockquote cite="mid:517851C8.50802@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 4/24/2013 4:49 PM, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:5178455A.9000206@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 4/24/13 10:32 PM, Coleen
Phillimore wrote:<br>
</div>
<blockquote cite="mid:5178414C.7090607@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Stefan,<br>
<br>
Is there a common place for this code? <br>
</div>
</blockquote>
<br>
I don't think there's any common place in the GCs for this. I'm
open for suggestions if you know a good place in the runtime
code.<br>
</blockquote>
<br>
memory/genCollectedHeap? I have no idea...<br>
<blockquote cite="mid:5178455A.9000206@oracle.com" type="cite"> <br>
<blockquote cite="mid:5178414C.7090607@oracle.com" type="cite">
<div class="moz-cite-prefix"> It's 4 copies of the similar
code, which takes me a while to find when I set out looking
for it. <br>
</div>
</blockquote>
<br>
Yes, I tried to make them look alike so that we/I have an
opportunity to extract it out to one function. The fact that CMS
does this differently made me hesitant to combine them.<br>
<br>
<blockquote cite="mid:5178414C.7090607@oracle.com" type="cite">
<div class="moz-cite-prefix"> In the future, I think
StringTable::unlink() could be moved after CLDG::purge()
which I thought should be around here too (but it's not).
The only reason StringTable::unlink() used to be here was
because it used to have Oops in it.</div>
</blockquote>
<br>
Don't you mean the SymbolTable? I'm OK with moving the
SymbolTable, but not the StringTable ...<br>
</blockquote>
<br>
I did mean SymbolTable.<br>
<br>
<blockquote cite="mid:5178455A.9000206@oracle.com" type="cite"> <br>
<blockquote cite="mid:5178414C.7090607@oracle.com" type="cite">
<div class="moz-cite-prefix"> Can you unite this duplicated
code?<br>
</div>
</blockquote>
<br>
It's easy to unit the code for psMarkSweep, genMarkSweep,
g1MarkSweep and psParallelCompact but it's a bit more work to
also unify the CMS code. Would it be good enough for this patch
if I united the former GCs? And maybe handle CMS when G1 also
starts doing class unloading?<br>
</blockquote>
<br>
Ok, keep it in mind for future work. It would be nice to have the
runtime GC things be called in one place or easy to find.<br>
<br>
I semi-reviewed the code and except for the GC part (which is most
of the change). We don't pass the keep_alive closure anymore for
compiledICHolders so I guess this makes sense if this is what the
keep_alive closure did. So this is a partial review.<br>
</blockquote>
<br>
I've update the patch to gather the unloading code in a helper class
called GCUnloading. What do you think?<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~stefank/8013160/webrev.01/">http://cr.openjdk.java.net/~stefank/8013160/webrev.01/</a><br>
<br>
thanks,<br>
StefanK<br>
<br>
<blockquote cite="mid:517851C8.50802@oracle.com" type="cite"> <br>
Thanks,<br>
Coleen<br>
<blockquote cite="mid:5178455A.9000206@oracle.com" type="cite"> <br>
thanks,<br>
StefanK<br>
<blockquote cite="mid:5178414C.7090607@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Coleen<br>
<br>
On 4/24/2013 4:18 PM, Stefan Karlsson wrote:<br>
</div>
<blockquote cite="mid:51783E0D.5080408@oracle.com" type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/8013160/webrev.00/">http://cr.openjdk.java.net/~stefank/8013160/webrev.00/</a><br>
<br>
There's some remnants from the PermGen that's confusing in
the class unloading code. This patch removes the unnecessary
code, cleans up some comments and unifies the code structure
over different GCs.<br>
<br>
From the Bug report:<br>
---<br>
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
The PermGen Removal changed CompiledICHolders from being
Java objects to C++ objects. <br>
<br>
CodeCache::do_unloading(...) used to take a keep_alive
closure that was applied to CompiledICHolders deep down in
nmethod::do_unloading. Since CompileICHolders don't move
anymore, and we have other ways to keep them alive, we don't
use the keep_alive closures in these functions anymore. <br>
<br>
Because of this, CodeCache::do_unloading will not populate
the marking stacks, and we can remove the call to drain the
stacks after the calls to CodeCache::do_unloading. <br>
<br>
This also has the neat effect that we now can verify that
the marking has really completed before we start unloading
classes, which makes the code easier to maintain/understand.<br>
---<br>
<br>
thanks,<br>
StefanK<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>