<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/25/2013 11:47 PM, Coleen
Phillimore wrote:<br>
</div>
<blockquote cite="mid:5179A46E.9070605@oracle.com" type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
It only saves code duplication in four of the collectors, the other
two still does the unloading differently.<br>
<br>
Could someone else review the .00 version?<br>
<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>
thanks,<br>
StefanK<br>
<br>
<blockquote cite="mid:5179A46E.9070605@oracle.com" type="cite"> <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>
</blockquote>
<br>
</body>
</html>