RFR(s): 8037112: gc/g1/TestHumongousAllocInitialMark.java caused SIGSEGV

[Mikael Gerdin]

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.