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
