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, 8:41 AM, Coleen Phillimore wrote:
>
> 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.

Sorry, I mean _saved_unloading.
Coleen

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