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

[Per Liden]

Thanks Mikael!

Updated webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.1/

/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.
>