<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi, Sorry Stefan I think I liked your .00 version better. Now
there's a level of indirection and it doesn't seem to save any code
because the call sites are slightly different.<br>
<br>
Coleen<br>
<br>
<div class="moz-cite-prefix">On 04/25/2013 08:07 AM, Stefan Karlsson
wrote:<br>
</div>
<blockquote cite="mid:51791C94.7070604@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Estefank/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>
</blockquote>
<br>
</body>
</html>