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
