RFR(S): 8047812: Ensure ClassLoaderDataGraph::classes_unloading_do only delivers klasses from CLDs with non-reclaimed class loader oops

[Coleen Phillimore]

On 6/24/14, 3:58 AM, serguei.spitsyn at oracle.com wrote:
> Hi Markus,
>
> I'm Ok with the "_saved_unloading" if you prefer it.

Yes, I prefer it also because of _saved_head, also used for CMS.

Coleen

>
> Thanks!
> Serguei
>
> On 6/24/14 12:49 AM, Markus Grönlund wrote:
>> Hi Serguei,
>>
>> Thanks for taking a look and coming up with some suggestions on 
>> renaming the field.
>>
>> In regards to naming, I have followed the existing pattern for CMS 
>> support (see "_saved_head" field).
>>
>> As always on naming, I think it depends on which perspective you 
>> take: if you take the view of an outside consumer of the iterator 
>> function, then maybe "_processed_unloading" might be a useful name 
>> for the file (but this only as "processed" in regards to "delivered 
>> via callbacks" sense of the term).
>>
>> But, if we take the GC centric view (which this does since it add 
>> specialization for CMS GC), then the list is "saved" for by each GC 
>> thread enter until the point of purging. We have still not really 
>> reached the point where the rationale for this list (_unloading) in 
>> used - and that is during "purge" where all the CLDs reachable via 
>> the _unloading list are deallocated.
>>
>> So the other candidate I would consider would be something like 
>> "_previous_unloading", but this is close enough to "_saved_unloading".
>>
>> For these reasons I would suggest going with "_saved_unloading".
>>
>> Thanks for the suggestions though.
>>
>> /Markus
>>
>>
>> -----Original Message-----
>> From: Serguei Spitsyn
>> Sent: den 24 juni 2014 02:20
>> To: Markus Grönlund; hotspot-runtime-dev; serviceability-dev
>> Subject: Re: RFR(S): 8047812: Ensure 
>> ClassLoaderDataGraph::classes_unloading_do only delivers klasses from 
>> CLDs with non-reclaimed class loader oops
>>
>> Markus,
>>
>> It looks correct in general and the comments are good.
>>
>> However, it makes it a little bit more complex.
>> I'd suggest to rename the "_saved_unloading" to something more 
>> meaningful.
>> What about "_iterated_unloading", "_processed_unloading" or 
>> "_posted_unloading" ?
>> No pressure, just some thinking. :)
>>
>> Thanks,
>> Serguei
>>
>> On 6/23/14 8:02 AM, Markus Grönlund wrote:
>>> Greetings,
>>>
>>>
>>> Kindly asking for reviews for the following change:
>>>
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8047812
>>>
>>> Webrev: http://cr.openjdk.java.net/~mgronlun/8047812/webrev01
>>>
>>>
>>> Description:
>>>
>>> The "8038212: Method::is_valid_method() check has performance 
>>> regression
>>>     impact for stackwalking" - changeset introduced a change in how 
>>> the ClassLoaderDataGraph::_unloading list of ClassLoaderData's is 
>>> purged.
>>>
>>> This change to the purging of the CLD's work the same as before for 
>>> most GC's, but when using CMS GC, SystemDictionary::do_unloading() 
>>> is called twice with no explicit purge call in between. On the 
>>> second call (post-sweep), we can now get stale class loader oops 
>>> delivered as part of the Klass closure callbacks from the _unloading 
>>> list. Again, this is because there is no explicit purge call in 
>>> between these two entries to SystemDictionary::do_unloading() - and 
>>> being CMS and concurrent, it is very hard to accommodate a timely 
>>> and proper purge call here.
>>>
>>> The first do_unloading call comes after CMS concurrent marking, and 
>>> the second comes from a Full GC triggered while sweeping the CMS heap.
>>>
>>> This fix ensures the unloading purge mechanism to work correctly 
>>> also for the CMS collector, in that only CLDs with non-reclaimed 
>>> class loader oops will deliver klasses from the _unloading list. In 
>>> addition, this will ensure a single "logical" pass is achieved when 
>>> iterating the unloading list in-between purges (avoiding the 
>>> processing of the same data twice).
>>>
>>> This fix is precipitated by nightly testing failures with CMS after 
>>> the introduction of 8038212: Method::is_valid_method() check has 
>>> performance regression
>>>     impact for stackwalking" - for example 
>>> "nsk/sysdict/vm/stress/jck12a//sysdictj12a008" which is crashing 
>>> because of following up stale klass loader oop's from the 
>>> ClassLoaderDataGraph::_unloading list.
>>>
>>>
>>> Thanks
>>>
>>> Markus
>