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

[Mikael Gerdin]

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.