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

[Coleen Phillmore]

Hi Thomas,
I'm happy to see this work going forward.

http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/agent/src/share/classes/sun/jvm/hotspot/memory/ProtectionDomainEntry.java.udiff.html 
<http://cr.openjdk.java.net/%7Etschatzl/8003420/webrev.0/cleanup/agent/src/share/classes/sun/jvm/hotspot/memory/ProtectionDomainEntry.java.udiff.html>

Did you also have to add a class for ProtectionDomainCacheEntry?

The code in dictionary.hpp looks good except for the reference 
counting.   Sorry I didn't comment on this before but I was trying to 
think of alternatives.   I think you can treat the protection domain 
table like the StringTable, and unlink the not_alive entries at the 
end.   That avoids the pd cache handing when you're deleting a 
dictionary entry also.   Since these things are oops, we can use GC and 
not something C++ like reference counting.

Some additional minor comments:
http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/src/share/vm/classfile/dictionary.hpp.udiff.html 
<http://cr.openjdk.java.net/%7Etschatzl/8003420/webrev.0/cleanup/src/share/vm/classfile/dictionary.hpp.udiff.html>

Can you capitalize first letter of comments and line up the {} in 
functions that are next to each other?   Maybe webrev isn't showing the 
indentation right.

http://cr.openjdk.java.net/~tschatzl/8003420/webrev.0/cleanup/src/share/vm/runtime/globals.hpp.udiff.html 
<http://cr.openjdk.java.net/%7Etschatzl/8003420/webrev.0/cleanup/src/share/vm/runtime/globals.hpp.udiff.html>

We are trying to get away from having tunable parameters that know that 
there's a protection domain oop cache that is a hash table. Can you make 
this size a ratio of PredictedLoadedClassCount?   This hashtable should 
be subject to resizing when we do resizing for the other tables.

Also, I will double check that classes on the boot class loader never 
have protection domain sets.   I believe that's the case. 
is_strongly_reachable in Dictionary also checks !ClassUnloading but 
checking that can be hoisted above the loop.

Thanks,
Coleen

On 9/25/2013 6: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.
> - 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?
>
> Thanks,
> Thomas
>