<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix"><tt>HI Thomas,</tt><tt><br>
</tt><tt><br>
</tt><tt>The changes look good to me. Thanks for doing the clean
up and adding verification</tt><tt> code.</tt><tt><br>
</tt>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<tt><br>
</tt><tt>One minor point here:</tt><tt><br>
</tt><tt><br>
</tt><tt> const int minimumStringTableSize = 1009;</tt><tt><br>
</tt><tt><br>
</tt><tt>shouldn't this be "NOT_LP64(137) LP64_ONLY(1009)"? or
maybe just </tt><tt>"137"?</tt><tt><br>
</tt><tt><br>
</tt><tt>More comments below</tt><tt> (with an interesting twist
at the very end ...)<br>
</tt><tt><br>
</tt><tt>On 09/25/2013 06:39 AM, Thomas Schatzl wrote:</tt><br>
</div>
<blockquote cite="mid:1380116373.28153.40.camel@cirrus" type="cite">
<pre wrap="">Hi all,
I would like to have reviews for the following change that improves
iteration over the system dictionary to find references to the heap when
not class unloading.
We noticed that going over the system dictionary to find references to
the heap is time consuming and doing needless work.
After the NPG changes the only references to the heap from the system
dictionary are from the protection domain oops. However these are
relatively rare (i.e. mostly NULL), typically magnitudes less in amount
than the number of system dictionary entries.
The idea to improve this part of the gc root scanning is to collect the
protection domain oops into a separate set (ProtectionDomainCacheTable).
The system dictionary entries refer to these entries of the set instead
of directly to the protection domain oops. During gc root scan when
there is no class unloading, instead of looking through all system
dictionary entries, the code then only iterates over the much smaller
set of protection domain oops.
When there is class unloading to find all protection domain oops
referenced by system dictionary entries the changed code still iterates
over the system dictionary, setting a mark on the entries if a reference
is found. After that, in a pass over the protection domain entry set, a
closure is applied on all marked entries (and the mark removed).
Life-cycle analysis for the set entries is done using a reference count
on them. I.e. if the reference count drops to zero, the set entry is
removed.
The original patch for this issue has been contributed by Ioi from the
runtime team, I just took over and tried to document and clean it up
slightly.
The original change from Ioi is available at:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/orig/">http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/orig/</a>
One containing my cleanup and that is subject to this review is at
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/">http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/</a>
For easier review I made a diff from the original to the cleanup version
at <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/diff">http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/diff</a>
Here is a summary of the changes in the cleanup:
- moved some hardcoded constants into global variables similar to the
system dictionary with different default values for 32/64bit operation.
the actual values (32bit: 137, 64bit: 1009) were derived by Ioi in tests
with PSR.
There is now a new global that allows tweaking this if necessary.
- the original code used a "generation counter" to mark protection
domain entries as referenced by classes from the NULL loader. I changed
this to a flag that is reset while iterating over the marked protection
domain set entries.
This seemed simpler and more understandable.</pre>
</blockquote>
<tt>^^^ Yeah! This is a good fix. Now I wonder why I didn't think of
that :-)</tt><br>
<br>
<blockquote cite="mid:1380116373.28153.40.camel@cirrus" type="cite">
<pre wrap="">
- some additional comments
- some verification/printing code
Bugs.openjdk.java.net
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8003420">https://bugs.openjdk.java.net/browse/JDK-8003420</a>
Testing:
fmw/psr, jprt, manual dacapo runs
In the future system dictionary scan during class unloading could be
improved. It seems that actually classes loaded by the primordial/null
class loader never have a protection domain associated (i.e. they are
always in the NULL domain), so actually the entire code in
Dictionary::always_strong_oops_do() could be removed. Although testing
seemed to indicate that there is never a protection domain attached to
the NULL class loader, does someone know if that is actually the case?
</pre>
</blockquote>
<tt><br>
It turns out there ARE protection domains associated with boot
classes.<br>
You can verify that by adding a printf in </tt><tt>Dictionary::always_strong_oops_do,<br>
and run</tt><tt> a simple tests that causes a lot of GC, like the
G1Checker2.java<br>
I attached in this mail.<br>
</tt><tt><br>
The meaning of the PD table is confusing at best. Perhaps we
should<br>
add some comments :-)<br>
<br>
The PD table is a mapping of:<br>
<br>
(InstanceKlass C, initiating class loader ICL) => Protection
Domain PD<br>
<br>
[Note that C.protection_domain(), which is stored in the
java.lang.Class <br>
mirror of C, is NOT the same as PD]<br>
<br>
If such an entry (C, ICL) => PD exists in the table, it means
that <br>
<br>
It's OK for a class Foo to reference C, where<br>
Foo.</tt><tt><tt>protection_domain()</tt> == PD, and<br>
Foo's defining class loader == ICL<br>
<br>
</tt><tt>You can see it in action in gdb:</tt><tt><br>
</tt><tt><br>
</tt><tt>$ hotspot -gdb -cp ~/tmp -XX:+PrintSystemDictionaryAtExit
HelloWorld</tt><tt><br>
</tt><tt>(gdb) b Dictionary::add_protection_domain</tt><tt><br>
</tt><tt>(gdb) c</tt><tt><br>
</tt><tt>Breakpoint 2, Dictionary::add_protection_domain
(this=0xf702fbd0, index=273, <br>
hash=8004670, klass=..., loader_data=0xe3923ac0, </tt><tt>protection_domain=...,<br>
__the_thread__=0xf7008400)</tt><tt><br>
</tt><tt>393 Symbol* klass_name = klass->name();</tt><tt><br>
</tt><tt>(gdb) n</tt><tt><br>
</tt><tt>(gdb) p klass_name->print_on(tty)</tt><tt><br>
</tt><tt>Symbol: 'java/lang/Object</tt><tt><br>
</tt><tt>(gdb) p loader_data</tt><tt><br>
</tt><tt>$2 = (ClassLoaderData *) 0xe3923ac0</tt><tt><br>
</tt><tt><br>
</tt><tt><tt>-XX:+PrintSystemDictionaryAtExit shows there's an entry
for an entry of<br>
java.lang.Object where the initiating class loader is the app
class loader.<br>
<br>
</tt>^java.lang.Object, loader class loader 0xe312bae8a
'sun/misc/Launcher$AppClassLoader'<br>
<br>
The usage of the PD table can be seen in
SystemDictionary::validate_protection_domain()<br>
<br>
The PD table is essentially a cache to avoid repeated Java
up-calls to<br>
ClassLoader.checkPackageAccess().<br>
<br>
Hmm, come to think of it, maybe it's OK to completely drop the PD
table, since <br>
it is just a cache???<br>
<br>
Thanks<br>
<br>
- Ioi<br>
</tt>
<blockquote cite="mid:1380116373.28153.40.camel@cirrus" type="cite">
<pre wrap="">
Thanks,
Thomas
</pre>
</blockquote>
<br>
</body>
</html>