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

[Thomas Schatzl]

Hi Coleen,

On Thu, 2013-09-26 at 08:03 -0700, Coleen Phillmore wrote:
> 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?

Yes, sorry, I forgot to add it to the local repo.

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

So at the end of GC, going through the PD cache table and remove
non-live ones? That's a good idea.

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

Will fix the capitalization and indentation. When lining up, did you
mean the three *_strongly_reachable() methods from
ProtectionDomainCacheEntry?

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

The ratio would be very small though (maybe 1:100 or even smaller), I
fear this will not work well without resizing support in the PD cache
table. The PD cache table does not support resizing yet, but I could add
that to fix bad decisions?

> Also, I will double check that classes on the boot class loader never 
> have protection domain sets.   I believe that's the case.

Ioi had a test case that showed that, although I did not have time to
check that.

> is_strongly_reachable in Dictionary also checks !ClassUnloading but 
> checking that can be hoisted above the loop.

Thanks for your review. I will start working on these issues and will
send another webrev as they are finished.

Thomas