RFR(s): 8037112: gc/g1/TestHumongousAllocInitialMark.java caused SIGSEGV
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Apr 4 12:28:17 UTC 2014
Per,
On Friday 04 April 2014 14.13.56 Per Liden wrote:
> Thanks Mikael!
>
> Updated webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.1/
Looks good.
/Mikael
>
> /Per
>
> On 04/03/2014 03:59 PM, Mikael Gerdin wrote:
> > Hi Per,
> >
> > On Thursday 03 April 2014 13.15.12 Per Liden wrote:
> >> Hi,
> >>
> >> Could I please have this bugfix reviewed.
> >>
> >> Summary: G1's ConcurrentMarkThread tries to use the gclog_or_tty when
> >> the JVM is about to exit and the gclog_or_tty instance has been
> >> destroyed. To prevent this, the ConcurrentMarkThread needs to join the
> >> SuspendibleThreadSet before accessing gclog_or_tty.
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8037112
> >> Webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.0/
> >
> > While I realize that this code is still semantically correct I think it
> > reads a bit strange. In my mind if we have a
> > if (...) {} else if (...) {} else {} block the checks in the "if" and
> > "else
> > if" should usually somehow related checks.
> >
> > 282 if (!cm()->has_aborted()) {
> > 283 g1_policy->record_concurrent_mark_cleanup_completed();
> > 284 } else if (G1Log::fine()) {
> > 285 gclog_or_tty->date_stamp(PrintGCDateStamps);
> > 286 gclog_or_tty->stamp(PrintGCTimeStamps);
> > 287 gclog_or_tty->print_cr("[GC concurrent-mark-abort]");
> > 288 }
> >
> > I would prefer if it was structured the other way around:
> > if (cm()->has_aborted()) {
> >
> > if (G1Log::fine()) { ... }
> >
> > else {
> >
> > g1_policy->record_concurrent_mark_cleanup_completed();
> >
> > }
> >
> > The rest of the change looks good.
> >
> > /Mikael
> >
> >> Thanks!
> >> /Per
> >>
> >> Btw, I created a separate RFE, JDK-8039147, to do a little bit of
> >> cleanup of the SuspendibleThreadSet and how it's used. Will send that
> >> out as separate review request shortly.
More information about the hotspot-gc-dev
mailing list