[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(S): 8047812: Ensure ClassLoaderDataGraph::classes_unloading_do only delivers klasses from CL
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2014-06-24 7:58:37
Message-ID: 53A92FAD.4030602 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]