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

[Bengt Rutisson]

Hi Per,

On 2014-04-10 15:34, 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/
>
> Diff against previous webrev:
> http://cr.openjdk.java.net/~pliden/8037112/webrev.3-diff_2vs3/

Looks good.

Bengt

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