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

[Per Liden]

Thanks Bengt, thanks Mikael!

/Per

On 04/11/2014 09:57 AM, Mikael Gerdin wrote:
> Per,
>
> On Thursday 10 April 2014 15.34.03 Per Liden wrote:
>> Bengt reviewed this off-line and had one comment, which was that we
>> should also stop G1's string dedup thread if it's running. The dedup
>> thread is not affected by the original problem so stopping it is
>> strictly not needed, but for completness we still want to do it.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~pliden/8037112/webrev.3/
>
> The change looks good to me.
> /Mikael
>
>>
>> Diff against previous webrev:
>> http://cr.openjdk.java.net/~pliden/8037112/webrev.3-diff_2vs3/
>>
>> /Per
>>
>> On 04/08/2014 11:27 AM, Per Liden wrote:
>>> 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.
>