RFR (M): 8003420: NPG: make new GC root for pd_set

[Ioi Lam]

HI Thomas,

The changes look good to me. Thanks for doing the clean up and adding 
verificationcode.

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
> 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:
> http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/orig/
>
> One containing my cleanup and that is subject to this review is at
> http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/
>
> 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
>
> Bugs.openjdk.java.net
> https://bugs.openjdk.java.net/browse/JDK-8003420
>
> 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?

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
(gdb) c
Breakpoint 2, Dictionary::add_protection_domain (this=0xf702fbd0, 
index=273,
     hash=8004670, klass=..., loader_data=0xe3923ac0, protection_domain=...,
     __the_thread__=0xf7008400)
393      Symbol*  klass_name = klass->name();
(gdb) n
(gdb) p klass_name->print_on(tty)
Symbol: 'java/lang/Object
(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 
'sun/misc/Launcher$AppClassLoader'

The usage of the PD table can be seen in 
SystemDictionary::validate_protection_domain()

The PD table is essentially a cache to avoid repeated Java up-calls to
ClassLoader.checkPackageAccess().

Hmm, come to think of it, maybe it's OK to completely drop the PD table, 
since
it is just a cache???

Thanks

- Ioi
> Thanks,
> Thomas
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130925/2eea7ec2/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: G1Checker2.java
Type: text/x-java
Size: 1556 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130925/2eea7ec2/G1Checker2.java>