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

[Ioi Lam]

Hi Thomas,

Looks good.

I have modified the comments slightly. I originally used the => sign to 
indicate some sort of mapping, which may be confusing. I think it's 
better to refer an element of the PD set to be a tuple:

----------------
   // This protection domain set is a set of tuples:
   //
   // (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 is okay for a class Foo to reference C, where
   //
   //    Foo.protection_domain() == PD, and
   //    Foo's defining class loader == ICL
   //
   // The usage of the PD set can be seen in 
SystemDictionary::validate_protection_domain()
   // It is essentially a cache to avoid repeated Java up-calls to
   // ClassLoader.checkPackageAccess().
   //
----------------

The new unlink code is indeed much cleaner.

Thanks
- Ioi


On 10/02/2013 05:21 AM, Thomas Schatzl wrote:
> Hi all,
>
>    I tried to incorporate all suggestions so far, here is a list of the
> changes:
>
> - fixing the capitalization of the first letter of the comments
> - added Ioi's comment about the protection domain table
> - implemented Coleen's suggestions to remove the reference counting, and
> use an unlink-pass after class unloading. Class unloading (i.e. in the
> SystemDictionary::do_unloading() method) is the only case where we need
> to unlink, as in all other cases we use the whole contents of the PD
> cache table as roots anyway.
> All collectors call SystemDictionary::do_unloading().
> - added a newline after the printout of the PD cache table
> - removed the ProtectionDomainCacheSize global as the sizes have been
> tested to be appropriate, and automatic sizing will be implemented
> later.
> - added missing ProtectionDomainCacheEntry Java source file for the
> agent to the webrev
> - tried to group methods "naturally"
> - fixed a few "ProctectionDomain*" typos
>
> (note that globals.hpp and arguments.cpp files have no change in the
> current revision. Webrev apparently picked them up because in earlier
> revisions there has been some).
>
> Thanks everyone, looks much better now, especially removing the
> reference counting.
>
> Bugs.openjdk.java.net
> https://bugs.openjdk.java.net/browse/JDK-8003420
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8003420/webrev.1/
>
> Testing:
>
> jprt, manual verification of all operations (add, unlink), dacapo/scala
> benchmarks with VerifyBefore/AfterGC; have some Aurora tests for the
> vm.parallel_class_loading.testlist vm.quick.testlist vm.runtime.testlist
> in the queue.
>
> Some additional comments inlined.
>
> On Thu, 2013-09-26 at 09:32 -0700, Coleen Phillmore wrote:
>> 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,
>>>> Some additional minor comments:
>> 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.
> That's true, the tested values were fine. For the sake of simplicity I
> removed the global and use the default values during instantiation.
>
>>>> 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.
> I tried Ioi's test program, but did not get any successful
> is_strongly_reachable() check in always_strong_oops_do(). Ioi, is there
> something special I need to do (command line?)?
>
> I kept the existing code for always_strong_oops_do(). Any optimizations
> for finding the PDs quickly when class unloading (during
> always_strong_oops_do) cannot come quick enough :)  We are working on
> class unloading during concurrent marking for g1, and this will be an
> issue.
>
>>>> 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,
> Thomas
>
>