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

[Coleen Phillmore]

Some comments below.

On 9/26/2013 9:32 AM, Ioi Lam wrote:
> 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

We do care about wasting memory...   I assume this change is future 
investigation and not part of Thomas's change.   Please don't add other 
protection domain table with this.

>> ProtectionDomainTableEntries) we could just have completely separate PD
>> table caches for the NULL class loader klasses and the others.
>>

Yes, this is future work.   We could also represent the system 
dictionary per-classloader as well as the protection domain sets.

>> 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!).

Yeah, needs further investigation if it has a benefit.   I think 
Thomas's change will speed things up nicely now.
Thanks,
Coleen

>
> - Ioi
>
>
>