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

[Thomas Schatzl]

Hi,

On Wed, 2013-09-25 at 12:11 -0700, Ioi Lam wrote:
> HI Thomas,
> 
> The changes look good to me. Thanks for doing the clean up and adding
> verification code.

 thanks.

> 
> One minor point here:
> 
>  const int minimumStringTableSize = 1009;
> 
> shouldn't this be "NOT_LP64(137) LP64_ONLY(1009)"? or maybe just
> "137"?

That's about the string table size, not the protection domain cache
table size. I only fixed the spacing there.

> 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 run a simple tests that causes a lot of GC, like the
> G1Checker2.java
> I attached in this mail.

That's great to hear, and thanks for the example program. I already have
a change anyway that can handle that: it simply has two
ProtectionDomainCacheTables, one for PD cache entries referenced by the
boot/NULL class loader, one for the others.

PD cache entries are juggled around as needed, i.e. if there is at least
one reference by a class of the NULL class loader it will be moved into
the NULL class loader PD cache table.

Actually, if we do not care about wasting a little memory (duplicate
ProtectionDomainTableEntries) we could just have completely separate PD
table caches for the NULL class loader klasses and the others.

Then during gc, when class loading is enabled, you can find the ones
referenced to by the NULL class loader quickly too.

What do you think?

> The meaning of the PD table is confusing at best. Perhaps we should
> add some comments :-)

Definitely :)

> The PD table is a mapping of:
> 

With PD table you mean the _pd_set member of DictionaryEntry basically;
I can put the documentation you provided here there.

>   (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???

I assume that the cache has a purpose, i.e. don't drop to avoid
performance regressions :)

Thanks a lot,
  Thomas