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

[Coleen Phillmore]

Thomas,
This looks really good.  Thank you for the cleanups - this came out well.
(Ioi, thank you for the initial analysis and implementation of the 
hashtable).
Coleen

On 10/2/2013 8: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
>
>