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

[Per Liden]

Hi,

Here's an updated webrev which fixes the potential dead lock issue I 
found. I've taken a whole new approach to this problem. Instead of 
joining/leaving the STS as protection from shutdown I've added a 
CollectedHeap::stop() which is called in the shutdown paths (in 
before_exit()) to stop and take down the concurrent threads in a 
controlled manner. This feel like a much cleaner approach and less error 
prone as it doesn't require any future log printouts to be guarded by 
join/leave.

Webrev: http://cr.openjdk.java.net/~pliden/8037112/webrev.2/

cheers,
/Per

On 04/04/2014 06:14 PM, Per Liden wrote:
> 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.
>>
>