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

[Per Liden]

Hmm, I discovered a potential dead lock with this approach, so please hold reviews on this for now.

Sorry!
/Per

On 04 Apr 2014, at 14:28, Mikael Gerdin <mikael.gerdin at oracle.com> wrote:

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