RFR (M): 8003420: NPG: make new GC root for pd_set
ioi.lam at oracle.com
Wed Sep 25 19:11:52 UTC 2013
The changes look good to me. Thanks for doing the clean up and adding
One minor point here:
const int minimumStringTableSize = 1009;
shouldn't this be "NOT_LP64(137) LP64_ONLY(1009)"? or maybe just "137"?
More comments below(with an interesting twist at the very end ...)
On 09/25/2013 06:39 AM, Thomas Schatzl wrote:
> 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
> 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
> The original change from Ioi is available at:
> One containing my cleanup and that is subject to this review is at
> For easier review I made a diff from the original to the cleanup version
> at http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/diff
> 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.
^^^ Yeah! This is a good fix. Now I wonder why I didn't think of that :-)
> - some additional comments
> - some verification/printing code
> 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?
It turns out there ARE protection domains associated with boot classes.
You can verify that by adding a printf in Dictionary::always_strong_oops_do,
and runa simple tests that causes a lot of GC, like the G1Checker2.java
I attached in this mail.
The meaning of the PD table is confusing at best. Perhaps we should
add some comments :-)
The PD table is a mapping of:
(InstanceKlass C, initiating class loader ICL) => Protection Domain PD
[Note that C.protection_domain(), which is stored in the java.lang.Class
mirror of C, is NOT the same as PD]
If such an entry (C, ICL) => PD exists in the table, it means that
It's OK for a class Foo to reference C, where
Foo.protection_domain() == PD, and
Foo's defining class loader == ICL
You can see it in action in gdb:
$ hotspot -gdb -cp ~/tmp -XX:+PrintSystemDictionaryAtExit HelloWorld
(gdb) b Dictionary::add_protection_domain
Breakpoint 2, Dictionary::add_protection_domain (this=0xf702fbd0,
hash=8004670, klass=..., loader_data=0xe3923ac0, protection_domain=...,
393 Symbol* klass_name = klass->name();
(gdb) p klass_name->print_on(tty)
(gdb) p loader_data
$2 = (ClassLoaderData *) 0xe3923ac0
-XX:+PrintSystemDictionaryAtExit shows there's an entry for an entry of
java.lang.Object where the initiating class loader is the app class loader.
^java.lang.Object, loader class loader 0xe312bae8a
The usage of the PD table can be seen in
The PD table is essentially a cache to avoid repeated Java up-calls to
Hmm, come to think of it, maybe it's OK to completely drop the PD table,
it is just a cache???
-------------- next part --------------
An HTML attachment was scrubbed...
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 1556 bytes
Desc: not available
More information about the hotspot-gc-dev