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

[Ioi Lam]

On 09/26/2013 04:58 AM, Thomas Schatzl wrote:
> On Wed, 2013-09-25 at 12:11 -0700, Ioi Lam wrote:
>> 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.

Oops, sorry I was confused. Anyway, your diff page flagged a change in
this line -- maybe there's some white-space only change there?

http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/diff/src/share/vm/utilities/globalDefinitions.hpp.sdiff.html

>> 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?
This sounds good.
>> 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.
Yes, please do that, for the benefit of the next victim that needs
to modify this code :-)
>>    (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 :)
>
The cache is used only when you load classes, perform constant pool
resolution, etc. So once your app stops loading classes, this cache will
be infrequently used. In fact, I think many entries will never be used
again once the program has run to a steady state. Dropping the cache
will actually free up a small amount of memory.

How about dropping the cache when GC is about to move objects. How often
doesthis happen?

If GC is not moving objects, we don't need to go through the pd_set to
do marking, because the ProtectionDomain objects would marked by other
references to them (I think ...).

After a GC, parts of the cache would be rebuilt, but you don't have to
rebuild everything that was dropped -- you only need those entries
required by recently loaded classes.


Alternatively --

I keep thinking maybe we don't need the complexity here. If all we need are
{C, ICL, PD} tuples, maybe we can store a constant unique ID for each PD,
and store that in the pd_set instead?I.e.,

    void DictionaryEntry::add_protection_domain(oop protection_domain) {
      ...
      if (!contains_protection_domain(protection_domain)) {
        ProtectionDomainEntry* new_head =
    //WAS           new ProtectionDomainEntry(protection_domain, _pd_set);
                    new 
ProtectionDomainEntry(protection_domain->uniq_id() _pd_set);
      }
      ...

The unique ID can be generated by a monotonously increasing
64-bit counter, which will take thousands years to overflow
even if you create a new protection domain every millisecond.

The tricky part is we have to modify java.security.ProtectionDomain
to add thisfield, or add a new INJECTED_FIELDS macro in java (yuck!).

- Ioi