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

[Coleen Phillmore]

Some clarifications.   Thanks Thomas!

On 9/26/2013 8:53 AM, Thomas Schatzl wrote:
> 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.

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

It's hard to tell from the webrev - just line up stuff that looks like 
it should be - how about that?
>
>> 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?

Resizing is going to be added later so don't do it now for this because 
we want to resize all of these related hashtables the same way.

The likelihood of bad decisions seems rather low, because this is a 
relatively small hash table so unless our performance team tested a 
larger value (they or related groups using FMW also use 
PredictedLoadedClassCount), I would not expose this value.   If they did 
use it, use some ratio based on PredictedLoadedClassCount.

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

I believe it.

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

Thanks!
Coleen

>
> Thomas
>
>