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

[serguei.spitsyn at oracle.com]

Hi Markus,

I'm Ok with the "_saved_unloading" if you prefer it.

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